Stacked Borrows: Retag (private) fields of ADTs?

[RalfJung]

The following code currently gets rejected by Miri:

use std::cell::{RefCell, Ref};

fn break_it(rc: &RefCell<i32>, r: Ref<'_, i32>) {
    // `r` has a shared reference, it is passed in as argument and hence
    // a protector is added that marks this memory as read-only for the entire
    // duration of this function.
    drop(r);
    // *oops* here we can mutate that memory.
    *rc.borrow_mut() = 2;
}

fn main() {
    let rc = RefCell::new(0);
    break_it(&rc, rc.borrow())
}

A similar issue exists with RefMut, and vec_deque::Drain also has this problem.

In each of these cases, a protector gets added for a reference that is stored in a private field, and that reference gets invalidated while the protector is still active.

Another way to phrase is: Are types allowed to "lie" about the lifetime of references stored in private fields? Also see rust-lang/rust-memory-model#5.

[RalfJung]

With rust-lang/miri#872, retags no longer descend into fields of structs/enums/...

In particular, no barriers/protectors are added for references passed inside other types. This fixes the problem here. But I will leave the issue open because, it is very unclear if that is the right solution -- I implemented it because it helps not reject code that we are not sure is wrong.

[Lokathor]

I'm not super fluent with RefCell stuff, but near as I can tell, break_it should be valid as long as the Ref and the RefCell aren't related. Similarly, main looks like it's totally fine on its own.

Since this is all safe code, then there should be a panic instead of UB when *rc.borrow_mut() = 2; is attempted. Since my guess is that there's no way to make a panic happen correctly in that situation, I think that functions need to relax their barriers until it works right.

[RalfJung]

This is all safe code, so if there is UB it's RefCell's fault. What it could do to avoid UB is to use raw pointers in Ref and RefMut. In some sense that is more correct, but so far it is not clear whether we want to require that.

[Lokathor]

Yeah, I know that in some cases RefCell type can panic to avoid UB, I'm not sure if it's possible here.

If I understand you correctly, the problem that RefCell has is that the LLVM IR for it is being too strict, but RefCell has no way to examine what the IR is of course.

Since something like RefCell can be made by any user of Rust, I think it's better to fix the LLVM IR generation for this sort of situation (if possible) instead of trying to explain this edge case to every user of Rust ever.

[RalfJung]

No, this shouldn't panic. This is an intended usecase for RefCell.

the LLVM IR for it is being too strict, but RefCell has no way to examine what the IR is of course.

Not sure where you are getting that from, LLVM IR has not been mentioned in this thread.
Stacked Borrows is too strict.

[Lokathor]

Ah, I thought that the "Barriers for private fields" was an LLVM IR element.

[RalfJung]

Turns out what we actually do emit noalias for some types that are not references but contain them, such as Pin<&mut T>. Visibility does not affect this.

@eddyb @rkruppe do you know for which types we do this? Is it only newtypes? If we also did this e.g. for Ref, RefMut or vec_deque::Drain, that would be plain wrong. The code for this is here but I cannot really turn that into a high-level description of when we do or do not add noalias.

[RalfJung]

At least Ref and RefMut actually do get noalias... ouch. I'll turn this into a rustc bugreport, for Ref I think we can get UB here even without -Zmutable-noalias.