Make copy[_nonoverlapping] const #79684

usbalbin · 2020-12-04T01:57:39Z

Constifies

intrinsics::copy and intrinsics::copy_nonoverlapping
ptr::read and ptr::read_unaligned
- *const T::read and *const T::read_unaligned
- *mut T::read and *mut T::read_unaligned
MaybeUninit::assume_init_read

rust-highfive · 2020-12-04T01:57:42Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

usbalbin · 2020-12-04T01:59:47Z

library/core/src/intrinsics.rs

        fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: usize);
    }

-    if cfg!(debug_assertions)
+    // FIXME: Is it possible to make this work in const fn?
+    /*if cfg!(debug_assertions)
        && !(is_aligned_and_not_null(src)
            && is_aligned_and_not_null(dst)
            && is_nonoverlapping(src, dst, count))


Is it possible to make this work in const fn? Is there any overlap with what is done in compiler/rustc_mir/src/interpret/intrinsics.rs, or is that only affecting CTFE?

is_aligned_and_not_null() and is_nonoverlapping() both seems to do some *const _ as usize plus math which I believe is a no no during CTFE?

For is_nonoverlapping you could create another intrinsic that implements the logic in the const evaluator, but then you'd have to implement a normal codegen version of that, too.

The other two can actually be implement in a const-evaluable way with https://doc.rust-lang.org/std/primitive.pointer.html#method.align_offset

That said... Since the const-eval intrinsic checks all these invariants anyway, what we can do is to finally introduce a way to run different code at runtime vs at compile-time rust-lang/const-eval#7 (comment)

This is as good as a PR to do that as any. Do you want to tackle that or is it more than you wanted on your plate right now? It may be more effective to do just the new intrinsic in a separate PR so we have some unit tests before we use it in copy_nonoverlapping

I am really new working on the compiler and do not really know what to do. If you think it might be a not-too-great first thing to implement then I will happily start with something else. Otherwise, if I knew where to start and given some (probably lots of) pointers along the way... Maybe I could give it a try, but I would not want to promise anything and am not sure about when and how much time I can spend on it. I certainly would not want to stand in the way for anyone else who want to give it a go.

I just did a dive into the compiler to look at what needs to be done and while the const eval parts are fairly self contained, the codegen parts are not, so... I don't think we can make copy_nonoverlapping const fn for now, but copy could still be doable by replacing the implementation of the debug assertion functions at

rust/library/core/src/intrinsics.rs

Line 1749 in d015f0d

pub(crate) fn is_aligned_and_not_null<T>(ptr: *const T) -> bool {

with a const fn capable one. This should be doable by using guaranteed_ne for the null pointer check and align_offset for the alignment check. (you should be able to experiment with this on the playground, so you don't have to keep recompiling libcore).

Would it be useful to continue working on this PR draft, or open a more restricted one, as an example use case/background for the problem and one potential solution?

For this PR, I see two options:

Leave it as "something we can do once we have a story for const-dependent dispatch".

Comment out the debug assertions for now. Their usefulness is anyway limited since the libstd everyone uses is compiled without debug assertions.

I guess the question is one of evaluating the relative usefulness of these new const operations vs the assertions.

For this PR, I see two options:

Leave it as "something we can do once we have a story for const-dependent dispatch".

Comment out the debug assertions for now. Their usefulness is anyway limited since the libstd everyone uses is compiled without debug assertions.

I guess the question is one of evaluating the relative usefulness of these new const operations vs the assertions.

I have constified ptr::read[_unaligned] and ptr::write locally by commenting out the call to is_nonoverlapping and align_offset. I assume we can always remove the comments to bring back the checks and wait for const-dependent dispatch should we go for the latter option, right? ptr::read[_unaligned] requires #79621, should I rebase and push my changes (does not include the unleash_the_miri_inside_of_you_here attribute)?

ptr::read[_unaligned] requires #79621, should I rebase and push my changes (does not include the unleash_the_miri_inside_of_you_here attribute)?

yes, that sounds right

by commenting out the call to is_nonoverlapping and align_offset

Do you mean is_aligned_and_not_null? Which align_offset call?

usbalbin · 2020-12-07T12:45:23Z

Blocked on some sort of const align_offset

Edit: Maybe not?

rust-log-analyzer · 2020-12-21T13:33:00Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/library/core/src/intrinsics.rs at line 1858:
     }
 
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/library/core/src/intrinsics.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
     if cfg!(debug_assertions)
-        && !(is_aligned_and_not_null(src)
-            && is_aligned_and_not_null(dst)
-            /*&& is_nonoverlapping(src, dst, count)*/)
+        && !(is_aligned_and_not_null(src) && is_aligned_and_not_null(dst)/*&& is_nonoverlapping(src, dst, count)*/)
     {
         // Not panicking to keep codegen impact smaller.
         abort();
Build completed unsuccessfully in 0:00:21

RalfJung · 2020-12-21T14:20:36Z

compiler/rustc_mir/src/interpret/intrinsics.rs

@@ -322,6 +322,24 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                let result = Scalar::from_uint(truncated_bits, layout.size);
                self.write_scalar(result, dest)?;
            }
