try_cast_aligned: avoid bare int-to-ptr casts by RalfJung · Pull Request #141381 · 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
Conversation15 Commits1 Checks6 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 }})
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
labels
I totally missed that..
I think that without_provenance make the examples less readable (in particular for the NonNull) so I was trying to think of another example that would use provenance but I guess it is not easy to create a reference to an object that is for sure not properly aligned (for a larger aligment).
it is not easy to create a reference to an object that is for sure not properly aligned (for a larger aligment).
The best way is to do something like
let x = &0i32; // guaranteed 4-aligned let ptr = (x as *const i32).cast::().add(1); // guaranteed not 4-aligned
I think that without_provenance make the examples less readable
Maybe they do, but the original example is not representative of how we want anyone to write Rust code. int2ptr casts are extremely rarely needed. This is the only test (across all doc tests, unit tests, integration tests) in all of core, alloc, and the parts of std that Miri can run that uses them!
The best way is to do something like
let x = &0i32; // guaranteed 4-aligned let ptr = (x as *const i32).cast::().add(1); // guaranteed not 4-aligned
it is less expressive than raw adresses that are obviously well/not aligned.
Do you think this is better than using raw int2ptr? In particular regarding your next comment:
Maybe they do, but the original example is not representative of how we want anyone to write Rust code. int2ptr casts are extremely rarely needed. This is the only test (across all doc tests, unit tests, integration tests) in all of core, alloc, and the parts of std that Miri can run that uses them!
I totally agree. According to you, how big of a problem is it ? Maybe we should totally remove such a cast to be more idiomatic?
It's a problem because it causes CI in https://github.com/rust-lang/miri-test-libstd/ to fail. We don't have a good way of allowing such casts in a single test -- and even if we did, I don't think we should, since we consider such casts to be very unidiomatic (except in special situations such as MMIO accesses to hardware registers at fixed addresses). So I think everything is better than using int2ptr casts.
If you think the examples I proposed are too unreadable, I'd encourage you to write alternative examples that also avoid int2ptr casts. Then we can see which one @tgross35 prefers. :)
it is less expressive than raw adresses that are obviously well/not aligned.
this is confusing to me. offsetting a pointer to a 4-byte object by 1 byte feels quite expressive, because it expresses it without drawing attention to irrelevant minutiae like the specific address.
What I wanted to say is that I found the without_provenance example more readable than using references and cast+add, but it is unclear to me wether without_provenance is included in the unidiomatic int2ptr casts.
If yes, then maybe we should use references, if no, then let’s keep it like you originally proposed.
To be extra clear: all your examples are very readable, Im just trying to figure out which one is the best
this is confusing to me. offsetting a pointer to a 4-byte object by 1 byte feels quite expressive, because it expresses it without drawing attention to irrelevant minutiae like the specific address.
I think that settles it? What about something like this (directly using what @RalfJung wrote):
let x = 0;
let aligned: *const i32 = &x; let unaligned = unsafe { aligned.byte_add(1) };
assert!(aligned.try_cast_aligned::().is_some()); assert!(unaligned.try_cast_aligned::().is_none());
I like because the pointer-creation part doesn't bloat the example (especially since NonNull::from_ref has been stabilised).
The without_provenance example is still very unrealistic - nobody is going to use the API like that. It is probably easier to follow than the example with "real" pointers, but we could mitigate that to some extent with comments. In the end it boils down to the usual trade-off in docs: make them simple vs make them realistic.
let x = 0;
let aligned: *const i32 = &x; let unaligned = unsafe { aligned.byte_add(1) };
assert!(aligned.try_cast_aligned::().is_some()); assert!(unaligned.try_cast_aligned::().is_none());
I like because the pointer-creation part doesn't bloat the example (especially since
NonNull::from_refhas been stabilised).
Yeah, that seems good.
With pointers it can be easy to wander pretty far into the weeds, away from real code that people actually write, so "simplest example that reuses code people actually want to write" seems most helpful to me. The "fact" that pointers "are integers" usually only comes up when you have two of them and then try to do math on their addresses. Alone, each is mostly just its offset from an origin, because you can't actually usefully interact with their address (since it's unstable).
Ahk sorry for missing this. I think I have a slight preference for the example using real pointers rather than magic numbers since I agree that seems more realistic.
let x = 0;
let aligned: *const i32 = &x; let unaligned = unsafe { aligned.byte_add(1) };
assert!(aligned.try_cast_aligned::().is_some()); assert!(unaligned.try_cast_aligned::().is_none());
One note to this example is it would make more sense as let aligned: *const u64 = &0; so unaligned = ptr + 1 would still be valid for a read if the alignment were correct.
📌 Commit 204d174 has been approved by tgross35
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
One note to this example is it would make more sense as let aligned: *const u64 = &0; so unaligned = ptr + 1 would still be valid for a read if the alignment were correct.
That resolves my one concern with the example, so I went for that.
The last push changes a from_ref to from_mut since we're passing it an &mut anyway.
@bors r=tgross35
📌 Commit 09ae053 has been approved by tgross35
It is now in the queue for this repository.
bors added a commit to rust-lang-ci/rust that referenced this pull request
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the library team, which will review and decide on the PR/issue.