-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Tracking Issue for pointer metadata APIs #81513
Comments
After experimenting with custom implementations of The pre-allocator representation of
The post-allocator representation is very similar:
Both automatically implements If one wants to make Box generic over its storage, then the representation becomes:
If
Hence, in the case of inline storage, In order to have
And of course, Box being coercible is very much desirable. As a result, I believe a slight change of course is necessary:
I would note that having a single Addendum: What's all that jazz about inline storage? At a high-level,
Having the memory inline in the A motivating example is therefore |
IMO this is different from the case of |
No, that's the whole point of it actually. In C++, you have This is exactly the same principle:
So, here, If you stored a single pointer (+v-table), well, you're paying a bit too much, but that's the price for flexibility. It's up to you size it appropriately for the largest variant. In any case, unsized locals is strictly unnecessary, as can be seen in the tests of storage-poc's |
Oh I see, so this is more like Back to pointer metadata though, I have a bit of a hard time following the |
It's up to you, you can have either a purely inline storage, or you can have "small" inline storage with heap fallback. The main point is that the "inline" portion is always of fixed size and alignment (up to the storage) and therefore (You can an equivalent of
I don't think so, given the language from the documentation of
It appears that |
Implement RFC 2580: Pointer metadata & VTable RFC: rust-lang/rfcs#2580 ~~Before merging this PR:~~ * [x] Wait for the end of the RFC’s [FCP to merge](rust-lang/rfcs#2580 (comment)). * [x] Open a tracking issue: rust-lang#81513 * [x] Update `#[unstable]` attributes in the PR with the tracking issue number ---- This PR extends the language with a new lang item for the `Pointee` trait which is special-cased in trait resolution to implement it for all types. Even in generic contexts, parameters can be assumed to implement it without a corresponding bound. For this I mostly imitated what the compiler was already doing for the `DiscriminantKind` trait. I’m very unfamiliar with compiler internals, so careful review is appreciated. This PR also extends the standard library with new unstable APIs in `core::ptr` and `std::ptr`: ```rust pub trait Pointee { /// One of `()`, `usize`, or `DynMetadata<dyn SomeTrait>` type Metadata: Copy + Send + Sync + Ord + Hash + Unpin; } pub trait Thin = Pointee<Metadata = ()>; pub const fn metadata<T: ?Sized>(ptr: *const T) -> <T as Pointee>::Metadata {} pub const fn from_raw_parts<T: ?Sized>(*const (), <T as Pointee>::Metadata) -> *const T {} pub const fn from_raw_parts_mut<T: ?Sized>(*mut (),<T as Pointee>::Metadata) -> *mut T {} impl<T: ?Sized> NonNull<T> { pub const fn from_raw_parts(NonNull<()>, <T as Pointee>::Metadata) -> NonNull<T> {} /// Convenience for `(ptr.cast(), metadata(ptr))` pub const fn to_raw_parts(self) -> (NonNull<()>, <T as Pointee>::Metadata) {} } impl<T: ?Sized> *const T { pub const fn to_raw_parts(self) -> (*const (), <T as Pointee>::Metadata) {} } impl<T: ?Sized> *mut T { pub const fn to_raw_parts(self) -> (*mut (), <T as Pointee>::Metadata) {} } /// `<dyn SomeTrait as Pointee>::Metadata == DynMetadata<dyn SomeTrait>` pub struct DynMetadata<Dyn: ?Sized> { // Private pointer to vtable } impl<Dyn: ?Sized> DynMetadata<Dyn> { pub fn size_of(self) -> usize {} pub fn align_of(self) -> usize {} pub fn layout(self) -> crate::alloc::Layout {} } unsafe impl<Dyn: ?Sized> Send for DynMetadata<Dyn> {} unsafe impl<Dyn: ?Sized> Sync for DynMetadata<Dyn> {} impl<Dyn: ?Sized> Debug for DynMetadata<Dyn> {} impl<Dyn: ?Sized> Unpin for DynMetadata<Dyn> {} impl<Dyn: ?Sized> Copy for DynMetadata<Dyn> {} impl<Dyn: ?Sized> Clone for DynMetadata<Dyn> {} impl<Dyn: ?Sized> Eq for DynMetadata<Dyn> {} impl<Dyn: ?Sized> PartialEq for DynMetadata<Dyn> {} impl<Dyn: ?Sized> Ord for DynMetadata<Dyn> {} impl<Dyn: ?Sized> PartialOrd for DynMetadata<Dyn> {} impl<Dyn: ?Sized> Hash for DynMetadata<Dyn> {} ``` API differences from the RFC, in areas noted as unresolved questions in the RFC: * Module-level functions instead of associated `from_raw_parts` functions on `*const T` and `*mut T`, following the precedent of `null`, `slice_from_raw_parts`, etc. * Added `to_raw_parts`
Note how a However, as there's quite a few ways of manipulating slice pointers without |
Yes that’s the idea behind this RFC: generalize |
So a thing that seems to be missing here is a stable layout for A really annoying thing is that currently you cannot opaquely pass trait objects across FFI without doing a second allocation, because |
Would it make sense to add conversions between |
Yes, it would. It would be annoying to use, but it would suffice. |
Doesn't even need to be a pointer type, jsut an opaque type with a well-defined layout. Though pointers makes it easier for other tools to use, definitely. |
We could document that |
That would be nice, and it would be nice if it had explicit conversion functions to |
A |
That works too yeah |
It was brought to my attention that this feature has some very subtle interaction with unsafe code. Specifically, the following function is currently sound in the sense that safe code cannot cause any UB even when it calls this function: pub fn make_weird_raw_ptr() -> *const dyn Send {
unsafe { std::mem::transmute((0x100usize, 0x100usize)) }
} This RFC is a breaking change in that it makes the above function unsound: let ptr = make_weird_raw_ptr();
let meta = metadata(ptr);
let size = meta.size(); // *oops* UB At the very least, this should be listed as an open concern to be resolved. Maybe |
Don't size and align already have to be in the same location? Certainly Miri assumes this in its implementations of For |
Re: operate on references, `Weak<T>` needs to be able to get a layout for a dropped value to deallocate memory. So what we probably want is a `MaybeDropped<T>` wrapper to represent a type whose value may have been dropped, and have `metadata` operate on it.
`MaybeDropped` can be implemented as a wrapper around `ManuallyDropped` that prohibits (safe) access to the inner value.
…On March 13, 2021 6:01:58 AM EST, Ralf Jung ***@***.***> wrote:
It was [brought to my attention](rust-lang/unsafe-code-guidelines#166 (comment)) that this feature has some very subtle interaction with unsafe code. Specifically, the following function is currently *sound* in the sense that safe code cannot cause any UB even when it calls this function:
```rust
pub fn make_weird_raw_ptr() -> *const dyn Send {
unsafe { std::mem::transmute((1usize, 1usize)) }
}
```
This RFC is a breaking change in that it makes the above function *unsound*:
```rust
let ptr = make_weird_raw_ptr();
let meta = metadata(ptr);
let size = meta.size(); // *oops* UB
```
At the very least, this should be listed as an open concern to be resolved.
Maybe `metadata` should only be safe on references, not raw pointers?
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#81513 (comment)
|
@RalfJung Yes, this is an important point. It came up in RFC discussions but I forgot to incorporate it in unresolved questions then. I’ve done so in the issue description here. My understanding was that
I’m also skeptical that it could be any other way, this point is mostly copied from a comment on the RFC
I think that would be a serious limitation. I don’t see a reason extracting the components of any raw pointer and putting them back together shouldn’t be sound. However if we end up deciding that raw trait object pointers shouldn’t have any validity invariant for their vtable pointer then |
Thanks. Notice however that for this RFC to work, it is not required to make "valid vtable" part of the validity invariant. Making it part of the safety invariant would be sufficient, since this is a library-level concern.
Indeed, neither the validity invariant nor the safety invariant of raw pointers are pinned down exactly. I think it is safe to say though that people would expect these invaraints to be as weak as is at all possible, i.e., to require as little as possible. Even the fact that the vtable pointer is non-NULL is tripping people up and we might want to change that.
That would be another option, yes. (There could also be |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Is there a reason why these functions are not |
@Rua most functions described in the tracking issue are |
Hmm strange, they are shown as |
Maybe it's just because the UI is a bit... confusing. And like https://doc.rust-lang.org/std/ptr/fn.copy.html, the function signature will not show |
Created #125511 to track that rustdoc bug, so it doesn't pollute this issue any longer. |
Just filed an API question related to this: #125517 |
Has there been any progress on stabilizing this feature over the last year, perhaps in the depths of zulip? |
So...I'm dropping a line here because I've realized that I would like to leverage this trait as a better name for |
I think #125517 needs to be looked at before anything is stabilised here. Either that, or stabilise only for |
@nikomatsakis good point and I strongly suspect this also ought to have a way to implement custom |
Note that the naming of the APIs here would be affected by the ongoing proposed FCP in: |
Over here, @HeroicKatora suggested some functions that would belong in this tracking issue. In slightly adjusted form, those would be: impl<T: ?Sized> *const T {
pub fn metadata(self) -> <T as Pointee>::Metadata;
pub fn with_metadata<U: ?Sized>(self, other: <U as Pointee>::Metadata) -> *const U;
}
impl<T: ?Sized> *mut T {
pub fn metadata(self) -> <T as Pointee>::Metadata;
pub fn with_metadata<U: ?Sized>(self, other: <U as Pointee>::Metadata) -> *mut U;
} Also of note is rust-lang/libs-team#246 which would replace all |
Also, this comment in the tracking issue is not quite accurate:
By now it is clear that the safety invariant of a dyn trait raw pointer requires vtable validity. So |
In fact, #![feature(ptr_metadata)]
struct S<T: ?Sized> {
x: i32,
y: T,
}
fn main() {
let x = S { x: 0, y: 0 };
let xref: &S<dyn Send> = &x;
dbg!(std::ptr::metadata(xref).size_of()); // 4
dbg!(std::mem::size_of_val(xref)); // 8
}
At the very least, this needs to be more clearly documented. But I think ideally we'd not provide a |
TL;DR: I'd like to provide some (not-so-prior) prior art and use cases. I hope that this motivates that it'd be very useful to stabilize this feature, but also that some changes may be in order (at least to support the use cases described here). I'm going to be basing these examples on zerocopy 0.8.5, which is at this Git commit. In zerocopy, we have the Just like Here's the full source code of pub unsafe trait KnownLayout {
/// The type of metadata stored in a pointer to `Self`.
///
/// This is `()` for sized types and `usize` for slice DSTs.
type PointerMetadata: PointerMetadata;
/// The layout of `Self`.
#[doc(hidden)]
const LAYOUT: DstLayout;
#[doc(hidden)]
fn raw_from_ptr_len(bytes: NonNull<u8>, meta: Self::PointerMetadata) -> NonNull<Self>;
/// Extracts the metadata from a pointer to `Self`.
fn pointer_to_metadata(ptr: *mut Self) -> Self::PointerMetadata;
/// Computes the length of the byte range addressed by `ptr`.
///
/// Returns `None` if the resulting length would not fit in an `usize`.
#[doc(hidden)]
fn size_of_val_raw(ptr: NonNull<Self>) -> Option<usize> {
let meta = Self::pointer_to_metadata(ptr.as_ptr());
// SAFETY: `size_for_metadata` promises to only return `None` if the
// resulting size would not fit in a `usize`.
meta.size_for_metadata(Self::LAYOUT)
}
}
/// The metadata associated with a [`KnownLayout`] type.
#[doc(hidden)]
pub trait PointerMetadata: Copy + Eq + Debug {
/// Constructs a `Self` from an element count.
///
/// If `Self = ()`, this returns `()`. If `Self = usize`, this returns
/// `elems`. No other types are currently supported.
fn from_elem_count(elems: usize) -> Self;
/// Computes the size of the object with the given layout and pointer
/// metadata.
///
/// # Safety
///
/// `size_for_metadata` promises to only return `None` if the resulting size
/// would not fit in a `usize`.
fn size_for_metadata(&self, layout: DstLayout) -> Option<usize>;
} Mostly,
However, there are some items which would be useful to add to
pub struct DstLayout {
align: NonZeroUsize,
size_info: SizeInfo,
}
enum SizeInfo {
Sized { size: usize },
SliceDst(TrailingSliceLayout),
}
struct TrailingSliceLayout {
// The offset of the first byte of the trailing slice field. Note that this
// is NOT the same as the minimum size of the type. For example, consider
// the following type:
//
// struct Foo {
// a: u16,
// b: u8,
// c: [u8],
// }
//
// In `Foo`, `c` is at byte offset 3. When `c.len() == 0`, `c` is followed
// by a padding byte.
offset: usize,
// The size of the element type of the trailing slice field.
elem_size: usize,
} This contains two pieces of information that
These are needed in order to implement In order for zerocopy to replace EDIT: It may also be useful to support slicing slice DSTs. |
This is a tracking issue for the RFC 2580 "Pointer metadata & VTable" (rust-lang/rfcs#2580).
The feature gate for the issue is
#![feature(ptr_metadata)]
.About tracking issues
Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.
Steps
instructions?)
Unresolved Questions
Language-level:
DynMetadata
methods likesize
may need to beunsafe fn
. Or maybe something like*const ()
should be metadata of trait objects instead ofDynMetadata
.Right now, there is some inconsistency here:
size_of_val_raw(ptr)
is unsafe, butmetadta(ptr).size_of()
does the same thing and is safe.Update (2024-10-04): It is definitely the case that the safety invariant for raw trait objects requires a valid vtable. So
metadta(ptr).size_of()
being safe is fine.size_of_val_raw(ptr)
must be unsafe because of slices, so there is no inconsistency here.Metadata
be required to beFreeze
API level:
*const ()
appropriate for the data component of pointers? Or should it be*const u8
? Or*const Opaque
with some newOpaque
type? (Respectively*mut ()
andNonNull<()>
)ptr::from_raw_parts
and friends beunsafe fn
?Thin
be added as a supertrait ofSized
? Or could it ever make sense to have fat pointers to statically-sized types?DynMetadata
not have a type parameter? This might reduce monomorphization cost, but would force that the size, alignment, and destruction pointers be in the same location (offset) for every vtable. But keeping them in the same location is probaly desirable anyway to keep code size small.Pointee
trait with aptr::Metadata
type libs-team#246DynMetadata::size_of
does not always return the same value assize_of_val
since the former only reads the size from the vtable, but the latter computes the size of the entire type. That seems like a pretty bad footgun?API bikesheds:
Pointee
(v.s. Referent?),Thin
(ThinPointee
?),DynMetadata
(VTablePtr
?), etccore::ptr
. For example: shouldThin
be incore::marker
instead?Implementation history
ptr::from_raw_parts
#125701NonNull::from_raw_parts
per ACP362 #132895Tracked APIs
Last updated for #81172.
The text was updated successfully, but these errors were encountered: