Stacked Borrows: How precise should UnsafeCell be tracked?

[RalfJung]

Currently, when looking for UnsafeCell behind shared references, Miri descends through arrays, structs and the like, but does not descend through enums. Instead, when it sees an enum, it checks if T: Freeze, and if not, treats the entire enum as an UnsafeCell.

The benefit of this is that finding UnsafeCell does not require reading from memory (rust-lang/miri#931), which makes formal reasoning about Stacked Borrows a lot simpler. Accessing memory during UnsafeCell search opens all sorts of nasty questions, such as whether those accesses are themselves subject to Stacked Borrows rules or not (and if yes, which tag they use). Not reading enum discriminants also avoids potential confusion from Stacked Borrows partially checking the validity invariant of the referenced data.

On the other hand, being more precise with UnsafeCell search could help optimizations. When a function works on &Result<Cell<i32>, i32>, and the compiler somehow can know that the Err variant is active, we would be able to rely on this shared reference being read-only -- currently, that is not an assumption that the compiler can make. (But note that once a shared reference gets created to the i32 in the Err variant, that is already guaranteed to be read-only.)

This is somewhat related to #204.

Some posts with useful datapoints (not exhaustive):

• Stacked Borrows: How precise should UnsafeCell be tracked? #236 (comment)
• Stacked Borrows: How precise should UnsafeCell be tracked? #236 (comment)

Other parts of this question:

• If we don't have strict subobject provenance, then what is the mutability of bytes outside the range of T?
• Does UnsafeCell inside PhantomData have any effect?

[RalfJung]

To be precise, here is currently how we compute the permissions associated with a shared reference &T: we do recursive descend over T.

• If we hit an UnsafeCell, the memory it covers is writable.
• If we hit a field of type U such that U: Freeze, we stop searching for UnsafeCell here. This is purely an optimization, we could equivalently continue our descend, but we know that U: Freeze guarantees the absence of UnsafeCell.
• If we hit a union or a type where variants is Variants::Multiple, and since we already checked Freeze, we treat memory this type covers as writable.
• For other types (Variants::Single), we descend recursively through its fields.

Every location not marked as writable in the above procedure is marked as read-only.

[chorman0773]

For reasons similar to those brought up in #77, #84, and #253, I think the multiple variants case should consider the active variant, when determining runtime writability.
Consider the type Result<Cell<i32>,i32>, brought up in the original example. Under the model presented by lccc, in an object of that type, reguardless of the variant, there is a subobject of type i32. However, that subobject has the mutable characteristic (used to implement UnsafeCell) only when the result is Ok. Behind a shared reference, it should be reasonable to conclude that the value of this subobject does not change if you have the Err variant. This allows the movement/removal of reads behind a shared reference of this type when the variant is known to be Err (without loss of generality).

I cannot think of a particular example where this distinction would have an effect (one would probably use an external UnsafeCell, which does fun things anyways, and I've pretty much given up trying to optimize things inside an UnsafeCell, and focus my efforts on things outside of one).

[RalfJung]

FWIW, making UnsafeCell "maximally imprecise" would also help with issues like rust-lang/rust#98498, where currently the obvious fix is not sound because one of the fields of Scope is not an UnsafeCell.

[RalfJung]

In fact, even just making each field be in an UnsafeCell is not always sufficient to allow deallocating that reference, since the padding between the fields is not in an UnsafeCell.

So making use of the exception that is being added in rust-lang/rust#98017 is rather fragile currently, but would fairly very well if we don't attempt to track where inside a type the UnsafeCell are.

[CAD97]

There is still a possible axiomatic alternative: treat padding even more weakly. All fields being UnsafeCell can be sufficient to say the whole struct is in UnsafeCell if you explicitly don't care about padding. Treating the answer to “is this padding byte in an UnsafeCell” as undefined isn't strong enough; that could resolve to always say no and be the same as today; I think you'd need to somehow say the answer is always yes but still somehow justify Freeze from lack of non-padding UnsafeCell.

I don't know exactly what this would look like, and it would probably break some important properties of Stacked Borrows — the simplest interpretation is probably to have a “padding” permission tag on the borrow stack for a byte reborrowed as padding, and then reading through a padding permission both returns an uninitialized byte and stores uninit. (Making a read perform a write is the certainly broken bit.)

Ultimately I'm also in favor of the weakening applied by UnsafeCell (and by analogy, PhantomPinned) applying to the entire pointee if the pointee type contains any UnsafeCell. As chorman has pointed out, this greatly limits the reasoning the compiler¹ can make about generic code, but giving that up this seems justified² by the operational specification simplifications gained.

What might be beneficial under such a model is splitting the UnsafeCell weakening into parts, so that you can remove the dereferencable_always(n) and noalias qualities individually.

Footnotes

1. But I want to point out that code authors can still make these assumptions in a generic environment due to privacy, encapsulation, and generally the concept of soundness of an API not considering outside uses of unsafe that break such. ↩

2. Especially since for better or for worse Rust already relies heavily on monomorphizations for performance. ↩

[RalfJung]

There is still a possible axiomatic alternative: treat padding even more weakly. All fields being UnsafeCell can be sufficient to say the whole struct is in UnsafeCell if you explicitly don't care about padding.

We could do something operational where if all fields of a struct are entirely UnsafeCell, then its padding is considered UnsafeCell, too.

What might be beneficial under such a model is splitting the UnsafeCell weakening into parts, so that you can remove the dereferencable_always(n) and noalias qualities individually.

Basically, separating its effect on protectors and permissions? Sure, that would be easy to represent.

[chorman0773]

As chorman has pointed out, this greatly limits the reasoning the compiler can make about generic code, but giving that up this seems justified

I actually think there might be a more insidious problem, if this includes the fact that you can get a &mut T to those fields (while the exterior can still be freely moved as a shared reference). If it's just mutability, then the loss in generic contexts actually affects non-generic contexts as well, since non-trivial validity attributes are salient parts of pointer types in XIR (if they weren't the same problem I mentioned before is trivial to recreate, since it basically involves smuggling a unique pointer into a function as a trivially-valid pointer type, which doesn't cause implicit derives, and thus allows you to alias it to another unique pointer passed into the same function, since the raw pointer in the second parameter is its direct parent).

We could do something operational where if all fields of a struct are entirely UnsafeCell, then its padding is considered UnsafeCell, too.

I'd be even fine with a more expansive restriction: Any contiguous extent of padding bytes that preceeds or follows an UnsafeCell gets SRW as well. Unlike actual fields, XIRs model doesn't let you just point to padding bytes the same way as a subobject (I mean, you can have a pointer to it, but not a pointer to it), which is where the problem would arrise.

[RalfJung]

If it's just mutability, then the loss in generic contexts actually affects non-generic contexts as well,

I don't see why that has to be the case, or why it would be the case in MIR+LLVM.

I'd be even fine with a more expansive restriction: Any contiguous extent of padding bytes that preceeds or follows an UnsafeCell gets SRW as well. Unlike actual fields, XIRs model doesn't let you just point to padding bytes the same way as a subobject (I mean, you can have a pointer to it, but not a pointer to it), which is where the problem would arrise.

Even precisely defining the notion of "padding byte" is super hard (and subtle), on top of the already subtle rules for how far we traverse looking for UnsafeCell, on top of the already super subtle aliasing rules... I think this is way overspending our complexity budget.

[chorman0773]

On Tue, Jun 28, 2022 at 09:52 Ralf Jung ***@***.***> wrote: If it's just mutability, then the loss in generic contexts actually affects non-generic contexts as well, I don't see why that has to be the case, or why it would be the case in MIR.

In XIR, pointer types consistent of a number of validity attributes and some definition attributes (pointer kind and syntax declaration kinds), and pointee type. Different attributes forms different pointer types that are incompatible with each other. An explicit conversion (called the derive operation) is necessary. This is because an implicit conversion is otherwise needed (else you can get a *T that is the same pointer as a *unique T, and which allows both to be passed to the same function) and I don't like doing non-trivial operations implicitly.

I'd be even fine with a more expansive restriction: Any contiguous extent of padding bytes that preceeds or follows an UnsafeCell gets SRW as well. Unlike actual fields, XIRs model doesn't let you just point to padding bytes the same way as a subobject (I mean, you can have a pointer to it, but not a pointer to it), which is where the problem would arrise. Even precisely defining the notion of "padding byte" is super hard (and subtle), on top of the already subtle rules for how far we traverse looking for UnsafeCell, on top of the already super subtle aliasing rules... I think this is way overspending our complexity budget.

Fair, though it encompasses the previous proposal. Since searching for Freeze itself is structural on a type, though, couldn't it just use the implicit Pad fields?

…

— Reply to this email directly, view it on GitHub <#236 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGLD26CJRDU24DNQR672N3VRL7RTANCNFSM4N4GIRZQ> . You are receiving this because you commented.Message ID: ***@***.***>

[RalfJung]

In XIR,

Can you explain this without going into the specifics of XIR? As in, is there actually something fundamental here, or just a design quirk of XIR?

Fair, though it encompasses the previous proposal. Since searching for
Freeze itself is structural on a type, though, couldn't it just use the
implicit Pad fields?

There are no implicit Pad fields. Padding is the absence of a field at a given offset into a struct. (And something more complicated for enums.) At least that's how I prefer to think of it.

[chorman0773]

Can you explain this without going into the specifics of XIR? As in, is there actually something fundamental here, or just a design quirk of XIR?

It's fundamental, part of the type system. Especially for non-trivial validity, it changes how the pointers interact with certain operations, like being moved (particularily into a function, local, or a static, the same way that &mut T and &T do. And there are no implicit conversions in XIR, only weak ones with the convert instruction, especially when the conversion is as non-trivial as an implicit derive operation (which can best be described as being equivalent to a generalization of the reborrow operation - although as an instruction, derive is "Change pointer attributes and produce new pointer value").

There are no implicit Pad fields. Padding is the absence of a field at a given offset into a struct

Yeah, that's how I'd think of it as well. Parts of the object-representation that don't participate in determining the value.

[RalfJung]

I don't mean fundamental to XIR. I mean fundamental in a way that does not depend on the details of the language this is formalized in.

[RalfJung]

Context: I am mostly interested in XIR insofar as it teaches us general lessons that we might miss by taking a MIR/LLVM view, due to the quirks of those languages. I am much less interested in XIR itself. So if there is a lesson here that transcends XIR, I would like to learn it.

[chorman0773]

I don't mean fundamental to XIR. I mean fundamental in a way that does not depend on the details of the language this is formalized in.

Fair, it's not really fundmental in that way, It comes arround from the design of XIR and just not wanting to do highly complicated operations implicitly. I think you could argue that any time you've got a generic IR that doesn't have high-level source language queries, you'd have a similar problem though. Having it be part of the type system isn't but is that not what the type system is for, encoding information about the constraints on values?

[CAD97]

not wanting to do highly complicated operations implicitly.

What I don't get is why you aren't willing to / think you can't handle making the operations explicit as part of lowering to XIR. This kind of elaboration is an intrinsic part of lowering to any IR.

I think you could argue that any time you've got a generic IR that doesn't have high-level source language queries, you'd have a similar problem though.

This is in fact a pessimisation of what optimizations can be done on a generic IR (e.g. MIR, XIR), but it's not like this is a choice we're making without knowing its the case; we'd love to have more MIR opts in rustc as well.

What I think you/XIR needs and doesn't have at the moment is to be able to apply the pointer (restricting) attributes to the types, with the semantics that this makes any pointer access to that type act as if it had the attribute.

And if you're throwing away the required type information before the point where it would be required to provide/take advantage of pointer attributes on the pointee, then I think you'll just need to accept that you won't be able to optimize Rust code as well as otherwise.

(An option to preserve doing the optimizations in generic paths relying on the lost attributes would be to assume a monomorphization adding the "usual" attributes and preprovide a partial morphization as such.)

[RalfJung]

Having it be part of the type system isn't but is that not what the type system is for, encoding information about the constraints on values?

Yes, but then the question is what is the type system that actually encodes this properly. :)

My proposal is to view interior mutability as a property of the reference, i.e., we have two kinds of shared references: interior mutable ones and regular frozen ones. The only purpose of UnsafeCell is to automatically infer which kind of shared reference to use. Once that inference is done, you can entirely forget about UnsafeCell.

Of course actually erasing it can only be done after monomorphization, so a compiler working on polymorphic IR has to consider that each &T is actually a &{frozen: bool} T, where frozen == true iff T: Freeze.

[RalfJung]

FWIW, making UnsafeCell "maximally imprecise" would also help with issues like rust-lang/rust#98498, where currently the rust-lang/rust#98498 (comment) is not sound because one of the fields of Scope is not an UnsafeCell.

OTOH, already the current status of UnsafeCell "infecting" surrounding enums is a problem for some possible approaches to ensuring that match in a UB-free program always behaves the way one might think it would.

[chorman0773]

On Wed, 29 Jun 2022 at 19:12, Christopher Durham ***@***.***> wrote: not wanting to do highly complicated operations implicitly. What I don't get is why you aren't willing to / think you can't handle making the operations explicit as part of lowering to XIR. This kind of elaboration is an intrinsic part of lowering to *any* IR.

The issue here is something like function pointers. `fn<T>(&T)` instantiated with `i32` has to be compatible with `fn(&i32)`. fn(&i32) currently becomes `function(*readonly dereferenceable(4) aligned(4) int(32))`, which requires being called with a single parameter of type `*readonly dereferencable(4) aligned(4) int(32)`. A conversion does exist (the `derive` instruction does pointer attribute conversion w/o directly changing types), but the problem is that `fn foo<T>(&T)` under these rules doesn't get `readonly`, but if I instantiate `foo<T>` with `i32` and take an fn-ptr to it, then the resulting pointer must be the same type as `fn(&i32)`. Extending mutable similarily to UnsafeCell is a possibility but I run into problems with optimizing C and C++ (specifically, non-mutable subobjects of a const complete object with a mutable subobject can't assume immutability), and if I'm allowed to get a `unique` interior pointer that moving around the exterior pointer doesn't invalidate, then I'm fairly certain I can get a `*unique` pointer using offsetting, due to lack of reachability restrictions (which is basically banned by C and restrained by C++) and a no-attribute pointer to have the same pointer value (which means that both can be passed into a function, and still alias).

