-
-
Notifications
You must be signed in to change notification settings - Fork 15.2k
stabilize size_of_val_raw, align_of_val_raw, Layout::for_value_raw #157572
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -230,24 +230,33 @@ impl Layout { | |
| /// | ||
| /// - If `T` is `Sized`, this function is always safe to call. | ||
| /// - If the unsized tail of `T` is: | ||
| /// - a [slice], then the length of the slice tail must be an initialized | ||
| /// - a [slice] `[U]` or `str`, then the length of the slice tail must be an initialized | ||
| /// integer, and the size of the *entire value* | ||
| /// (dynamic tail length + statically sized prefix) must fit in `isize`. | ||
| /// For the special case where the dynamic tail length is 0, this function | ||
| /// is safe to call. | ||
| /// - a [trait object], then the vtable part of the pointer must point | ||
| /// to a valid vtable for the type `T` acquired by an unsizing coercion, | ||
| /// and the size of the *entire value* | ||
| /// (dynamic tail length + statically sized prefix) must fit in `isize`. | ||
| /// - an (unstable) [extern type], then this function is always safe to | ||
| /// call, but may panic or otherwise return the wrong value, as the | ||
| /// extern type's layout is not known. This is the same behavior as | ||
| /// [`Layout::for_value`] on a reference to an extern type tail. | ||
| /// - otherwise, it is conservatively not allowed to call this function. | ||
| // NOTE: the reason this is safe is that if an overflow were to occur already with size 0, | ||
| // then we would stop compilation as even the "statically known" part of the type would | ||
| // already be too big (or the call may be in dead code and optimized away, but then it | ||
| // doesn't matter). | ||
| /// - a [trait object] `dyn Trait`, then the vtable part of the pointer must point | ||
| /// to a valid vtable for `Trait`, and the size | ||
| /// of the *entire value* (dynamic tail length + statically sized prefix) | ||
| /// must fit in `isize`. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We document that the first part here ("vtable part of the pointer must point to a valid vtable") is a validity invariant for raw pointers (https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html#r-undefined.validity.wide). For obtaining a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. So it is technically redundant. It still seems worth repeating? It is dangerous to rely on validity invariants are a precondition since validity invariants may become weaker in future versions of Rust.
Maybe we do, but I don't think that has to be part of this stabilization.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed on all points -- I think the impact on naming is the only thing for this stabilization.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that we want (to at least hold the space for) a safe option with checked overflow for e.g. the API name in my head takes the shape
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function name discussion should probably happen on the tracking issue as that's where libs-api will do their FCP.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If we weaken the validity requirement in the future, the caller has something to justify. Formally speaking, removing this text doesn't even change much, since talking e.g. about the integer value in the slice metadata implicitly imposes an initialization requirement. Every single time the docs talk about the value of an integer, they implicitly say "the integer is initialized and ...". Whatever code calls this has to argue why the value of the metadata satisfies the precondition, which means it has to argue that the value even exists, i.e., that it is initialized. So I can't think of any safety comments that would be simplified by this.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When a raw pointer exists in the first place, it would have implied the metadata already exists and is valid (and of course that implies initialized). For the slice case you even have the safe
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am quite concerned about implicitly relying on external invariants here. We have been bitten by this in the past and we should learn from our mistakes. At the same time apparently many people find it confusing to repeat those currently redundant external invariants. No idea how to satisfy both of these at the same time -- any ideas? It sounds like you are suggesting we shorten the entire thing to just this: /// - a [slice] `[U]`, `str`, or a [trait object] `dyn Trait`, then the size of the *entire value*
/// (dynamic tail length + statically sized prefix) must fit in `isize`.
/// For the special case where the dynamic tail length is 0, this function
/// is safe to call.
But then how do we protect against #138351 -style problems?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, that is what I meant, i.e. I was expecting safety requirements to not state conditions that are "impossible" to begin with. Otherwise, it can immediately make a reader wonder what one is missing, i.e. how that situation could happen to begin with (and, worse, wonder if one may have had bugs elsewhere due to such a misunderstanding and so on). Of course, from the point of view of an expert, it is likely obvious that it is redundant, but if one is not 100% sure that is the case, then it can be quite confusing. Regarding the risk of updating a definition and not these conditions, I guess one could attempt to "tag" the functions that implicitly need to rely on a particular invariant (with an attribute or similar), so that one can force a review of those when the definition changes. I guess it could help other analysis too, but I imagine it can be a ton of work to set up. I always wondered how upstream actually handled such changes and kept track of everything, to be honest! Having said that, the original problem above, i.e. the one for the reader, is really just to know whether it is redundant or not. Thus it may just be fine to keep them inline but at least somehow "highlight" that they are redundant -- that is why I suggested the footnote perhaps with a link to the invariant or similar. (It may also have some value as a low-tech, coarse tag to grep for, i.e. the point above, but less fancy.)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking about it some more, there is a key difference to #138351 -- as you said, we can view this as relying on the safety invariant of raw ptr metadata. That one isn't properly documented but it's also not at liberty to change -- and it implicitly is a precondition for every function that doesn't say otherwise. |
||
| /// - No other kind of unsized tail currently exists that satisfies the trait bounds for this | ||
| /// function. If more kinds of unsized tails get introduced in the future, the documentation | ||
| /// of this function will have to be extended before it can be used for such types. | ||
| /// | ||
| /// Here, *unsized tail* refers to the type obtained by recursively descending through the last | ||
| /// field of a tuple or struct until we arrived at a built-in unsized type. | ||
|
Comment on lines
+245
to
+246
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tuples with unsized tails were removed from the language in #137728.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Errr ... actually forget about that. We just removed the coercion and the promise that So forget about this comment. |
||
| /// | ||
| /// As a consequence of these rules, it is the case that whenever it is allowed to convert `val` | ||
| /// into a shared reference, then it is also allowed to invoke this function. | ||
| /// | ||
| /// [trait object]: ../../book/ch17-02-trait-objects.html | ||
| /// [extern type]: ../../unstable-book/language-features/extern-types.html | ||
| #[unstable(feature = "layout_for_ptr", issue = "69835")] | ||
| #[stable(feature = "layout_for_ptr", since = "CURRENT_RUSTC_VERSION")] | ||
| #[rustc_const_stable(feature = "layout_for_ptr", since = "CURRENT_RUSTC_VERSION")] | ||
| #[must_use] | ||
| #[inline] | ||
| pub const unsafe fn for_value_raw<T: ?Sized>(t: *const T) -> Self { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| #![feature(layout_for_ptr)] | ||
| use std::mem; | ||
|
|
||
| #[repr(packed, C)] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,3 @@ | ||
| #![feature(layout_for_ptr)] | ||
| use std::mem; | ||
|
|
||
| #[repr(packed(4))] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| #![feature(ptr_metadata, layout_for_ptr)] | ||
| #![feature(ptr_metadata)] | ||
|
|
||
| use std::{mem, ptr}; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,5 @@ | ||
| //@ run-pass | ||
|
|
||
| #![feature(layout_for_ptr)] | ||
|
|
||
| use std::{mem, ptr}; | ||
|
|
||
| struct Foo(#[allow(dead_code)] u32); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,4 @@ | ||
| //@ run-pass | ||
| #![feature(layout_for_ptr)] | ||
| use std::mem; | ||
|
|
||
| #[repr(packed(4))] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,4 @@ | ||
| //@ run-pass | ||
| #![feature(layout_for_ptr)] | ||
| use std::mem; | ||
|
|
||
| #[repr(packed, C)] | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.