+            sym::copy_nonoverlapping => {


Don't you need to also implement copy and move_val_init?

See https://github.com/rust-lang/miri/blob/master/src/shims/intrinsics.rs for the Miri implementation.

Oh, I forgot that I reverted copy. Thanks :)

compiler/rustc_mir/src/interpret/intrinsics.rs

RalfJung · 2020-12-21T14:21:52Z

library/core/src/ptr/mod.rs

@@ -880,7 +882,8 @@ pub unsafe fn read_unaligned<T>(src: *const T) -> T {
 /// ```
 #[inline]
 #[stable(feature = "rust1", since = "1.0.0")]
-pub unsafe fn write<T>(dst: *mut T, src: T) {
+#[rustc_const_unstable(feature = "const_ptr_write", issue = "none")]
+pub const unsafe fn write<T>(dst: *mut T, src: T) {


See this Zulip thread -- I think we can implement this without the intrinsic, and that seems more consistent with ptr::read.

Ok. Should I make move_val_init const then?

Maybe leave out ptr::write for now until that Zulip thread reaches a conclusion.

Sure.

Possibly relevant to the Zulip discussion? - The compiler does not seem too happy about the reference to src in const fn context

error[E0492]: cannot borrow a constant which may contain interior mutability, create a static instead --> library/core/src/ptr/mod.rs:890:29 | 890 | copy_nonoverlapping(&src as *const T as *const u8, dst as *mut u8, mem::size_of::<T>()); | ^^^^

Ah... that's another issue that @oli-obk recently mentioned.

Yet another reason to maybe hold off on ptr::write for now.^^

library/core/src/intrinsics.rs

RalfJung · 2020-12-22T12:13:31Z

LGTM -- except that we should also have some tests, to make sure that this works and to demonstrate that this can actually be called. :)

Also, can you squash the commits together? Most of that internal history doesn't seem relevant long-term.

usbalbin · 2020-12-22T15:20:50Z

LGTM -- except that we should also have some tests, to make sure that this works and to demonstrate that this can actually be called. :)

Also, can you squash the commits together? Most of that internal history doesn't seem relevant long-term.

The current history obviously is quite far from nice (lots of reverts and reverts of reverts). On one hand I do not want to confuse anyone looking at the code by force pushing all the time. On the other hand I do not want my own confusion to affect others by not cleaning up my history at all. Do you have any sort of general rule of thumb in regards to squashing of commits/rebasing?

In this case how many commits would you prefer? A single one, or more like 3ish: add intrinsics::copy, add ptr::read, add test?

usbalbin · 2020-12-22T15:37:35Z

Oh I forgot to constify read and read_unaligned in const_ptr.rs and mot_ptr.rs is it ok if I add them?

Also I believe MaybeUninit::assume_init_read can be constified thanks to *const::read, may I add that one too :)

As for tests, would this do, but for read_unaligned as well?

#[test]
fn const_read() {
    let foo = 42;
    assert_eq!(read_ptr(&foo), foo);

    const fn read_ptr(x: &i32) -> i32 {
        // SAFETY: x is a reference which is garantueed to be safe to read from
        // thus the same goes for the pointer
        unsafe{ read(x as *const i32) }
    }
}

If I constify read in const_ptr.rs and mot_ptr.rs, should all those combinations be tested as well?

usbalbin · 2020-12-25T01:17:29Z

@RalfJung

LGTM -- except that we should also have some tests, to make sure that this works and to demonstrate that this can actually be called. :)

Are there any sort of general guidelines in regards to testing when constifying code like this? Should one first and foremost test that the added intrinsics works as expected, as that is the only actually changed code(except sprinkling const and adding some attributes)? Or should one also test that all the constified (non intrinsic) functions are usable in const context?

oli-obk · 2020-12-25T18:13:05Z

Are there any sort of general guidelines in regards to testing when constifying code like this? Should one first and foremost test that the added intrinsics works as expected, as that is the only actually changed code(except sprinkling const and adding some attributes)?

