Open e00E opened 4 months ago
This behavior might be intentional because the documentation for ConstPtr says that Loom doesn't track liveness. I'm not sure. I did not find an existing issue about this topic so I created one. If the behavior is intentional, then this issue can be closed and can serve as documentation for the reason.
Loom doesn't track liveness of pointers in the general case, because that would be very challenging to implement in loom
(and is arguably better served by other tools, such as miri
), but, IMO, that doesn't mean we shouldn't handle this specific case. We probably should treat dropping an UnsafeCell
as a mutable access, IMO.
I've thought about the implementation of this. The naive (is there another?) way to implement this is to implement Drop and get a mutable reference while dropping. That way you detect other active references. This is awkward for two reasons:
into_inner
stops working because you can't destructure Self anymore. We can fix that by storing an Option.Let's try with a Drop
implementation for now. As for into_inner
, we can implement it unsafely.
Actually, I think both the examples written in the issue shouldn't fail, because there is no error according to borrowing/aliasing rules.
Using miri
, this code runs fine:
let cell = std::cell::UnsafeCell::new(0);
let shared = unsafe { &*cell.get() };
let exclusive = unsafe { &mut *cell.get() };
drop(cell);
What is not fine is to access shared
after exclusive
declaration for example.
It makes me think that loom behavior is too much conservative here, because it is tracking the lifetime of the pointers (which shouldn't have lifetime in fact), instead of tracking their accesses as miri
does.
A solution could be to change ConstPtr::deref
/MutPtr::deref
to return a guard instead, and only raise an error if the guard is alive when there is a concurrent exclusive access, or if the cell has been dropped.
Moreover, an interesting thing I've noticed with miri
: the following example doesn't fail
let cell = std::cell::UnsafeCell::new(0);
let shared = unsafe { &*cell.get() };
drop(cell);
dbg!(*shared);
Because UnsafeCell<i32>
doesn't need to be dropped, it seems that miri
doesn't consider drop(cell)
as an invalidation. I've asked about it in zulip.
But if this behavior is correct, we would need to take Actually, because we cannot track the dropping of a potential parent struct as std::mem::needs_drop<T>()
result in account to invalidate the cell when it's dropped.miri
does, it is not possible to take std::mem::needs_drop
in account.
It is a good observation that access is the true problem. I touch on this in the OP and hawkw mentions it too. Search for "liveness". As hawkw writes, it is hard for loom to be as correct as miri for this case. The loom check is an approximation of the correct check.
My examples technically do not access the cell contents. I omitted this for brevity because it doesn't matter since loom does not enforce this. Imagine that the examples had extra lines like this:
unsafe { first_ref.deref() };
unsafe { second_ref.deref() };
This is what loom is meant to help catch.
Consider the following code:
The test fails because loom correctly detects that mutable access is not allowed while there is already immutable access.
Now change the last line of the code:
Loom no longer detects an error.
This is surprising because dropping could be considered a write. For example, if instead of
()
I was using a type with a custom Drop implementation, then I would get a mutable reference toself
. This write doesn't go through UnsafeCell but it still leads to the reference guarantees being violated.This behavior might be intentional because the documentation for ConstPtr says that Loom doesn't track liveness. I'm not sure. I did not find an existing issue about this topic so I created one. If the behavior is intentional, then this issue can be closed and can serve as documentation for the reason.