Make MIR drop terminators borrow the dropped location

[Zoxc]

r? @eddyb
cc @tmandry

[Centril]

@bors rollup

[Centril]

cc #51114 #53667

[Centril]

Use if let?

[eddyb]

r? @pnkfelix cc @nikomatsakis

[Zoxc]

HaveBeenBorrowedLocals is used to find when references and raw pointers to a local can exist and is only used for immovable generators when computing liveness of locals. User code may capture pointers to locals with Drop impls and access them using unsafe code.

[eddyb]

@Zoxc Isn't it UB to capture self in a Drop impl? cc @rust-lang/wg-unsafe-code-guidelines

[RalfJung]

@eddyb I don't understand the question. What is "capturing self"? Do you have some example code?

[eddyb]

@RalfJung I mean, I'm assuming @Zoxc meant something like this:

struct Foo<T>(T, &mut *const T);
impl<T> Drop for Foo<T> {
    fn drop(&mut self) {
        *self.1 = &self.0 as *const T;
    }
}

[RalfJung]

I don't see any grounds for making this UB. However, if the caller does a StorageDead or otherwise deallocates the backing storage of the drop, then ever using that pointer to access memory is UB (as part of the normal rule that accessing a dangling pointer is UB).

[RalfJung]

Also, why the question? What is the analysis/optimization that would benefit from that code being UB? Honestly I have no idea what this PR even does, drop already borrows the dropped location because it receives an &mut, does it not?

[eddyb]

@RalfJung Well, the issue is that in MIR it's Drop(Place), i.e. drop(x) not drop(&mut x).
So there's no explicit borrow involved, and this PR takes into account the implicit borrow.
(A borrow would technically be wrong anyway, without it being &move / &own)

I guess we shouldn't just assume it's UB, just like keeping a raw pointer to a moved-from location?

It does throw a wrench into my plan of "assume borrows are cancelled by moves/drops" to limit the effects of borrows, but at least for some optimizations it all works out if there are no derefs/calls between Move(x)/Drop(x) and StorageDead(x) (since then any raw pointers couldn't have been used anyway).

[RalfJung]

this PR takes into account the implicit borrow

Takes into account for what? The borrow checker?

"assume borrows are cancelled by moves/drops"

Yeah, that is hard to realize. Even if we make moves write Undef in the storage they come from (which would be annoying to do in Miri because then reads need mutable access to the abstract machine memory), pointers to that storage would still be live and could be written to.

[eddyb]

Just because I got a bit confused and thought this PR might not have any effect, here's an example:

let mut x = foo();
x = bar(); // DropAndReplace
drop(x);
yield;
baz(); // may access `x` via raw pointers, `x` must be in generator state
// no Drop at the end of scope, only StorageDead

Note that without the drop(x) you'd have an end-of-scope Drop which I would expect to keep x in the generator state across the yield point, even without this PR.

And I think that for regular end-of-scope Drops, that are followed by StorageDead without any intervening yields, this PR has no effect, so it's only Drops from DropAndReplace (assuming drop elaboration ran already) that will cause more locals to be kept in the generator state.

cc @cramertj @tmandry How can we measure the impact this has on async fn generators?
It may result in bloat, in certain specific conditions (which could be rare but I'm not sure).

[tmandry]

Maybe I'm similarly confused, but we primarily use liveness_of_locals to determine which locals to save, and that already says it takes drops into account. Assuming that's true I actually don't think this would have an impact today.

That being said, it makes me nervous that we allow such things like capturing the pointer to a local inside a drop impl and later using it. Like if I were writing unsafe code, I wouldn't expect this to be allowed, at least without consulting a reference of some sort. It sounds like I've missed some previous discussions on the topic, but keeping our options open for future optimizations is important if we want to deliver on Rust's promises about performance.

[RalfJung]

keeping our options open for future optimizations is important if we want to deliver on Rust's promises about performance.

At the same time, keeping our semantics concrete and operational is important if we want it to be possible for mere mortals to write correct unsafe code.

Right now, I consider drop to be just another function call. There's nothing special about it. drop(place) (in the MIR) as a terminator is equivalent to _tmp = &mut place; Drop::drop(_tmp). That's an easy to implement, easy to remember, easy to reason about rule.

"pointers may not escape from drop" is an axiom, a wish that you say you want to be true. To make things worse, "escape" is a rather fuzzy notion. To make that wish come true we have to figure out what exactly "escape" means, and we have to add clauses to our semantics that say, in a concrete operational way, where the UB here comes from. Think of implementing this as a check in Miri.

To give one example of what "escape" can not mean: it cannot mean "the address is not written to any memory outside the drop stack frame". It is legal today to write safe code in drop that calls rayon::join to send the &mut self of drop temporarily to another thread.

I also don't see the benefit in terms of optimization: if the compiler is supposed to know that some memory is not reachable any more through pointers "escaped" into somewhere, it should deallocate that memory (e.g. using StorageDead). Alternatively we might want to argue based on aliasing and provenance using Stacked Borrows. Which extra power is this clause you are suggesting giving?

[pnkfelix]

Okay, after some puzzling and re-reading the dialogue here, I think I understand what this PR is doing.

The important bit is what eddyb said up above; in terms of runtime semantics, the MIR operator drop(local) really behaves as _tmp = &mut place; Drop::drop(_tmp); (or if you prefer, _tmp = &move place; Drop::drop(_tmp)). And that's important, because that means that drop(local) has an implicit borrow of local that is not reflected in a MIR (and thus is not exposed by the visit code's traversal over the MIR that the borrowed-locals code is otherwise using to identify the borrows).

• Maybe that is a footgun we should think about trying to address, in terms of the design of MIR itself or in the design of the mir::visit code.

[pnkfelix]

@tmandry wrote:

That being said, it makes me nervous that we allow such things like capturing the pointer to a local inside a drop impl and later using it

Well, at this point we have set down our drop order for e.g. struct fields.

So one can imagine examples like this, which I think should be valid (play):

use std::fmt::Debug;

struct D<A: Debug>(*const A);
struct Two<A: Debug> { d: D<A>, a: A }

impl<A: Debug> Drop for Two<A> {
    fn drop(&mut self) {
        println!("Dropping Two");
        self.d.0 = &mut self.a as *const A;
    }
}
    
impl<A: Debug> Drop for D<A> {
    fn drop(&mut self) {
        let a: A = unsafe { std::ptr::read(self.0) };
        println!("Dropping D({:?})", a);
        std::mem::forget(a);
    }
}

fn main() {
    let t = Two { d: D(std::ptr::null()), a: format!("Hello") };
}

and examples like this, which I think are UB (play):

use std::fmt::Debug;

struct D<A: Debug>(*const A);
struct Two<A: Debug> { a: A, d: D<A> }

impl<A: Debug> Drop for Two<A> {
    fn drop(&mut self) {
        println!("Dropping Two");
        self.d.0 = &mut self.a as *const A;
    }
}
    
impl<A: Debug> Drop for D<A> {
    fn drop(&mut self) {
        let a: A = unsafe { std::ptr::read(self.0) };
        println!("Dropping D({:?})", a);
        std::mem::forget(a);
    }
}

fn main() {
    let t = Two { d: D(std::ptr::null()), a: format!("Hello") };
}

(Note that the only difference between those two examples is the order of the struct field declarations in struct Two. When d comes before a lexically, as in the first case, that means d will be dropped before a, and so the unsafe pointer it carries will remain valid long enough for the destructor for d to run successfully. When d comes after a, then a is dropped before d's destructor runs, and we get haywire.)

[tmandry]

Okay, what I'm gathering from this discussion is that we can use StorageDead to get the semantics that I really want. I'm working on a PR that emits StorageDead after every drop. Unfortunately, it can't be used in functions that aren't generators due to the compile time performance hit of doing this, but I guess that's another matter.

The important bit is what eddyb said up above; in terms of runtime semantics, the MIR operator drop(local) really behaves as _tmp = &mut place; Drop::drop(_tmp); (or if you prefer, _tmp = &move place; Drop::drop(_tmp)). And that's important, because that means that drop(local) has an implicit borrow of local that is not reflected in a MIR (and thus is not exposed by the visit code's traversal over the MIR that the borrowed-locals code is otherwise using to identify the borrows).

Semantically I agree with this change then. I don't think it changes anything we do today, but it is more correct. I think this reasoning should be laid out a bit more explicitly in comments, since a few of us were confused by what the change was doing.

[eddyb]

Unfortunately, it can't be used in functions that aren't generators due to the compile time performance hit of doing this, but I guess that's another matter.

Just to be clear, there's a significant hit from StorageDeads added only to the unwind path?
Are you suppressing the codegen of llvm.lifetime.end calls in the unwind path? If not, I'd try it.

[tmandry]

Just to be clear, there's a significant hit from StorageDeads added only to the unwind path?
Are you suppressing the codegen of llvm.lifetime.end calls in the unwind path? If not, I'd try it.

Yes; some benchmark results are here. Keep in mind that this does cause new basic blocks to be created. I have not tried suppressing those in the codegen yet.

[RalfJung]

I'm working on a PR that emits StorageDead after every drop.

Just to be clear, this would happen during MIR construction when we know that this is really a "final" drop because something went out of scope, and not a "drop-and-replace", right?

[tmandry]

Just to be clear, this would happen during MIR construction when we know that this is really a "final" drop because something went out of scope, and not a "drop-and-replace", right?

Yep.

[pnkfelix]

@tmandry are you saying I should close this PR and wait for you to implement the semantics you actually desire?

As far as I can tell, this PR is putting in a fix, though its not 100% clear to me whether the fix is actually observable. (The lack of tests in the PR may be a reason to r- for the short term...)

[tmandry]

@pnkfelix No sorry, I agree with your conclusion that this PR does implement a fix, which probably isn't observable today (but should still be merged IMO). My work doesn't change that.

Sorry for throwing the discussion off track a bit, I was just trying to clarify my own understanding.

[pnkfelix]

@bors r+

[bors]

📌 Commit cb6beef has been approved by pnkfelix