5

Github Make copy[_nonoverlapping] const by usbalbin · Pull Request #79684 · rust...

 3 years ago
source link: https://github.com/rust-lang/rust/pull/79684
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.

Collaborator

rust-highfive commented on Dec 4, 2020

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.

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

usbalbin on Dec 4, 2020

Author

Contributor

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?

usbalbin on Dec 4, 2020

Author

Contributor

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?

oli-obk on Dec 4, 2020

Contributor

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

usbalbin on Dec 5, 2020

Author

Contributor

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.

oli-obk on Dec 5, 2020

edited

Contributor

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

usbalbin 28 days ago

Author

Contributor

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?

RalfJung 28 days ago

Member

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.

usbalbin 26 days ago

Author

Contributor

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

oli-obk 26 days ago

Contributor

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

RalfJung 26 days ago

Member

by commenting out the call to is_nonoverlapping and align_offset

Do you mean is_aligned_and_not_null? Which align_offset call?


About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK