Nuke slice_as{,_mut}_ptr methods of MaybeUninit by SUPERCILEX · Pull Request #103133 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation49 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
These methods are of questionable value: they mix together removing bounds checks with converting the MaybeUninit slice into a raw pointer. These two behaviors should be explicitly invoked separately to improve code clarity and safety. There is evidence of unnecessary bounds checking removal in the stdlib uses that are updated in this PR.
In rust-lang/libs-team#122, there is discussion of a need for generalized containers. However, that's going to be a long ways off, so in the meantime this PR adds an into_inner method on MaybeUninit raw pointers to unwrap the raw pointer. These aren't strictly necessary and could be replaced by cast, but then you don't get and type system guarentees.
Relevant to the compiler team, which will review and decide on the PR/issue.
Relevant to the library team, which will review and decide on the PR/issue.
labels
Hey! It looks like you've submitted a new PR for the library teams!
If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.
Examples of T-libs-api changes:
- Stabilizing library features
- Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
- Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
- Changing public documentation in ways that create new stability guarantees
- Changing observable runtime behavior of library APIs
r? @m-ou-se
(rust-highfive has picked a reviewer for you, use r? to override)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could potentially be replaced with a bunch of MaybeUninit::writes, but there are so many of them that I don't want to risk bounds checking perf or use get_unchecked a million times. Though maybe using get_unchecked a million times is actually better because it shows the unsafety?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add is already unsafe, so I guess probs not needed.
This comment has been minimized.
I'm not a libs API person, so I'm the wrong reviewer here.
r? libs
Right, keep forgetting, sorry! :)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds a runtime bounds check where there was none before. Same for all the other places where add became [...].
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. But the unsafety is centered around copy_nonoverlapping and presumably llvm can optimize out the bounds checking since it's done right above?
Happy to remove the bounds checking if that's what reviewers want, but either way get_unchecked is the method designed to express the desire to avoid bounds checking, not pointer addition IMO.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if the bounds checks here are a problem, I only know I took care to not introduce any new bounds checks when this code was originally ported to MaybeUninit.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went back through and removed all the bounds checks that wouldn't be optimized out by the compiler. Anything remaining should be trivially removable.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO into is way to implicit for use around unsafe code. Unsafe code needs clearly stated intent, so just saying "please convert this to something else" is not explicit enough.
So IMO we actually want dedicated methods here, and we need to bikeshed their name. I don't like inner but I also don't have better suggestions at the moment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with into_inner to be consistent with other APIs.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know any API this is consistent with? into_inner is usually for methods of the form Type<T> -> T; I find it extremely surprising to see it here for a raw pointer method.
Something like as_inner_ptr or so maybe?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I don't see *const Type<T> -> *const T as being too different from Type<T> -> T. I guess the difference is that one is a type cast whereas the other is providing a view into a type.
as_ is supposed to be for borrowed types—are we counting pointers as a kind of borrow for naming purposes? If so as_inner_ptr seems great or maybe even just as_inner.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see *const Type -> *const T as being too different from Type -> T
Why not? It makes a huge difference whether ownership is consume vs whether there is a form of "borrowing". We never use into_inner for methods like &Type<T> -> &T or &mut Type<T> -> &mut T. These functions here are very much like them, except they are borrowing via raw pointers, not references.
One similar function we have is UnsafeCell::raw_get. On MaybeUninit we already have as_(mut_)ptr that take references; now here we have the exact same methods using raw pointers so the naming should be similar. raw_as_ptr sounds odd though...
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotya, I wasn't really thinking of pointers as references for naming but you're totally right that makes more sense. raw_as_ptr does seems a little weird, but it would be consistent... will rename.
If you have to add a completely new method to actually replace these methods, then maybe they aren't ready for removal imo. You should add the new method through the correct processes first, and then we can maybe remove these later.
Also I agree with Ralf that these should not be From impls at all.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
Status: Blocked on something else such as an RFC or other implementation work.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
@SUPERCILEX
ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Is thanks still blocked? Thanks!
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
Signed-off-by: Alex Saveau saveau.alexandre@gmail.com
Relevant to the library API team, which will review and decide on the PR/issue.
and removed T-libs
Relevant to the library team, which will review and decide on the PR/issue.
labels
Do we need FCP or something for posterity, or should this just be r+'d? Can I even
r? libs-api
I don't think I like this stripped down version - every new use of 'cast' is a step backwards IMO. These are hard to review since one cannot easily tell what they are being cast *from*.
There is evidence of unnecessary bounds checking removal in the stdlib uses that are updated in this PR.
Which ones did you think were unnecessary? As far as I am aware, most/all of these are ported from old pre-MaybeUninit code that already did not have bounds checks. So the removal was entirely deliberate, and not related to the signature of these methods.
FWIW I agree that with a raw ptr projection method (*mut MaybeUninit<T> -> *mut T) this becomes clearer, albeit more verbose. But reverting to cast strikes me as a bad intermediate step. It makes the code harder to read and it is unlikely to be ported to the raw ptr projection method once that exists (since how would we even find the places where it can be used).
Nuke slice_as{,_mut}_ptr methods Nuke slice_as{,_mut}_ptr methods of MaybeUninit
Which ones did you think were unnecessary?
Take a look at all the places I used slicing operators in num.rs for this commit: f2e9b40 (#103133). All of those should have probably eliminatable bounds checks.
But reverting to cast strikes me as a bad intermediate step.
Yeah, that's fair. So do those two changes need to happen together?
"should have probably" is fairly weak. This is all unsafe code because it is considered performance-sensitive. And this code is old, it used unchecked accesses already before it used MaybeUninit:
Notice all the offset. They were added in ebf9e1a.
So this is definitely not evidence that the current API leads to omitted bounds checks.
So do those two changes need to happen together?
I would personally prefer that. I don't have the power to block changes in this part of the compiler though.
"should have probably"
Typo that autocorrect keeps trying to put in, sorry. Should have provably. As in if they don't get optimized out it's an llvm bug.
I would personally prefer that.
Yeah I think that's reasonable unfortunately. I'll have time to look into this in a month or two again.
Based on the prev. comment switching to waiting on author to incorporate changes. Feel free to request a review with @rustbot ready, thanks!
@rustbot author
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
@SUPERCILEX
ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!
FYI: when a PR is ready for review, send a message containing@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.
This PR is waiting on me to figure out what to do with *{const,mut} MaybeUninit<T> to *{const,mut} T ptr casts. Hoping to have time to think about this in December/January.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
I'll close for now. I still think these methods should be removed, but sadly don't have the time to see that work through right now.
Labels
Status: This is awaiting some action (such as code changes or more information) from the author.
Relevant to the compiler team, which will review and decide on the PR/issue.
Relevant to the library API team, which will review and decide on the PR/issue.