Miri: properly check return-position noalias

[RalfJung]

As discussed in rust-lang/unsafe-code-guidelines#385, Box-returning functions get a noalias on their return value, which has a bunch of extra aliasing rules that Miri did not properly check.

We now do this by marking the post-return retags as FnReturn, and then when retagging a Box in that vein we clear the borrow stack of all the other tags. On its own this change is incompatible with the weak protectors on Box arguments that I recently introduced to fix Box noalias enforcement, so we replace those protectors by also doing such a "clear" retagging in FnEntry for Box. Conveniently this means we can remove weak protectors entirely while still properly enforcing noalias (as far as I can tell).

Basically this means that Box cannot be reborrowed; when they are passed to and from a function, they always must be the only pointer used henceforth for this allocation. With Box not being a reference type, this kind of makes sense I guess. There is a chance that this might break even more unsafe code that uses Box in creative ways, though that seems unlikely -- when a Box is passed to a function, I would expect that function to either deallocate or return the Box; the new point now is that the caller must use the returned box rather than considering this a "reborrow" and sticking to some sneaky copy of the old box. Such code would be unsound with return-position noalias anyway.

@saethlin I hope this doesn't break the tag cache in subtle ways, though given that it can handle clearing the stack for the "unknown bottom" situation, I assume that will also work here.

One odd thing that can now happen is that the "base tag" of an allocation might cease to be valid for that allocation. But I don't think we ever assumed that it would always remain valid, and this does seem to match the semantics of return-position noalias, so I think this is fine.

r? @oli-obk @saethlin

[rustbot]

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[rust-log-analyzer]

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

  IMAGE: x86_64-gnu-tools
##[endgroup]
From https://github.com/rust-lang/rust
 * branch              master     -> FETCH_HEAD
Searching for toolstate changes between 9b889e53e78a186a861a8407c225f9d8e0d436f5 and a81054320b615da05fe0c3119fe339b51114f1c9
Tool subtrees were updated
##[group]Run src/ci/scripts/verify-channel.sh
src/ci/scripts/verify-channel.sh
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
---

actual output differed from expected
--- tests/pass/box-custom-alloc.stderr
+++ <stderr output>
+error: Undefined Behavior: trying to retag from <TAG> for Unique permission at ALLOC[0x0], but that tag does not exist in the borrow stack for this location
+   |
+   |
+LL | pub unsafe fn drop_in_place<T: ?Sized>(to_drop: *mut T) {
+   | |
+   | |
+   | trying to retag from <TAG> for Unique permission at ALLOC[0x0], but that tag does not exist in the borrow stack for this location
+   | this error occurs as part of retag at ALLOC[0x0..0x1]
+   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
+   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
+   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
+help: <TAG> was created here, as the base tag for ALLOC
+   |
+   |
+LL |     let mut space = vec![MaybeUninit::new(0); 1];
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+help: <TAG> was later invalidated at offsets [0x0..0x1] by a Unique Box retag
+   |
+   |
+LL |     let boxed = Box::new_in([42u8; 1], &once_alloc);
+   = note: BACKTRACE (of the first span):
+   = note: BACKTRACE (of the first span):
+   = note: inside `std::ptr::drop_in_place::<[std::mem::MaybeUninit<u8>]> - shim(None)` at RUSTLIB/core/src/ptr/mod.rs:LL:CC
+   = note: inside `<std::vec::Vec<std::mem::MaybeUninit<u8>> as std::ops::Drop>::drop` at RUSTLIB/alloc/src/vec/mod.rs:LL:CC
+   = note: inside `std::ptr::drop_in_place::<std::vec::Vec<std::mem::MaybeUninit<u8>>> - shim(Some(std::vec::Vec<std::mem::MaybeUninit<u8>>))` at RUSTLIB/core/src/ptr/mod.rs:LL:CC
+  --> $DIR/box-custom-alloc.rs:LL:CC
+   |
+LL | }
+   | ^
---
+


There were 1 unmatched diagnostics that occurred outside the testfile and had no pattern
    Error: Undefined Behavior: trying to retag from <3064> for Unique permission at alloc1631[0x0], but that tag does not exist in the borrow stack for this location
full stderr:
full stderr:
error: Undefined Behavior: trying to retag from <3064> for Unique permission at alloc1631[0x0], but that tag does not exist in the borrow stack for this location
   |
   |
LL | pub unsafe fn drop_in_place<T: ?Sized>(to_drop: *mut T) {
   | |
   | |
   | trying to retag from <3064> for Unique permission at alloc1631[0x0], but that tag does not exist in the borrow stack for this location
   | this error occurs as part of retag at alloc1631[0x0..0x1]
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <3064> was created here, as the base tag for alloc1631
   |
   |
LL |     let mut space = vec![MaybeUninit::new(0); 1];
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: <3064> was later invalidated at offsets [0x0..0x1] by a Unique Box retag
   |
   |
LL |     let boxed = Box::new_in([42u8; 1], &once_alloc);
   = note: BACKTRACE (of the first span):
   = note: BACKTRACE (of the first span):
   = note: inside `std::ptr::drop_in_place::<[std::mem::MaybeUninit<u8>]> - shim(None)` at /checkout/library/core/src/ptr/mod.rs:490:1: 490:56
   = note: inside `<std::vec::Vec<std::mem::MaybeUninit<u8>> as std::ops::Drop>::drop` at /checkout/library/alloc/src/vec/mod.rs:3054:13: 3054:91
   = note: inside `std::ptr::drop_in_place::<std::vec::Vec<std::mem::MaybeUninit<u8>>> - shim(Some(std::vec::Vec<std::mem::MaybeUninit<u8>>))` at /checkout/library/core/src/ptr/mod.rs:490:1: 490:56
  --> tests/pass/box-custom-alloc.rs:56:1
   |
LL | }
   | ^

[JakobDegen]

Can we just stop emitting ret-position noalias instead? I'm not fundamentally opposed to having semantics as suggested here, but I can't really claim to understand all the consequences of it and actually supporting allocators reasonably well needs a lot more work

[RalfJung]

I mean, we could, but I won't pursue that for now. This is obviously fine for safe code, and this PR is a way to learn how bad it is for unsafe code. I am not fundamentally opposed to rid Box of all its aliasing requirements, but that RFC needs to be written by someone else. I also think there should be a type for "Box without noalias", aka " NonNull with deallocation on drop", but making that type the same as Box leaves a lot of optimization potential on the table in safe code. We have references and raw pointers for people to make that choice themselves, it would make sense to likewise have a "raw box" type.

[RalfJung]

Ah, but this is a problem for boxes with custom allocators.

[saethlin]

I hope this doesn't break the tag cache in subtle ways
...
One odd thing that can now happen is that the "base tag" of an allocation might cease to be valid for that allocation. But I don't think we ever assumed that it would always remain valid

Yes, I think the new clear-on-return logic is correctly handled by the same code path that handles the creation of an unknown bottom. Bless whoever thought of factoring the code in this way instead of writing the base tag over the whole cache ;)

[saethlin]

that RFC needs to be written by someone else

Working on it, slowly. Keep mentioning this to fuel the fire. I'm hoping I'll shop around a pre-RFC at the end of January.

[RalfJung]

Can we just stop emitting ret-position noalias instead?

You got your wish. ;) This PR is superseded by #106371.