Stream: t-lang

Topic: UnsafeCell exposes niche(s) of T; should it? #68206


pnkfelix (Jan 14 2020 at 16:26, on Zulip):

Interesting question posed on issue #68206

pnkfelix (Jan 14 2020 at 16:28, on Zulip):

at least, I find the question of "What kind of 'primitive' is UnsafeCell supposed to be? Something that you are expected to add extra safe-guards to in order to prevent compiler from doing things like niche optimization?

simulacrum (Jan 14 2020 at 16:32, on Zulip):

I think to me at least it makes sense to not combine UnsafeCell with niches, though it may also be a good argument that using that niche can never be safe as you then can't really abstract around it, with &mut T now no longer making the guarantees it should be able to

pnkfelix (Jan 14 2020 at 16:32, on Zulip):

I cannot tell if your first sentence is stating "UnsafeCell should expose niche" or not

pnkfelix (Jan 14 2020 at 16:34, on Zulip):

(probably because my brain is too willing to freely reinterpret what "combine" might mean here.)

simulacrum (Jan 14 2020 at 19:58, on Zulip):

ah, I think it should, because generally we don't combine things like that

simulacrum (Jan 14 2020 at 19:58, on Zulip):

e.g. Arc<Mutex<_>> over SharedMemory<_> basically

nikomatsakis (Jan 15 2020 at 19:44, on Zulip):

This sort of vagely relates to the question of dereferenceable

nikomatsakis (Jan 15 2020 at 19:45, on Zulip):

that said, I feel like.. hmm.. I'm having memories of discussing this with @RalfJ at last year's all hands :)

nikomatsakis (Jan 15 2020 at 19:45, on Zulip):

basically the question is sort of 'what is the escape hatch that says "don't trust these bits"', in part?

nikomatsakis (Jan 15 2020 at 19:45, on Zulip):

I think @RalfJ was advocating that this should be union rather than UnsafeCell

nikomatsakis (Jan 15 2020 at 19:45, on Zulip):

but I do think we should be able to answer the question, regardless

RalfJ (Jan 15 2020 at 19:52, on Zulip):

I think RalfJ was advocating that this should be union rather than UnsafeCell

what's "this"?

simulacrum (Jan 15 2020 at 19:55, on Zulip):

"Don't trust these bits" I believe

RalfJ (Jan 15 2020 at 19:56, on Zulip):

I mean unions already do that (or I think they should, that's https://github.com/rust-lang/unsafe-code-guidelines/issues/73). not sure what niko was saying.^^

simulacrum (Jan 15 2020 at 21:31, on Zulip):

I think Niko is saying that UnsafeCell should not also do this

simulacrum (Jan 15 2020 at 21:31, on Zulip):

i.e., if you want this behavior for UnsafeCell then you should also add a union in

simulacrum (Jan 15 2020 at 21:32, on Zulip):

what is not clear to me is that there are use cases for UnsafeCell where the niche is exposed

simulacrum (Jan 15 2020 at 21:32, on Zulip):

I think the answer is that all non-threadsafe use cases are fine to expose the niche in, but I'm not confident in that assessment :)

pnkfelix (Jan 16 2020 at 03:16, on Zulip):

What are arguments for not having UnsafeCell also do this? The main one I can imagine is about "expressive power" : someone who knows what they are doing will be able to put the lego-pieces of the type system to precisely express what they want

pnkfelix (Jan 16 2020 at 03:17, on Zulip):

but my Point of View is that out of the box, this is a huge footgun for UnsafeCell, and there is probably too much code out there that was written without thinking about this issue.

centril (Jan 16 2020 at 06:27, on Zulip):

what is not clear to me is that there are use cases for UnsafeCell where the niche is exposed

There's discussion on the issue re. Option<Cell<T>>

centril (Jan 16 2020 at 06:28, on Zulip):

[...], and there is probably too much code out there that was written without thinking about this issue.

I am by default intensely skeptical of these types of assertions given past experience.

nikomatsakis (Jan 16 2020 at 16:14, on Zulip):

My current opinion is that UnsafeCell<T> should inhibit both

We know, after all, that we cannot access the data within without potentially triggering data races.

But it would be really good to start creating a comprehensive collection of the examples and cases to keep in mind.

nikomatsakis (Jan 16 2020 at 16:15, on Zulip):

This feels fairly analogous to me to the auto trait design:

nikomatsakis (Jan 16 2020 at 16:15, on Zulip):

We don't consider *mut Send, but you can opt back in at outer levels (except we don't have this opt-in .. yet?)

pnkfelix (Jan 16 2020 at 16:15, on Zulip):

[...], and there is probably too much code out there that was written without thinking about this issue.

I am by default intensely skeptical of these types of assertions given past experience.

Heh. Not me, I tend to buy those claims when I see them. Only defense Rust has against them is that it is still very young and not as popular as we might like.

nikomatsakis (Jan 16 2020 at 16:15, on Zulip):

I'd be inclined to put off the opt-in until we're convinced it's necessary

nikomatsakis (Jan 16 2020 at 16:16, on Zulip):

I think Niko is saying that UnsafeCell should not also do this

I was not saying that, but I thought I remembered @RalfJ saying it. It sounds like I may have been mistaken.

centril (Jan 16 2020 at 16:23, on Zulip):

Heh. Not me, I tend to buy those claims when I see them. Only defense Rust has against them is that it is still very young and not as popular as we might like.

Whereas I require constructive evidence to believe them. ;) Sometimes it might be the case that some widely used crate does something bad, but then it's also easier to fix centrally.

centril (Jan 16 2020 at 16:27, on Zulip):

As far as back-compat is concerned, UnsafeCell<T> is also repr(transparent), so it seems to me we have already made an observable promise that the niche will be exposed

centril (Jan 16 2020 at 16:27, on Zulip):

and folks can rely on that

pnkfelix (Jan 16 2020 at 16:28, on Zulip):

I spent some time last night trying to review the specs

pnkfelix (Jan 16 2020 at 16:28, on Zulip):

I couldn't find a place in the reference where the handling of the niche on repr(transparent) on structs was discussed

centril (Jan 16 2020 at 16:28, on Zulip):

As a separate point, I do find it elegant, simple, and understandable that UnsafeCell<T> does one thing, not 3 things

pnkfelix (Jan 16 2020 at 16:28, on Zulip):