I think you could argue that any time you've got a generic IR that doesn't have high-level source language queries, you'd have a similar problem though. This is in fact a pessimisation of what optimizations can be done on a generic IR (e.g. MIR, XIR), but it's not like this is a choice we're making without knowing its the case; we'd love to have more MIR opts in rustc as well. What I *think* you/XIR needs and doesn't have at the moment is to be able to apply the pointer (restricting) attributes to the types, with the semantics that this makes any pointer access to that type act as if it had the attribute. And if you're throwing away the required type information before the point where it would be required to provide/take advantage of pointer attributes on the pointee, then I think you'll just need to accept that you won't be able to optimize Rust code as well as otherwise.

I had originally considered this, but I opted against it in favour of doing the same for pointer types (`__lccc::xlang_pointer_attributes` adds pointer attributes to a pointer type used in a field, there's also `xlang_scalar_attributes` which is the same but for scalar types, and is used by `NonZero*` types). One consideration is that the user could just have plugins stop being run at any point in compilation (incl. before generics are instantiated), which means any and *all* information has to be encoded in the IR directly. Additionally, adding pointer attributes implicitly is bad for two reasons: * One, how would you distinguish between a pointer that can have it validly added vs. one that can't (IE. `*const i32` vs. `&i32`). Both use the XIR pointer type internally * Two, Adding pointer attributes changes the behaviour of operations, hence making it part of the pointer type. Further, the entire core of the pointer model in XIR is based on this special behaviour - implicit derive operations. Adding pointer attributes to pointees means that you come back to the "operations implicitly changing behaviour" The ability to remove pointer attributes, which would be better for the first, was even more problematic, due to backwards and forwards compatibility of the IR - adding an attribute to a type in the future strictly increases the defined behaviour of a program.

…

— Reply to this email directly, view it on GitHub <#236 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGLD2YTRDGN7MAFTBVY56LVRTJ7BANCNFSM4N4GIRZQ> . You are receiving this because you commented.Message ID: ***@***.***>

[RalfJung]

This came up in Zulip again because Tree Borrows, unlike Stacked Borrows, considers UnsafeCell to be "fully infectious" to its neighbors. Noteworthy examples:

• code that was rejected by SB but is now allowed under TB
• a variant that was already allowed with SB since it uses an enum (except that very early variants of SB did reject this, too)

[RalfJung]

I have used PhantomData<UnsafeCell<()>> to opt out of Sync in some cases

IMO that's clearly a hack, so I don't see that as a good argument for "you must keep noalias optimizations in that case".

[chorman0773]

IMO that's clearly a hack, so I don't see that as a good argument for "you must keep noalias optimizations in that case".

Is it though? Using PhantomData arround appropriate types is the way you are supposed to opt-out of auto traits.

[RalfJung]

Yes but UnsafeCell lacks several auto traits -- Sync and Freeze. You cannot reasonably expect its Phantom version to only affect one of them. Nor do the docs say "use a phantom UnsafeCell to opt-out of Sync".

[chorman0773]

Isn't there impl<T: ?Sized> Freeze for PhantomData<T>?

…

On Sun, Aug 6, 2023 at 10:11 Ralf Jung ***@***.***> wrote: Yes but UnsafeCell lacks several auto traits -- Sync and Freeze. You cannot reasonably expect its Phantom version to only affect one of them. — Reply to this email directly, view it on GitHub <#236 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGLD2Y6ZTYUGULTQISFAXDXT6Q2TANCNFSM4N4GIRZQ> . You are receiving this because you commented.Message ID: ***@***.***>

[RalfJung]

Not that I know of.

[chorman0773]

https://stdrs.dev/nightly/x86_64-unknown-linux-gnu/core/marker/trait.Freeze.html lists it.

[CAD97]

In any case, Freeze is currently an internal implementation detail and those details are permitted to change. It currently attempts to answer the question "is any byte shared mutable" roughly as accurately as possible, and thus we have that PhantomData<_>: Freeze unconditionally, but if Freeze were to be exposed and semantic, I would personally expect PhantomData to be transparent to it like it is to all other exposed auto traits.

There's no other good way to opt out of Sync but not Send

You can use PhantomData<&'static mut Cell<()>> for Send + !Sync just like you can use PhantomData<&'static Cell<()>> for !Send + !Sync. But for this reason I've often wanted some kind of PhantomSingleThread: Send + !Sync, PhantomThreadStuck: !Send + Sync, and PhantomThreadLocal: !Send + !Sync, but naming is hard, and "by example" is usually sufficient and also opt out of [Ref]UnwindSafe roughly correctly.

oibit impl rambling

When possible, I do think it's generally preferable to avoid exposing any types with explicit impls for autotraits, because the rustdoc render is preferable for the automatic trait impl than explicit ones, especially since I prefer to bound the impl "by example" but the where obligations should preferably be collapsed in the docs like the auto impl obligations are, e.g. I'd bound a custom Rc's obit impls with where std::Rc<T>: Oibit but still prefer the docs to say where T: Oibit.

[chorman0773]

You can use PhantomData<&'static mut Cell<()>> for Send + !Sync just like you can use PhantomData<&'static Cell<()>> for !Send + !Sync.

I'd note that this also opts-out of UnwindSafe, but I mostly treat {Ref,}UnwindSafe as if it doesn't exist. I don't like adding more decorators than necessary though (Although, says the person who does use PhantomData<&'static UnsafeCell<()>> instead of PhantomData<*const ()>).

