-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Allow calculating the layout behind a pointer #69079
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain how Layout::for_ptr
would be used? Are people supposed to create a dummy/aliasing pointer and pass it to the function? Is that really something useful? (Or just link to that previous discussion, so I can read up on it there.)
I don't think that is a good decision -- and certainly it is a decision that requires FCP at least. Given the general description of raw pointers as being geared towards unsafe code, I think having the safety invariant require more than the validity invariant is going against the expectation of most people. So I'd prefer if this function were |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
IIRC, the current proposed validity requirement for fat pointers would be enough for these functions. I only note this as a safety rather than validity requirement because the safety of these APIs only impacts the safety requirement. I'm perfectly happy to mark these APIs as I know the availability of The specific case that prompted me to actually do the legwork to see if this were possible is unsafe fn data_offset<T: ?Sized>(ptr: *const T) -> isize {
// Align the unsized value to the end of the `RcBox`.
// Because it is ?Sized, it will always be the last field in memory.
// Note: This is a detail of the current implementation of the compiler,
// and is not a guaranteed language detail. Do not rely on it outside of std.
data_offset_align(align_of_val(&*ptr))
} In this case the pointer is known to always be valid, but being able to directly query the layout of a type behind a pointer would eliminate the need to manifest a reference in this function. Getting the layout behind a pointer also makes typed dealloc easier. Just as a quick example, unsafe fn dealloc_t<T: ?Sized>(a: &mut impl AllocRef, ptr: NonNull<T>) {
a.dealloc(ptr.cast(), Layout::for_ptr(ptr.as_ptr()))
} The other primary potential usage I see for getting layout from the raw pointer rather than a reference is if the pointer is being passed around and the user wants to get the layout back without manifesting a reference, thus requiring grabbing exclusive access ( To be completely honest, I'd be fine if the decision is that this isn't needed/desired. If some code has created a pointer that has valid pointer metadata but isn't valid as a reference, they probably already know the layout (from allocation or when it was a valid reference); in that case this would only be allowing to retrieve said layout information back from the type system rather than a formal parameter. |
bd7b5c5
to
cce6ddf
Compare
I've not touched the safety of the intrinsics (yet) but I marked the exposed API as conservatively Also, potential footgun I just realized: |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
One proposal for validity requirements for vtables is that this is the same as I am perfectly on board with adding these methods for raw pointers, that has a clear use. I am just saying we should be cautious about making these methods safe. |
I know way too little about unsafe stuff to be a useful assignee for this PR. r? @RalfJung |
I agree with @RalfJung that this precise point -- if I'm understanding correctly -- has been a point of contention. I recall discussing it with @Manishearth, for example, who felt strongly that one should be able to construct |
(Just to be clear, right now NULL specifically is insta-UB because raw wide pointers use the vtable as niche for layout optimizations. But we could conceivably change that.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a UCG perspective, this LGTM.
However, this is a public (but unstable) libstd addition, so maybe @rust-lang/libs should be involved?
Also, the new libs feature needs a tracking issue.
From @RalfJung's comments it seems like the language level component of the PR was removed cause of the addition of |
|
What exactly should the safety section say for these functions? The technical requirement is that the pointer is thin, or that the fat pointer metadata is "enough" to determine size/align. And I'd argue that a safety requirement of "it is safe to do this" is worse than not having one. Of course, on top of this, with current defined Rust, it's not possible to violate this requirement. A "safe to deref" requirement is over-constraining, though it is still looser than making a real reference. "Has valid metadata" is too loose, as validity refers to the validity invariant, and the point of it being I definitely think these functions would already be useful, but I don't know how to specify their safety requirements while it's not possible to break them. |
Regardless of whether "valid" is the right term for these docs, as far as I can tell the requirements are something like:
(In 1.41.0 on x64,
But until we add something like |
90cade2
to
839d107
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (modulo the last nits), thanks for enduring!
@SimonSapin what do you think?
Let align/size_of_of_val intrinsics work on ptrs
Squashed. |
Thanks! @bors r+ |
📌 Commit dd973d1 has been approved by |
☀️ Test successful - checks-azure |
…ruppe re-add Layout::for_value_raw Tracking issue: rust-lang#69835 This was accidentally removed in rust-lang#70362 56cbf2f. Originally added in rust-lang#69079.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was some discussion around allowing this previously.
This does make the requirement for raw pointers to have valid metadata exposed as part of the std API (as a safety invariant, not validity invariant), though I think this is not strictly necessarily required as of current. cc @rust-lang/wg-unsafe-code-guidelines
Naming is hard; I picked the best "obvious" name I could come up with.
If it's agreed that this is actually a desired API surface, I'll file a tracking issue and update the attributes.