and I continue to believe that there is no reason to make such an observable promise (from repr(transparent) that you describe.

pnkfelix (Jan 16 2020 at 16:29, on Zulip):

I couldn't find a place in the reference where the handling of the niche on repr(transparent) on structs was discussed

(I did see it in the comments, especially in the repr(transparent) on enum/union RFC PR, but no where regarding the original feature...)

pnkfelix (Jan 16 2020 at 16:30, on Zulip):

and I continue to believe that there is no reason to make such an observable promise (from repr(transparent) that you describe.

Though I would be happy to see a, as you put it, constructive example of why such a promise has utility.

centril (Jan 16 2020 at 16:30, on Zulip):

Option<Cell<T>> was an example

centril (Jan 16 2020 at 16:31, on Zulip):

although it was claimed that that occurs infrequently

pnkfelix (Jan 16 2020 at 16:32, on Zulip):

i don't see why the optimization of Option<Cell<T>> needs to be something that follows from repr(transparent). That strikes me as an implementation detail of Cell<T> itself.

pnkfelix (Jan 16 2020 at 16:32, on Zulip):

to be honest, though, I'd be perfectly happy with something like #{repr(no_niche)] that we could apply to any ADT (struct/enum/union)

centril (Jan 16 2020 at 16:33, on Zulip):

that would be a utility of UnsafeCell<T> exposing niches specifically

pnkfelix (Jan 16 2020 at 16:34, on Zulip):

and then if that composed to yield #[repr(transparent, no_niche), then it seems like you would get what you want (#[repr(transparent)] alone has simple semantic interpretation) and I would get what I want (the ability to express the ABI desirata without exposing the niche unnecessarily.

centril (Jan 16 2020 at 16:34, on Zulip):

repr(transparent) exposing niches feels easy to motivate: just take a newtype wrapper around &'a T and then an Option<_> around that

centril (Jan 16 2020 at 16:34, on Zulip):

and then if that composed to yield #[repr(transparent, no_niche), then it seems like you would get what you want (#[repr(transparent)] alone has simple semantic interpretation) and I would get what I want (the ability to express the ABI desirata without exposing the niche unnecessarily.

This feels elegant.

centril (Jan 16 2020 at 16:35, on Zulip):

Although I wouldn't want to attach it to UnsafeCell<T> necessarily

pnkfelix (Jan 16 2020 at 16:35, on Zulip):

yeah that we might debate

centril (Jan 16 2020 at 16:35, on Zulip):

@pnkfelix but your no_niche feels like "composable language design"

centril (Jan 16 2020 at 16:36, on Zulip):

we have a more short-term question of fixing the soundness hole though

pnkfelix (Jan 16 2020 at 16:36, on Zulip):

yes

pnkfelix (Jan 16 2020 at 16:36, on Zulip):

use of a niche is itself a compiler optimziation attached to the unstable Rust layout

pnkfelix (Jan 16 2020 at 16:37, on Zulip):

so I believe we would be within our rights to still make UnsafeCell<T> act as if it has no_niche

pnkfelix (Jan 16 2020 at 16:37, on Zulip):

for short term

pnkfelix (Jan 16 2020 at 16:37, on Zulip):

that, or we can go ahead and make a NoNiche<T> wrapper type

pnkfelix (Jan 16 2020 at 16:37, on Zulip):

and deploy it in libstd where necessary

centril (Jan 16 2020 at 16:38, on Zulip):

I'll need to think about whether I agree we are within our rights re. the above discussion re. promises already made (or not made)

centril (Jan 16 2020 at 16:38, on Zulip):

NoNiche<T> feels more robust as a documentation tool though

pnkfelix (Jan 16 2020 at 16:39, on Zulip):

I'm working on #[repr(no_niche)] in my branch where I'm trying to make UnsafeCell hide its niche. It seemed like a natural thing to poke on while I figure out how to actually accomplish the end goal.

XAMPPRocky (Jan 16 2020 at 16:44, on Zulip):

Slightly off topic, what does "niche" mean in this context? I tried searching for it, but googling "niche programming" isn't really giving helpful answers. :sweat_smile:

centril (Jan 16 2020 at 16:45, on Zulip):

@XAMPPRocky it's the thing that allows Option<&T> to have the same size as &T

pnkfelix (Jan 16 2020 at 17:04, on Zulip):

@XAMPPRocky more specifically: enum E { A(...), B(...), ... } uses some state called a "discriminant" to differentiate which variant of the enum you are using. A niche allows Rust to store the discriminant intermixed with the payload(s) attached to the variants.

rkruppe (Jan 16 2020 at 17:16, on Zulip):

There is one relatively authoritative and user-visible sign that repr(transparent) guarantees something about niches: the behavior of the improper_ctypes lint after https://github.com/rust-lang/rust/pull/60300
In particular, Option<UnsafeCell<&i32>> is considered FFI-safe (and same for Cell)

Lokathor (Jan 16 2020 at 17:28, on Zulip):

I know that many, perhaps most, unsafe code authors assume repr transparent means "the same layout as the underlying type".
(leaving aside the fact that "layout" has no formal rust definition yet)

RalfJ (Jan 16 2020 at 17:49, on Zulip):

As a separate point, I do find it elegant, simple, and understandable that UnsafeCell<T> does one thing, not 3 things

note however that it turns out that UnsafeCell having niches means it's really hard to define what that one thing is

RalfJ (Jan 16 2020 at 17:50, on Zulip):

a variant of stacked borrows that correctly models the "one thing UnsafeCell does" with support for exposing niches might be possible, but I think it will require things I would like to strictly avoid -- like "ghost state" that records the actual enum discriminant in some magical place even when in memory, we just see its encoding via some niche. this is exactly the same kind of "ghost state" as the "active union variant", and I did everything I could to make sure we do not have that ;)

centril (Jan 16 2020 at 17:51, on Zulip):

@RalfJ will you be available for the lang meeting today?

RalfJ (Jan 16 2020 at 17:51, on Zulip):

@centril so, while I agree with not mixing concepts, I also cannot say that I know how to define the concept of interior mutability on its own, ignoring niches

RalfJ (Jan 16 2020 at 17:52, on Zulip):

I'm afraid I have tons of stuff to prepare before flying to a conference on Saturday :/

centril (Jan 16 2020 at 17:52, on Zulip):

ah; have fun at the conference

RalfJ (Jan 16 2020 at 17:52, on Zulip):

(and I'll give a talk about Stacked Borrows there ;)

pnkfelix (Jan 16 2020 at 17:58, on Zulip):

There is one relatively authoritative and user-visible sign that repr(transparent) guarantees something about niches: the behavior of the improper_ctypes lint after https://github.com/rust-lang/rust/pull/60300
In particular, Option<UnsafeCell<&i32>> is considered FFI-safe (and same for Cell)

Okay, I can understand the argument that a user might reasonably expect (based on the use of repr(transparent) and the semantics you are ascribing it w.r.t. niches), that Option<UnsafeCell<&i32>> is FFI-safe.

For now I remain internally divided about whether that type should be FFI-safe, which is a slightly different matter.

pnkfelix (Jan 16 2020 at 18:08, on Zulip):

/me wonders if we can/should instead lint for occurrences of UnsafeCell in a type that implements Sync, and if such occurrences also expose a niche, issue a warning.... (this option may have been already discussed in the issue)

rkruppe (Jan 16 2020 at 18:09, on Zulip):

Personally I am absolutely convinced that it should not be, because I don't think there's good use cases for it and because I think UnsafeCell should not have niches (for the same reason it should inhibit dereferenceable). But as you say, that is a different question.

centril (Jan 16 2020 at 18:11, on Zulip):

Ralf's point re. the formal model is compelling at any rate.

centril (Jan 16 2020 at 18:12, on Zulip):

The memory / perf benefits may be small in comparison to that cost

rkruppe (Jan 16 2020 at 19:30, on Zulip):

Aside: I can't help but think that we would be in less of a mess now if we'd stuck to repr(transparent) being only for passing things by value in FFI, instead of also (ab)using it as a marker indicating identical memory layout on types that never need to be passed by value in FFI.

Lokathor (Jan 16 2020 at 21:08, on Zulip):

throw it on the edition pile?

centril (Jan 16 2020 at 21:10, on Zulip):

The discussion in the meeting unanimously resolved in favor of inhibiting niches on UnsafeCell<T>

nikomatsakis (Jan 16 2020 at 22:39, on Zulip):

ah yes, I had forgotten about rust-lang/unsafe-code-guidelines#204...

nikomatsakis (Jan 16 2020 at 22:53, on Zulip):

Aside: I can't help but think that we would be in less of a mess now if we'd stuck to repr(transparent) being only for passing things by value in FFI, instead of also (ab)using it as a marker indicating identical memory layout on types that never need to be passed by value in FFI.

Remind me, @rkruppe, when did we decide to alter the meaning? I actually thought #[repr(transparent)] was only used for controlling ABI, and said nothing in particular about memory layout?

nikomatsakis (Jan 16 2020 at 22:58, on Zulip):

I started to write-up some notes on the issue in a hackmd. @RalfJ I was trying to write-up notes about unsafe-code-guidelines#204 and I wanted to see whether you agreed with this summary:

In particular, I agree the code is surprising, but I'm not sure if it can be blamed purely on niches or lack thereof. If I recall, you were arguing for a "infectious" semantics for UnsafeCell, in which having an UnsafeCell inside of a struct means that (from stacked borrows point-of-view) the entire struct is writable via a shared reference. (Though presently the semantics are somewhat different, and extend only through enums.)

In that case, it seems like a &Option<Cell<&u32>> may wind up changing from &Some to &None regardless, right? (Because the discriminant, even if not stored inside the Cell, would still be in the same struct.)

rkruppe (Jan 16 2020 at 23:08, on Zulip):

The meaning of the attribute hasn't changed, I am just referring to what it's used for. It's always been specified to give ABI compatibility and motivated by that, but as a side effect it also guarantees size+align equal to the non-ZST field (the size part is necessary for ABI compatibility, the align part perhaps not but it's still implied by the RFC). People took note and started using repr(transparent) to document and/or enforce that SomeWrapper<T> has the same memory layout as T -- perhaps for the lack of a better alternative (repr(C) implies a host or other things you may not want or need, and without any repr the layout is technically unspecified).

nikomatsakis (Jan 16 2020 at 23:09, on Zulip):

Actually, I believe we did specify it in the UCG

nikomatsakis (Jan 16 2020 at 23:09, on Zulip):

which of course aren't formally decided upon

nikomatsakis (Jan 16 2020 at 23:09, on Zulip):

but we might want to start moving towards changing that

nikomatsakis (Jan 16 2020 at 23:10, on Zulip):

see this section

nikomatsakis (Jan 16 2020 at 23:10, on Zulip):

but perhaps I'm confused

rkruppe (Jan 16 2020 at 23:13, on Zulip):

Oh, indeed. But as you say, UCG isn't normative. Moreover, this part is just a few months old (https://github.com/rust-lang/unsafe-code-guidelines/pull/164) so UnsafeCell and Cell being transparent predates it.

rkruppe (Jan 16 2020 at 23:16, on Zulip):

My framing above may be ahistorical, even the author of the RFC wanted to make UnsafeCell transparent -- not to pass UnsafeCells by values, but just to shut up improper_ctypes about UnsafeCell fields of structs passed "by pointer". See e.g. https://github.com/rust-lang/rfcs/pull/1758#issuecomment-249611805

Lokathor (Jan 17 2020 at 00:41, on Zulip):

It seems that "same layout as the inner type" is very useful, should it be introduced as a separate attribute?

pnkfelix (Jan 17 2020 at 01:08, on Zulip):

My current plan is to introduce #[repr(no_niche)] (or maybe #[repr(hide_niche)]), and then #[repr(tranparent, no_niche)] would have the ABI-oriented meaning, while #[repr(transparent)] would have the layout-oriented one.

pnkfelix (Jan 17 2020 at 01:08, on Zulip):

(I just mention this as a way of saying "I think that is a clean way to express these distinct ideas")

RalfJ (Jan 17 2020 at 08:30, on Zulip):

@nikomatsakis

In this case, the code converts a &Option<Cell<&u32>> into a *mut usize and writes a 0 to it. This changes the Option from Some to None. This means that, from the caller's point-of-view, if we invoke a function that takes an argument of type &Option<Cell<&u32>> we must assume that this argument may spontaneously change from &Some(_) to &None, even though safe code could not possibly do that.

It's not just that safe could couldnt do it, it's that this violates the idea that shared references point to immutable data: the discriminant of Option<Cell<i32>> is logically outside the UnsafeCell, so arguably it should be subject to the "does not change" guarantee and the optimizer should be allowed to exploit that. except that Stacked Borrows is location-based, so even in its most precise handling of UnsafeCell, it cannot justify this optimization. Justifying the optimization would require the crazy "shadow discriminant" stuff I mentioned.

RalfJ (Jan 17 2020 at 08:31, on Zulip):

In particular, I agree the code is surprising, but I'm not sure if it can be blamed purely on niches or lack thereof. If I recall, you were arguing for a "infectious" semantics for UnsafeCell, in which having an UnsafeCell inside of a struct means that (from stacked borrows point-of-view) the entire struct is writable via a shared reference. (Though presently the semantics are somewhat different, and extend only through enums.)

Indeed Stacked Borrows currently is less precise about UnsafeCell than a location-based model could be -- but the point is that with Option<Cell<bool>>, even if we made it fully precise, it'd still be "wrong" in some sense.

RalfJ (Jan 17 2020 at 08:41, on Zulip):

but indeed, with the "infectious" semantics (as it is currently implement for enums -- but structs are still handled precisely), it doesnt really make much of a difference whether Cell exposes niches or not

RalfJ (Jan 17 2020 at 08:42, on Zulip):

so, yeah, this is certainly not entirely clear-cut. I merely listed this as further evidence that UnsafeCell with niches is "odd" even in the sequential case -- with consequences for potential variants of Stacked Borrows, but indeed without direct ramifications for the current version

centril (Jan 17 2020 at 08:43, on Zulip):

@RalfJ Your argument re. active union field & stacked borrows weighed heavily into our decision last night :slight_smile:

RalfJ (Jan 17 2020 at 08:45, on Zulip):

hm, okay^^. maybe I should have pointed out the part about the "infectious" semantics earlier...
anyway, I need to start doing real work now, ttyl

centril (Jan 17 2020 at 08:45, on Zulip):

have fun

rkruppe (Jan 17 2020 at 10:24, on Zulip):

My current plan is to introduce #[repr(no_niche)] (or maybe #[repr(hide_niche)]), and then #[repr(tranparent, no_niche)] would have the ABI-oriented meaning, while #[repr(transparent)] would have the layout-oriented one.

@pnkfelix I'm not sure if this explanation makes sense to me. Whether there are niches is not really call ABI vs memory layout of the type itself, and more about a further property that does not fit under either: for which type constructors Foo<T> does Foo<Transparent<X>> have the same memory layout and/or call ABI as Foo<X>? There are many Foo where this is false even when niches are "propagated" by Transparent, and many Foo where it's true even when niches are "hidden", and it can be relevant to both call ABI and memory layout (cf. Option<UnsafeCell<&T>> being FFI safe or not). So I consider it a related but separate question from whether Transparent<X>has the same "layout" and/or "ABI" (however we define those terms) as X.

I'm also wondering if introducing repr(no_niche) is really better than just special-casing UnsafeCell: is there any other type that would have any reason whatsoever to use it? It seems to me that the inability to exploit niches is inseparable from interior mutability, and while some types with interior mutability (like Cell) might want to opt back into it for performance, there doesn't seem to be any incentive for types without interior mutability to opt out of it.

RalfJ (Jan 17 2020 at 11:54, on Zulip):

okay, turns out that we don't need to even talk about how "infectious" UnsafeCell are... https://github.com/rust-lang/rust/issues/68303 demonstrates that there is a more fundamental problem with niches and UnsafeCell and stacked borrows. And it's blindingly obvious, so I wonder why I didn't see that earlier... probably I'm just stretched too thin so I dont give each thing that comes up the time it needs :/

It's exactly the same thing as with concurrency: a mutable reference gets created, then a conflicting read happens through another pointer (invalidating the mutable reference), and then the (now invalidated) mutable reference gets used

RalfJ (Jan 17 2020 at 11:56, on Zulip):

well, at least this reaffirms the recommendation I gave yesterday, that interior mutability with niches is a problem even in sequential code -- maybe my intuition saw things I didnt entirely realize yet ;)

simulacrum (Jan 17 2020 at 12:46, on Zulip):

hm, so maybe I'm failing to understand something, but it feels like in the niches-don't-propagate through UnsafeCell model, then that code should/would pass miri? As the discriminant's place would be different, and the enum there would be essentially equivalent to a discriminant (u8) + a separate struct/union with the (maybe present) RefCell?

simulacrum (Jan 17 2020 at 12:47, on Zulip):

maybe stacked borrows does not model &(T, UnsafeCell<U>) -> &mut U as retaining &T in the allowed set in some apis though

nikomatsakis (Jan 17 2020 at 14:52, on Zulip):

@RalfJ thanks for your comments. Did you see what I wrote in the hackmd section? That was based on your prior comments, but I think it agrees with what you wrote. (I've not checked out rust-lang/rust#68303 yet, I guess I will extend the hackmd once I do...)

nikomatsakis (Jan 17 2020 at 14:53, on Zulip):

Oh, ok, I see what rust-lang/rust#68303 is about. Yes, very much.

nikomatsakis (Jan 17 2020 at 14:53, on Zulip):

This is the same basic point.

nikomatsakis (Jan 17 2020 at 14:53, on Zulip):

(But in reverse)

nikomatsakis (Jan 17 2020 at 14:54, on Zulip):

That is, here, the problem is that we are reading the discriminant and this is creating a borrow of the payload

gnzlbg (Jan 17 2020 at 16:41, on Zulip):

@nikomatsakis thank you for taking the time to prepare the hackmd

gnzlbg (Jan 17 2020 at 16:41, on Zulip):

I have some questions

gnzlbg (Jan 17 2020 at 16:42, on Zulip):

A &Option<Mutex<bool>>, for example, might be shared between two threads

IIUC Mutex has a niche, because it contains an UnsafeCell<bool> that has a niche, is that correct ?

gnzlbg (Jan 17 2020 at 16:46, on Zulip):

And two threads can match on the option, using the discriminant, one thread can get ahold of the lock, and write to the bool, which changes the discriminant, and there is a data-race. OTOH, &Mutex<Option<bool>> would not be unsound, right ?

gnzlbg (Jan 17 2020 at 16:46, on Zulip):

Since the discriminant is only modified by the thread that has the Mutex

gnzlbg (Jan 17 2020 at 16:48, on Zulip):

So a very different approach might be to not make Option<T>: Sync when T: Sync, but only when T: Sync and the option does not store the discriminant within T due to a layout optimizations, but this approach is probably not ok because it would mean that type checking would depend on layout optimizations..

gnzlbg (Jan 17 2020 at 16:50, on Zulip):

Inhibiting niches in UnsafeCell<T> does seem like the best way to fix things

nikomatsakis (Jan 17 2020 at 16:54, on Zulip):

IIUC Mutex has a niche, because it contains an UnsafeCell<bool> that has a niche, is that correct ?

correct

nikomatsakis (Jan 17 2020 at 16:55, on Zulip):

And two threads can match on the option, using the discriminant, one thread can get ahold of the lock, and write to the bool, which changes the discriminant, and there is a data-race. OTOH, &Mutex<Option<bool>> would not be unsound, right ?

correct

gnzlbg (Jan 17 2020 at 16:55, on Zulip):

Specifically, the type Option<UnsafeCell<&i32>> is currently considered FFI-safe (and same for Cell), as it would be represented as a Option<&i32> which in turn is a nullable pointer. After this change, that will no longer be the case.

I think extern blocks should require unsafe, so I'm not to worried about that, the FFI-safe lints are a "best effort" warning, and we have tuned those in the past.

nikomatsakis (Jan 17 2020 at 16:56, on Zulip):

So a very different approach might be to not make Option<T>: Sync when T: Sync, but only when T: Sync and the option does not store the discriminant within T due to a layout optimizations, but this approach is probably not ok because it would mean that type checking would depend on layout optimizations..

this would also be wildly backwards incompatible

RalfJ (Jan 17 2020 at 18:42, on Zulip):

hm, so maybe I'm failing to understand something, but it feels like in the niches-don't-propagate through UnsafeCell model, then that code should/would pass miri? As the discriminant's place would be different, and the enum there would be essentially equivalent to a discriminant (u8) + a separate struct/union with the (maybe present) RefCell?

@simulacrum yes, my hypothesis is that removing niches from UnsafeCell will fix this bug as well. I can try this in Miri once a PR for the niche removal exists.

RalfJ (Jan 17 2020 at 18:42, on Zulip):

(well, I'll be travelling next week, so -- after I'm back, probably)

RalfJ (Jan 17 2020 at 18:43, on Zulip):

maybe stacked borrows does not model &(T, UnsafeCell<U>) -> &mut U as retaining &T in the allowed set in some apis though

I dont understand what you are saying here

simulacrum (Jan 17 2020 at 18:43, on Zulip):

Ah okay, so you were talking about current behavior I think

simulacrum (Jan 17 2020 at 18:44, on Zulip):

I was trying to say that it seemed like once the discriminant was not in a niche then things should work just fine

simulacrum (Jan 17 2020 at 18:44, on Zulip):

Which I think we agree with

RalfJ (Jan 17 2020 at 18:45, on Zulip):

@nikomatsakis well, I had a brief phase of "you are right that the 'infectious' UnsafeCell almost make this niche thing a moot point for Stacked Borrows", followed by that miri issue and realizing "nope the point is not moot at all, this is important and yes the niches should go, even for sequential code".
when I saw that miri issue, things seemed so obvious that I really wonder why I didnt see it before... it was right there in front of my eyes whenever I had niche trouble with Stacked Borrows, but I was blinded because our formal models all dont implement niche optimizations, and I probably thought too much like them. I think that should teach me something, I'll have to reflect on it.

RalfJ (Jan 17 2020 at 18:47, on Zulip):

Ah okay, so you were talking about current behavior I think

yes I was -- I should probably be more clear when there are many candidate models floating around^^

nikomatsakis (Jan 18 2020 at 13:03, on Zulip):

@RalfJ FWIW, I feel the same way about #57893 -- I remember thinking to myself when implementing the current checks years ago "this seems so simplistic; can this really suffice?". Ah well. Anyway, I extended the hackmd section on the "deeper conflict" and I think I captured what's going on.

I admit to some discomfort still about how to square that text with the idea that we may sometimes expand the "scope" of struct affected by an UnsafeCell.

RalfJ (Jan 26 2020 at 16:57, on Zulip):

I admit to some discomfort still about how to square that text with the idea that we may sometimes expand the "scope" of struct affected by an UnsafeCell.

hm... that is entirely orthogonal though? the latest counterexample doesnt interact with that "scope expansion" at all

Last update: Jan 28 2020 at 00:40UTC