And yes, please give actual auto-trait opt-outs. I work with a lot of code that is on older versions of rust, though, for various reasons.

And in general, I think it's reasonable to expect PhantomData to have no runtime implications on a type given that is how it's documented. Another example is a user implementation of Box<T> or other Collections. I would not expect to be able to replace the pointer behind a &Box<AtomicI32>, implemented as:

pub struct Box<T>{
    ptr: NonNull<T>,
    phantom: PhantomData<T>
}

[CAD97]

The Box example is a good counterexample and argues that PhantomData probably should cover !Freeze... though PhantomData's semantics w.r.t. #[dropck_eyepatch] (#[may_dangle]) is awkward at best¹, and downstream types are probably better served by using PhantomData<Box<T>> to "by-example" owning T by pointer, rather than just PhantomData<T>.

Footnotes

1. There's an experiment towards replacing the fact that PhantomData impacts drop glue despite being Copy with splitting #[may_dangle] into two attributes, provisionally #[may_dangle(unused)] (the no-phantom-ownership case) and #[may_dangle(droppable)] (the yes-phantom-ownership case) IIRC + IIUC. That PhantomData behaves like this is certainly surprising and it's unfortunate that the PhantomData-for-#[may_dangle] drop glue effects are observable on stable. ((u8, PhantomData<T>) has no drop glue and doesn't require T to be live when "dropped"; (Box<u8>, PhantomData<T>) has drop glue and requires T to be live when dropped.) ↩

[CAD97]

And in general, I think it's reasonable to expect PhantomData to have no runtime implications on a type given that is how it's documented.

I don't quite agree. The docs say:

Zero-sized type used to mark things that “act like” they own a T.

Adding a PhantomData<T> field to your type tells the compiler that your type acts as though it stores a value of type T, even though it doesn’t really.

which I think would mean that the not-specifically-T-parts of (Some, Fields, T) and (Some, Fields, PhantomData<T>) should have identical semantics. The contentious phrase is I guess the next one:

This information is used when computing certain safety properties.

Does this mean it only impacts static safety properties? Imo that's not what it says, but it could be read that way.

[danielhenrymantilla]

In any case, Freeze is currently an internal implementation detail and those details are permitted to change

But it is not, as I had argued over Zulip

fn foo<'unbounded, T : ?Sized + 'unbounded>() {
    let _: &'unbounded _ = &("not a zst", ::core::marker::PhantomData::<T>);
}

says that there is an unconditional such impl of Freeze.

---

Since we already have PhantomPinned, we could always defined a PhantomUnfreeze along similar lines?

[chorman0773]

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7db3535609b295e98c1971a5acdb101f

Also says that PhantomData<UnsafeCell<u32>>: Freeze

[chorman0773]

Actually, I think that means that PhantomData cannot affect interior mutabiity of containing structure, because of this modification: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=adeff202ddc2cf314b79a3a8b121e980