You should not need to change any of the runtime tests, they will keep working as intended, as any const code will be usable in non-const code. (Unless there were some tests checking for the debug assertion triggering, which I don't believe we can even write tests for).

Or should one also test that all the constified (non intrinsic) functions are usable in const context?

Yes, since the change is that these functions are now (in addition to working in runtime code) callable in const contexts, some tests that test them in const contexts would be great. In this case ideally some tests that work as intended, and a separate test file which explores the edge cases (unaligned pointers, out of bound pointers, ...).

usbalbin · 2021-01-04T17:53:59Z

I just had a look at the const eval skill tree, and saw the ptr::copy_nonoverlapping thing. If I am reading the code right, this is the function in library/core/src/intrinsics.rs but reexported by library/core/src/ptr/mod.rs. Do we want a tracking issue for tracking the constification of copy_nonoverlapping(and copy?) that should be linked to in the skill tree?

oli-obk · 2021-01-04T18:02:14Z

Do we want a tracking issue for tracking the constification of copy_nonoverlapping(and copy?) that should be linked to in the skill tree?

Oh, good catch. Yes both a tracking issue and updating the skill tree would be great!

…=oli-obk const_intrinsic_copy - Add Reference to tracking issue Add reference to tracking issue rust-lang#80697 for feature gate added in previous PR rust-lang#79684

…ark-Simulacrum remove some outdated comments regarding debug assertions rust-lang#79684 removed those debug assertions.

…Mark-Simulacrum Make copy/copy_nonoverlapping fn's again Make copy/copy_nonoverlapping fn's again, rather than intrinsics. This a short-term change to address issue rust-lang#84297. It effectively reverts PRs rust-lang#81167 rust-lang#81238 (and part of rust-lang#82967), rust-lang#83091, and parts of rust-lang#79684.

This commit re-enables the debug checks for valid usages of the two functions `copy()` and `copy_nonoverlapping()`. Those checks were com- mented out in rust-lang#79684 in order to make the functions const. All that's been left was a FIXME, that could not be resolved until there is was way to only do the checks at runtime. Since rust-lang#89247 there is such a way: `const_eval_select()`. This commit uses that new intrinsic in order to either do nothing (at compile time) or to do the old checks (at runtime). The change itself is rather small: in order to make the checks usable with `const_eval_select`, they are moved into a local function (one for `copy` and one for `copy_nonoverlapping` to keep symmetry). The change does not break referential transparency, as there is nothing you can do at compile time, which you cannot do on runtime without get- ting undefined behavior. The CTFE-engine won't allow missuses. The other way round is also fine.

…acrum Re-enable `copy[_nonoverlapping]()` debug-checks This commit re-enables the debug checks for valid usages of the two functions `copy()` and `copy_nonoverlapping()`. Those checks were commented out in rust-lang#79684 in order to make the functions const. All that's been left was a FIXME, that could not be resolved until there is was way to only do the checks at runtime. Since rust-lang#89247 there is such a way: `const_eval_select()`. This commit uses that new intrinsic in order to either do nothing (at compile time) or to do the old checks (at runtime). The change itself is rather small: in order to make the checks usable with `const_eval_select`, they are moved into a local function (one for `copy` and one for `copy_nonoverlapping` to keep symmetry). The change does not break referential transparency, as there is nothing you can do at compile time, which you cannot do on runtime without getting undefined behavior. The CTFE-engine won't allow missuses. The other way round is also fine. I've refactored the code to use `#[cfg(debug_assertions)]` on the new items. If that is not desired, the second commit can be dropped. I haven't added any checks, as I currently don't know, how to test this properly. Closes rust-lang#90012. cc `@rust-lang/lang,` `@rust-lang/libs` and `@rust-lang/wg-const-eval` (as those teams are linked in the issue above).

rust-highfive assigned matthewjasper Dec 4, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 4, 2020

usbalbin commented Dec 4, 2020

View reviewed changes

usbalbin force-pushed the const_copy branch 2 times, most recently from cffd65a to 2b7adcb Compare December 5, 2020 02:08

DutchGhost mentioned this pull request Dec 5, 2020

Tracking issue for const slice::from_raw_parts_mut (const_slice_from_raw_parts_mut) #67456

Closed

1 task

usbalbin closed this Dec 7, 2020

usbalbin reopened this Dec 18, 2020

usbalbin force-pushed the const_copy branch from a8871e2 to 895854a Compare December 21, 2020 13:19