rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.17k stars 12.7k forks source link

Tracking Issue for [*const T|*mut T]::with_metadata_of #75091

Open oliver-giersch opened 4 years ago

oliver-giersch commented 4 years ago

Feature gate: #![feature(set_ptr_value)]

Public API

impl<T: ?Sized> *mut T {
    pub fn with_metadata_of<U: ?Sized>(self, val: *const U) -> *mut U;
}

impl<T: ?Sized> *const T {
    pub fn with_metadata_of<U: ?Sized>(self, val: *const U) -> *const U;
}

Steps / History

Unresolved Questions

SimonSapin commented 3 years ago

As discussed on Zulip this could be replaced with ptr::from_raw_parts(val, ptr::metadata(self)), tracked in https://github.com/rust-lang/rust/issues/81513

ChayimFriedman2 commented 2 years ago

Even if we want to keep these methods I think it's better to write them in terms of the metadata API as it's easy to reason about and less error prone than the current implementation.

WaffleLapkin commented 12 months ago

This needs a better motivational example. From current docs:

This function is primarily useful for allowing byte-wise pointer arithmetic on potentially fat pointers:

#![feature(set_ptr_value)]
use core::fmt::Debug;
let mut arr: [i32; 3] = [1, 2, 3];
let mut ptr = arr.as_mut_ptr() as *mut dyn Debug;
let thin = ptr as *mut u8;
unsafe {
    ptr = thin.add(8).with_metadata_of(ptr);
    assert_eq!(*(ptr as *mut i32), 3);
    println!("{:?}", &*ptr); // will print "3"
}

