Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make copy[_nonoverlapping] const #79684

Merged
merged 6 commits into from
Dec 30, 2020
Merged

Make copy[_nonoverlapping] const #79684

merged 6 commits into from
Dec 30, 2020

Conversation

usbalbin
Copy link
Contributor

@usbalbin usbalbin commented Dec 4, 2020

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
Copy link
Collaborator

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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 4, 2020
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))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@oli-obk oli-obk Dec 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by commenting out the call to is_nonoverlapping and align_offset

Do you mean is_aligned_and_not_null? Which align_offset call?

@usbalbin
Copy link
Contributor Author

usbalbin commented Dec 7, 2020

Blocked on some sort of const align_offset


Edit: Maybe not?

@rust-log-analyzer
Copy link
Collaborator

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

@@ -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 => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Should I make move_val_init const then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>());
    |                             ^^^^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@RalfJung
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 usbalbin changed the title Make ptr::copy[_nonoverlapping] const Make copy[_nonoverlapping] const Dec 22, 2020
@usbalbin
Copy link
Contributor Author

@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
Copy link
Contributor

oli-obk commented Dec 25, 2020

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, ...).

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 30, 2020
@bors bors merged commit bbcaed0 into rust-lang:master Dec 30, 2020
@rustbot rustbot added this to the 1.51.0 milestone Dec 30, 2020
@usbalbin
Copy link
Contributor Author

usbalbin commented Jan 4, 2021

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
Copy link
Contributor

oli-obk commented Jan 4, 2021

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!

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 5, 2021
…=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
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 18, 2021
…ark-Simulacrum

remove some outdated comments regarding  debug assertions

rust-lang#79684 removed those debug assertions.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 19, 2021
…ark-Simulacrum

remove some outdated comments regarding  debug assertions

rust-lang#79684 removed those debug assertions.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 9, 2021
…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.
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 17, 2021
…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.
jfrimmel added a commit to jfrimmel/rust that referenced this pull request Nov 9, 2021
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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 13, 2021
…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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants