Stacked Borrows: How precise should UnsafeCell be tracked? #236

RalfJung · 2020-06-12T10:15:28Z

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

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?

The text was updated successfully, but these errors were encountered:

RalfJung · 2020-06-13T09:18:17Z

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 · 2020-11-02T02:14:35Z

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 · 2022-06-26T13:55:12Z

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 · 2022-06-26T14:14:18Z

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 · 2022-06-26T20:49:45Z

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.

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. ↩
Especially since for better or for worse Rust already relies heavily on monomorphizations for performance. ↩

RalfJung · 2022-06-26T22:04:53Z

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 · 2022-06-28T13:25:50Z

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 · 2022-06-28T13:52:14Z

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 · 2022-06-28T14:16:08Z

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 · 2022-06-28T14:23:05Z

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 · 2022-06-28T14:42:53Z

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 · 2022-06-28T17:45:22Z

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 · 2022-06-28T17:47:16Z

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 · 2022-06-29T18:06:02Z

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 · 2022-06-29T23:12:37Z

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 · 2022-07-04T16:13:16Z

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 · 2022-07-05T19:05:24Z

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 · 2022-10-11T07:54:57Z

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 · 2023-04-13T15:05:42Z

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)

chorman0773 · 2023-08-06T14:21:59Z

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 · 2023-08-06T14:34:57Z

Not that I know of.

chorman0773 · 2023-08-06T15:17:53Z

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

CAD97 · 2023-08-06T15:45:44Z

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 · 2023-08-06T15:59:48Z

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 · 2023-08-06T16:22:35Z

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

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 · 2023-08-06T16:30:51Z

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 · 2023-08-06T16:46:10Z

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 · 2023-08-06T17:02:23Z

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

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

chorman0773 · 2023-08-06T17:10:12Z

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 · 2023-08-06T17:35:14Z

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

RalfJung · 2023-08-06T18:57:57Z

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

chorman0773 · 2023-08-06T18:59:42Z

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 · 2023-08-06T19:01:59Z

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 · 2023-08-06T19:37:53Z

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 · 2023-08-06T19:44:20Z

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

RalfJung · 2023-08-06T20:37:26Z

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 · 2023-08-14T17:40:07Z

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.

…dead const-eval interning: accept interior mutable pointers in final value …but keep rejecting mutable references This fixes rust-lang#121610 by no longer firing the lint when there is a pointer with interior mutability in the final value of the constant. On stable, such pointers can be created with code like: ```rust pub enum JsValue { Undefined, Object(Cell<bool>), } impl Drop for JsValue { fn drop(&mut self) {} } // This does *not* get promoted since `JsValue` has a destructor. // However, the outer scope rule applies, still giving this 'static lifetime. const UNDEFINED: &JsValue = &JsValue::Undefined; ``` It's not great to accept such values since people *might* think that it is legal to mutate them with unsafe code. (This is related to how "infectious" `UnsafeCell` is, which is a [wide open question](rust-lang/unsafe-code-guidelines#236).) However, we [explicitly document](https://doc.rust-lang.org/reference/behavior-considered-undefined.html) that things created by `const` are immutable. Furthermore, we also accept the following even more questionable code without any lint today: ```rust let x: &'static Option<Cell<i32>> = &None; ``` This is even more questionable since it does *not* involve a `const`, and yet still puts the data into immutable memory. We could view this as promotion [potentially introducing UB](rust-lang/unsafe-code-guidelines#493). However, we've accepted this since ~forever and it's [too late to reject this now](rust-lang#122789); the pattern is just too useful. So basically, if you think that `UnsafeCell` should be tracked fully precisely, then you should want the lint we currently emit to be removed, which this PR does. If you think `UnsafeCell` should "infect" surrounding `enum`s, the big problem is really rust-lang/unsafe-code-guidelines#493 which does not trigger the lint -- the cases the lint triggers on are actually the "harmless" ones as there is an explicit surrounding `const` explaining why things end up being immutable. What all this goes to show is that the hard error added in rust-lang#118324 (later turned into the future-compat lint that I am now suggesting we remove) was based on some wrong assumptions, at least insofar as it concerns shared references. Furthermore, that lint does not help at all for the most problematic case here where the potential UB is completely implicit. (In fact, the lint is actively in the way of [my preferred long-term strategy](rust-lang/unsafe-code-guidelines#493 (comment)) for dealing with this UB.) So I think we should go back to square one and remove that error/lint for shared references. For mutable references, it does seem to work as intended, so we can keep it. Here it serves as a safety net in case the static checks that try to contain mutable references to the inside of a const initializer are not working as intended; I therefore made the check ICE to encourage users to tell us if that safety net is triggered. Closes rust-lang#122153 by removing the lint. Cc `@rust-lang/opsem` `@rust-lang/lang`

const-eval interning: accept interior mutable pointers in final value …but keep rejecting mutable references This fixes rust-lang/rust#121610 by no longer firing the lint when there is a pointer with interior mutability in the final value of the constant. On stable, such pointers can be created with code like: ```rust pub enum JsValue { Undefined, Object(Cell<bool>), } impl Drop for JsValue { fn drop(&mut self) {} } // This does *not* get promoted since `JsValue` has a destructor. // However, the outer scope rule applies, still giving this 'static lifetime. const UNDEFINED: &JsValue = &JsValue::Undefined; ``` It's not great to accept such values since people *might* think that it is legal to mutate them with unsafe code. (This is related to how "infectious" `UnsafeCell` is, which is a [wide open question](rust-lang/unsafe-code-guidelines#236).) However, we [explicitly document](https://doc.rust-lang.org/reference/behavior-considered-undefined.html) that things created by `const` are immutable. Furthermore, we also accept the following even more questionable code without any lint today: ```rust let x: &'static Option<Cell<i32>> = &None; ``` This is even more questionable since it does *not* involve a `const`, and yet still puts the data into immutable memory. We could view this as promotion [potentially introducing UB](rust-lang/unsafe-code-guidelines#493). However, we've accepted this since ~forever and it's [too late to reject this now](rust-lang/rust#122789); the pattern is just too useful. So basically, if you think that `UnsafeCell` should be tracked fully precisely, then you should want the lint we currently emit to be removed, which this PR does. If you think `UnsafeCell` should "infect" surrounding `enum`s, the big problem is really rust-lang/unsafe-code-guidelines#493 which does not trigger the lint -- the cases the lint triggers on are actually the "harmless" ones as there is an explicit surrounding `const` explaining why things end up being immutable. What all this goes to show is that the hard error added in rust-lang/rust#118324 (later turned into the future-compat lint that I am now suggesting we remove) was based on some wrong assumptions, at least insofar as it concerns shared references. Furthermore, that lint does not help at all for the most problematic case here where the potential UB is completely implicit. (In fact, the lint is actively in the way of [my preferred long-term strategy](rust-lang/unsafe-code-guidelines#493 (comment)) for dealing with this UB.) So I think we should go back to square one and remove that error/lint for shared references. For mutable references, it does seem to work as intended, so we can keep it. Here it serves as a safety net in case the static checks that try to contain mutable references to the inside of a const initializer are not working as intended; I therefore made the check ICE to encourage users to tell us if that safety net is triggered. Closes rust-lang/rust#122153 by removing the lint. Cc `@rust-lang/opsem` `@rust-lang/lang`

RalfJung added C-open-question Category: An open question that we should revisit A-aliasing-model Topic: Related to the aliasing model (e.g. Stacked/Tree Borrows) labels Jun 12, 2020

RalfJung mentioned this issue Jun 13, 2020

RFC: add the Freeze trait to libcore/libstd rust-lang/rfcs#2944

Closed

RalfJung mentioned this issue Nov 3, 2020

Status of Unsafe Code Guidelines Reference #254

Open

RalfJung mentioned this issue Oct 28, 2021

Extend check for UnsafeCell in consts to cover unions rust-lang/rust#90383

Merged

This was referenced Jun 26, 2022

Stacked Borrows: protectors and the dereferenceable attribute #88

Closed

Arc::drop has a (potentially) dangling shared ref rust-lang/rust#55005

Closed

RalfJung mentioned this issue Jul 3, 2022

Optimizing Stacked Borrows (part 2): Shrink Item rust-lang/miri#2315

Merged

3 tasks

RalfJung mentioned this issue Aug 6, 2023

remove blanket-Freeze for PhantomData rust-lang/rust#114559

Closed

madsmtm mentioned this issue Jan 16, 2024

as_ptr()'s use of *mut is misleading sfackler/foreign-types#24

Open

This was referenced Feb 28, 2024

Promotion introduces UB into otherwise well-defined code #493

Open

regression: encountered mutable pointer in final value when "outer scope" rule applies in const/static with interior mutability rust-lang/rust#121610

Closed

RalfJung mentioned this issue Mar 29, 2024

Do not allocate for ZST ThinBox rust-lang/rust#123184

Closed

RalfJung added the A-SB-vs-TB Topic: Design questions where SB and TB are opposite sides of the design axis label Apr 1, 2024

RalfJung mentioned this issue May 16, 2024

[RFC] core::marker::Freeze in bounds rust-lang/rfcs#3633

Open

RalfJung mentioned this issue Aug 21, 2024

const-eval interning: accept interior mutable pointers in final value rust-lang/rust#128543

Merged

Stacked Borrows: How precise should UnsafeCell be tracked? #236

Stacked Borrows: How precise should UnsafeCell be tracked? #236

Comments

RalfJung commented Jun 12, 2020 • edited Loading

RalfJung commented Jun 13, 2020

chorman0773 commented Nov 2, 2020 • edited Loading

RalfJung commented Jun 26, 2022

RalfJung commented Jun 26, 2022

CAD97 commented Jun 26, 2022

Footnotes

RalfJung commented Jun 26, 2022

chorman0773 commented Jun 28, 2022 • edited Loading

RalfJung commented Jun 28, 2022 • edited Loading

chorman0773 commented Jun 28, 2022 via email

RalfJung commented Jun 28, 2022 • edited Loading

chorman0773 commented Jun 28, 2022

RalfJung commented Jun 28, 2022

RalfJung commented Jun 28, 2022

chorman0773 commented Jun 29, 2022

CAD97 commented Jun 29, 2022 • edited Loading

RalfJung commented Jul 4, 2022 • edited Loading

RalfJung commented Jul 5, 2022 • edited Loading

chorman0773 commented Oct 11, 2022 via email

RalfJung commented Apr 13, 2023 • edited Loading

chorman0773 commented Aug 6, 2023 via email

RalfJung commented Aug 6, 2023

chorman0773 commented Aug 6, 2023

CAD97 commented Aug 6, 2023 • edited Loading

chorman0773 commented Aug 6, 2023

CAD97 commented Aug 6, 2023

Footnotes

CAD97 commented Aug 6, 2023

danielhenrymantilla commented Aug 6, 2023 • edited Loading

chorman0773 commented Aug 6, 2023

chorman0773 commented Aug 6, 2023 • edited Loading

RalfJung commented Aug 6, 2023

RalfJung commented Aug 6, 2023

chorman0773 commented Aug 6, 2023

RalfJung commented Aug 6, 2023 • edited Loading

chorman0773 commented Aug 6, 2023

chorman0773 commented Aug 6, 2023

RalfJung commented Aug 6, 2023 • edited Loading

RalfJung commented Aug 14, 2023

RalfJung commented Jun 12, 2020 •

edited

Loading

chorman0773 commented Nov 2, 2020 •

edited

Loading

chorman0773 commented Jun 28, 2022 •

edited

Loading

RalfJung commented Jun 28, 2022 •

edited

Loading

RalfJung commented Jun 28, 2022 •

edited

Loading

CAD97 commented Jun 29, 2022 •

edited

Loading

RalfJung commented Jul 4, 2022 •

edited

Loading

RalfJung commented Jul 5, 2022 •

edited

Loading

RalfJung commented Apr 13, 2023 •

edited

Loading

CAD97 commented Aug 6, 2023 •

edited

Loading

danielhenrymantilla commented Aug 6, 2023 •

edited

Loading

chorman0773 commented Aug 6, 2023 •

edited

Loading

RalfJung commented Aug 6, 2023 •

edited

Loading

RalfJung commented Aug 6, 2023 •

edited

Loading