This is superceeded by byte_add and co (https://github.com/rust-lang/rust/issues/96283)

dtolnay commented 12 months ago

dyn-clone perhaps? https://github.com/dtolnay/dyn-clone/issues/8

HeroicKatora commented 11 months ago

flexible-io is the implementation of the solution devised a while ago to the problem and another instance which originally got me invested in this PR. It relies on with_metadata_of. The full metadata API might be an optimization for later, but is way more complex than necessary here.

kotakotik22 commented 6 months ago

here's my stable workaround, though Miri doesn't let it pass..

fn inherit_metadata<T : ?Sized>(from: *mut T, onto: *mut u8) -> *mut T {
    #[cfg(miri)]
    {
        panic!("inherit_metadata does not work in Miri")
    }

    unsafe {
        from.byte_offset(
            onto.byte_offset_from(from)
        )
    }
}

these functions would be a really nice utility until the main pointer metadata feature is stabilized (and it'll probably be a while..) they provide a workaround to problems with implementing structures supporting ?Sized that don't need the typed complexity of the metadata api

bjorn3 commented 6 months ago

That will use the provenance of from rather than the provenance of onto, which will cause UB if you dereference the resulting pointer, but the provenance of from doesn't allow accessing this address. Miri is entirely correct in rejecting this. With rustc it may work by accident or you may get a miscompilation.

GnomedDev commented 1 month ago

For the "Is this even needed/useful given that there now also is https://github.com/rust-lang/rust/issues/81513?", I feel like this is super useful even with or without the metadata APIs.

I'd love to see this stabilized, if that is a good resolution to the last question needed answering.

Noratrieb commented 1 month ago

@rust-lang/libs-api see above

scottmcm commented 1 month ago

Hmm, I think the primary alternative to this would be something like https://github.com/rust-lang/libs-team/issues/246 -- instead of copying the metadata from a full other pointer, just remove the blocker to letting you pass around the metadata directly.

It's not obvious to me that people really ever want this version where you need to pass a full pointer, just to ignore its address. Or is there some situation I'm not thinking of where that's really better than doing a let (p, m) = pointer.parts(); then p.whatever().with_metadata(m)?

GnomedDev commented 1 month ago

As I said above @scottmcm:

My motivating example is https://github.com/andylokandy/smallbox which has been assuming fat pointer layout for years and I recently added a nightly feature which uses this API and strict provenance. Since strict provenance is in the process of being stabilized, this being stabilized as well would be amazing for smallbox.

HeroicKatora commented 4 weeks ago

The ACP is not about stabilizing <T as Pointee>::Metadata but rather only a wrapper. @scottmcm Would it be possible to reflect that in the Unresolved questions of the tracking issue https://github.com/rust-lang/rust/issues/75091#issue-671868145 that only point to the Pointee::Metadata trait.

That is, a way to represent *const T without an address, for in cases where that address is redundant as here. The value for library authors in having the method proposed here is for storing / dealing with a the unsized pointer whose address is ignored anyways since the provenance is also lost when passed as argument. The proposed struct thus reflects the mechanism quite well. If we had an adjusted type such as that then the signature here might as well be:

I still think that the with_metadata_of method is more intuitive in usage than any from_raw_parts design, as explained in the PR that changed it originally. However, https://github.com/rust-lang/libs-team/issues/246 's API sketch could be reworked into a similar design. There's no need to do as much with free methods as in the sketch. For instance such a compromise could look like:

pub struct Metadata<T: ?Sized>(<T as Pointee>::Metadata);

impl<T: ?Sized> Metadata<T> {
    /// Not taking up space of `new() where T: Sized`.
    pub fn for_ptr(ptr: *const T) -> Self;
}

// impl for Metadata<T>: Copy, Eq, Ord, Hash

impl<T: ?Sized> *const T {
    pub fn metadata(self) -> Metadata<T>;
    pub fn with_metadata_of<U: ?Sized>(self, other: Metadata<U>) -> *const U;
}

impl<T: ?Sized> *mut T {
    pub fn metadata(self) -> Metadata<T>;
    pub fn with_metadata_of<U: ?Sized>(self, other: Metadata<U>) -> *mut U;
}

I've got a bunch of lines of reasoning I pondered going in the other direction but ultimately decided they wouldn't be persuasive: In terms of simple vs. complex there is prior art: `mem::size_of::()` vs `Layout::new::().size()`. It also happens that the semantics of the composite are simpler than the semantics of its parts. This also reduce the number of feature interactions that might complicate any design phases. (Note: size was const-stable before `Layout` was stabilized and then it took 24 versions to transfer that). Weak because: I don't think time should be too influential in the design of such a smallish feature. And also the alternative is _almost_ as simple. Answering the question of whether usability, we can look at `rustc`'s usage. I don't think it matters greatly to library authors and usage whether metadata would be stored on its own or in a fat-ptr with null address. - `rc`/`arc` instantly use the metadata pointer after an unsizing cast. That's also addressed by the alternative: "It's not practical to provide unsizing support directly for ::Metadata, but it can be added relatively straightforwardly for a proper Metadata struct". Slightly more verbose but not really more complex: - `mem.with_metadata_of((ptr as *const ArcInner).metadata())` (in case no unsizing cast and no method is provided) - `mem.with_metadata_of(ptr.metadata() as Metadata>)` - `mem.with_metadata_of(ptr.metadata().cast>())` - The use in pointer types such as `byte_offset` is a little odd. They do `__modify_address(self).with_metadata_of(self)`. It shouldn't be problematic to split this into two lines or adding the metadata constructor but it would be a little noisy. Alas it's not entirely clear if they should then be written with `from_raw_parts` or not. (In particular `mask` which calls an intrinsic already operating on `()`-pointers).
RalfJung commented 4 weeks ago

@HeroicKatora IMO it makes little sense to have a function with_metadata_of<U: ?Sized>(self, other: Metadata<U>) -> *mut U -- that should be called with_metadata! The of part refers to "with the metadata of another pointer"; if we are directly providing the metadata, we should just say "with this metadata", and there's no "of" involved any more.

This tracking issue is specifically about copying the metadata from one pointer to another. Your proposal would be part of https://github.com/rust-lang/rust/issues/81513.

HeroicKatora commented 4 weeks ago

I understood @scottmcm as a new question, separate from #81513 itself, to explore whether https://github.com/rust-lang/libs-team/issues/246 changes something about the unresolved redundancy question in the issue. It is not accepted, so not yet a part of #81513 to my understanding (and not linked there either). Maybe I'm misreading the process here.

Neither the API of this (#75091) nor the sketch based on https://github.com/rust-lang/libs-team/issues/246 would require stabilization of the Metadata trait itself. I think this affects the merit of the simplicity argument. The representation without address is superior for some library use cases. (It would definitely occur in practice, infact I'm sure my libraries would always prefer the address-less value). I concur, those methods should be named with_metadata, not with_metadata_of, so there's no necessary overlap in the interface and it would of course be possible to have both sets of methods.

Are you suggesting the redundancy even with the ACP is slight enough that it would still not be problematic, and there's enough merit to consuming a pointer directly? Definitely a reasonable position to take. I just intended for it to be clarified that the ACP presents a (middle ground) alternative not yet explored in comments in this thread. If libs is in favor of potentially having both that's fine 👍

RalfJung commented 4 weeks ago

All I am saying is, we should not have functions called with_metadata_of that take a pointer and a metadata. That name should only be used for functions that take two pointers.

This issue here is specifically about the two-pointer version of the APIs, which avoids stabilizing any new type. with_metadata in the form you suggested IMO makes a lot of sense (it nicely matches with_addr as well), but such an API should be proposed in https://github.com/rust-lang/rust/issues/81513 since it clearly requires talking about "a metadata value matching this pointer type". Whether that is expressed as Metadata<T> or <T as Pointee>::Metadata is a completely orthogonal question.

IOW, https://github.com/rust-lang/libs-team/issues/246 is a modification of https://github.com/rust-lang/rust/issues/81513, but has absolutely no bearing on this issue.