stabilize size_of_val_raw, align_of_val_raw, Layout::for_value_raw

[RalfJung]

View all comments

Stabilizes versions of size_of_val_raw and friends that can be invoked on raw pointers, which means they can be used even if one does not have a pointer to a "valid value". This was held up for a while because figuring out the exact safety requirements is tricky, but I think the ones we now have had for a while (plus the minor clarifications in this PR) are "good enough": we basically do case distinction on the unsized tail of the type, and spell out the appropriate requirement for each case. This avoids making blanket statements about future kinds of DST that we may introduce eventually.

These are the requirements I should we should stabilize:

• If T is Sized, this function is always safe to call.
• If the unsized tail of T is:
  • 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.
    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.
  • 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
    [align_of_val] on a reference to a type with an extern type tail.
  • No other kind of unsized tail currently exists. 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.

Going over the open questions from the tracking issue:

• What should the exact safety requirements of these functions be? -> the currently documented requirement look good to me.
• How should this interact with extern types? -> what we document looks good here; we can still adjust this until extern types are stabilized.
• Are the functions sound to call on invalid data pointers? -> for the unsized tails that currently exist: yes; that's kind of the only reason to use them.

Cc @rust-lang/opsem @rust-lang/lang
(FCP should probably include both teams, unless lang is okay with fully delegating this to t-opsem)
I'll nominate the tracking issue for libs-api so we can get their approval as well for how the operation is exposed.

Fixes #69835

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[rustbot]

r? @clarfonthey

rustbot has assigned @clarfonthey.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 10 candidates
• Random selection from Mark-Simulacrum, aapoalas, clarfonthey, jhpratt

[Mark-Simulacrum]

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 Layout, it makes sense to me that we can't exceed isize. However, I'm wondering if we want / will want a safe version of this function, that checks that you haven't exceeded isize. It seems like that's eminently possible since essentially the UB here is from doing an unchecked_add on the two involved sizes. I guess users will probably be able to do that themselves in the future via std::ptr::metadata (safe) and then checked_add of any sized prefix and the DynMetadata::size_of (safe). Maybe we know enough to stabilize at least some of the metadata surface area too...

View changes since the review

[RalfJung]

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

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.

I'm wondering if we want / will want a safe version of this function, that checks that you haven't exceeded isize.

Maybe we do, but I don't think that has to be part of this stabilization.
In terms of how that would impact this stabilization, I guess the point is that maybe the functions we are stabilizing here should be called size_of_val_unchecked or size_of_val_raw_unchecked then?

[Mark-Simulacrum]

Agreed on all points -- I think the impact on naming is the only thing for this stabilization.

[CAD97]

I agree that we want (to at least hold the space for) a safe option with checked overflow for MetaSized types. But since a requirement for being safe is specifically that the data pointer part doesn't matter, a part of me feels like the API for such shouldn't even be given the data part of the address, but only the metadata.

e.g. the API name in my head takes the shape Layout::with_metadata::<T>(<T as Pointee>::Metadata) -> Self.

[RalfJung]

The function name discussion should probably happen on the tracking issue as that's where libs-api will do their FCP.

[RalfJung]

This way, caller of this function doesn't need to justify and this is implied by the type.

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.

[nbdd0121]

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 .len() method to access it, and for vtables, you can make use of the vtable by performing a trait upcasting. These are API/language features that make use of these invariants already, so it's not like the invariant can go away, we can at most weaken it from validity to safety invariant.

[RalfJung]

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?

[ojeda]

It sounds like you are suggesting we shorten the entire thing to just this:

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

[RalfJung]

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.

[nikomatsakis]

I'm in favor of stabilizing these in some form but not strongly opinionated about the naming.

I don't personally think lang needs checkboxes myself though in this area (but I appreciate being cc'd). To me, it seems like it's up to T-opsem whether we feel comfortable with what is being stabilized and up to libs-api to decide the naming questions.

(Not necessarily a lang consensus position.)

[scottmcm]

