try_cast_aligned: avoid bare int-to-ptr casts

[RalfJung]

This fixes a CI failure in https://github.com/rust-lang/miri-test-libstd caused by strict provenance violations in doctests added in #141222.

r? @tgross35
Cc @mathisbot

[mathisbot]

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

[RalfJung]

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::<u8>().add(1); // guaranteed not 4-aligned

[RalfJung]

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!

[mathisbot]

The best way is to do something like

let x = &0i32; // guaranteed 4-aligned
let ptr = (x as *const i32).cast::<u8>().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?

[RalfJung]

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

[workingjubilee]

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.

[mathisbot]

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

[mathisbot]

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::<u32>().is_some());
assert!(unaligned.try_cast_aligned::<u32>().is_none());

I like because the pointer-creation part doesn't bloat the example (especially since NonNull::from_ref has been stabilised).

[RalfJung]

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.

[workingjubilee]

let x = 0;

let aligned: *const i32 = &x;
let unaligned = unsafe { aligned.byte_add(1) };

assert!(aligned.try_cast_aligned::<u32>().is_some());
assert!(unaligned.try_cast_aligned::<u32>().is_none());

I like because the pointer-creation part doesn't bloat the example (especially since NonNull::from_ref has 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).

[tgross35]

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::<u32>().is_some());
assert!(unaligned.try_cast_aligned::<u32>().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.

[tgross35]

@bors r+ rollup

[bors]

📌 Commit 204d174 has been approved by tgross35

It is now in the queue for this repository.

[RalfJung]

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

[bors]

📌 Commit 09ae053 has been approved by tgross35

It is now in the queue for this repository.