[RalfJung]

Looks like that would be a breaking change then. We should crater that.

[RalfJung]

Actually, even if I remove the blanket Freeze for UnsafeCell, these tests all still pass...

[chorman0773]

Actually, even if I remove the blanket Freeze for UnsafeCell, these tests all still pass...

Wait, what? Shared reference to !Freeze isn't allowed in a const.

[RalfJung]

const analyzes how the value is constructed in many cases, so e.g. Option<Cell<T>> = None is allowed.

It took me a while, here's a breaking change:

use std::cell::UnsafeCell;
use std::marker::PhantomData;

trait Trait {
    const C: (u32, PhantomData<UnsafeCell<u32>>);
}

fn bar<T: Trait>() {
    let x: &'static (u32, PhantomData<UnsafeCell<u32>>) = &T::C;
}

[chorman0773]

const analyzes how the value is constructed in many cases, so e.g. Option<Cell> = None is allowed.

TBH, this might have issues with the question presented here.

[chorman0773]

Or hmm... Miri at least seems to think that mutating a promoted constant, even an impl !Freeze one, is UB.

[RalfJung]

Promoted constants are put in read-only memory, so yeah their mutation is definitely UB. That's a different "source of UB" that &T-readonly-ness.

TBH, this might have issues with the question presented here.

Yeah there are definitely some interesting potential interactions.

[RalfJung]

There are at least 2 creates that rely on PhantomData being always Freeze, at least for the purpose of creating references in const-eval.

This doesn't necessarily impact the aliasing rules, I think, but it is still certainly useful to know.