👍 I'm a fan of this approach. This is a useful thing to provide for slices and dyn trait, and I agree with these rules for those tail types. Explicitly disclaiming things that aren't defined right now makes sense to me.

View changes since the review

[traviscross]

We talked about this last week. We'll let the libs-api side of this settle first, in terms of choice of name and their happiness to stabilize, then we'll lang FCP the stabilization PR here to sign off on our part of it, in the same way that we do for stabilizations of intrinsics.

[CAD97]

Should we explicitly call out that the behavior for an extern type tail is unstable and could change, or is extern type being unstable sufficient? The change I think is somewhat likely to happen is that extern type tails could become not "MetaSized" and thus be a type error to pass to both reference and pointer forms of this functionality.

[clarfonthey]

I think that from a libs perspective, once the outstanding FCP on the tracking issue is finished, this is okay to merge. Based upon Niko's comment it appears that lang is fine with this too, but I also agree that it would be good to hand this off to an opsem reviewer before merging.

r? opsem

Edit: oh, that's funny, does bors not like changing reviewers when it's waiting on FCP?
Edit 2: We've got a reviewer now, so, label added back.

[RalfJung]

We talked about this last week. We'll let the libs-api side of this settle first, in terms of choice of name and their happiness to stabilize, then we'll lang FCP the stabilization PR here to sign off on our part of it, in the same way that we do for stabilizations of intrinsics.

@traviscross I was hoping we could parallelize this (as we have done in the past, IIRC). I see no reason why t-lang could only look at this once all t-libs-api members ticked their boxes.

[RalfJung]

The 10-day FCP for libs has started in #69835 so this is unlikely to change further. So let's start the checkboxing for our side of things.

@rfcbot fcp merge lang,opsem

(Including lang based on this message which doesn't sound like lang has consensus for letting this one be opsem-only.)

[rust-rfcbot]

@RalfJung has proposed to merge this. The next step is review by the rest of the tagged team members:

• @CAD97
• @RalfJung
• @digama0
• @joshlf
• @joshtriplett
• @nikomatsakis
• @saethlin
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[CAD97]

A prior draft of these docs included a guarantee that if the pointer can be soundly converted to a reference and passed to a function (i.e. retagged) (e.g. size_of_val(&*ptr)) then size_of_val_raw(ptr) is allowed.

Is this a property that we want to provide? It's implied for all current unsized tails; this would be a statement covering future unsized tail developments. Or we could just tell people who ask for this guarantee to just use size_of_val, since they said they're allowed to create the reference.

View changes since this review

[RalfJung]

A prior draft of these docs included a guarantee that if the pointer can be soundly converted to a reference and passed to a function (i.e. retagged) (e.g. size_of_val(&*ptr)) then size_of_val_raw(ptr) is allowed.

Hm, do we not have that any more? Any idea why it got removed?

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[RalfJung]

A prior draft of these docs included a guarantee that if the pointer can be soundly converted to a reference and passed to a function (i.e. retagged) (e.g. size_of_val(&*ptr)) then size_of_val_raw(ptr) is allowed.

In fact we still have that?

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

[traviscross]

Thanks @RalfJung.

@rfcbot reviewed

[Darksonn]

Tuples with unsized tails were removed from the language in #137728.

View changes since the review

[Darksonn]

Errr ... actually forget about that. We just removed the coercion and the promise that (i32, i32) and (i32, dyn Send) have compatible layouts, but (i32, dyn Send) is still technically a type that exists in the language.

So forget about this comment.

[nikomatsakis]

@rfcbot reviewed

My take is: this is a sensible thing to stabilize. I think these are the correct safety conditions. I think the safety conditions will ultimately be dependent on the type, so the approach we have done now of saying "this is valid for exactly these sets of types but not necessarily future ?Sized types that get added" is the right one and is backwards compatible.

I expect at some point we will want a subtrait that designates "this is safe so long as metadata is valid, data pointer doesn't matter". But we don't have that yet.

(This intersects a discussion I had with @tmandry @davidtwco and @the8472 the other day.)