Resolve the interaction between StorageDead, drops, and unwinding

[cramertj]

#60840 introduces the expectation that once drop runs on a local, it is UB for that local to be accessed, regardless of the success or failure (panic) of the drop. Unfortunately, adding extra StorageDeads for all of these values caused a performance regression, so at the moment we don't do this. #60840 instead uses drop to indicate that a value is implicitly StorageDead. However, this interacts poorly with drop elaboration, which can turn a drop of a structure into drops of its fields.

cc @Zoxc @tmandry @RalfJung

[tmandry]

Also, for locals which are not Drop, #60840 adds StorageDead, but only on generator bodies. Adding it for all functions would be a big performance regression.

Technically, we could also emit StorageDead for locals which are Drop in generators, but it would have been slower (unclear how much) and would have required more significant changes to the MIR generation code.

[RalfJung]

To repeat what I said in the other thread:

This changes the semantics of Drop to implicitly have a StorageDead in both the return and the unwind edge. If we decide to go with this approach, please open an issue in Miri so that we can implement this in Miri's semantics. It would also be helpful if you could come up with some example code that should be flagged as UB.

I have a few questions about this:

• Drop works on an arbitrary place, StorageDead works on a local. I suppose the new semantics only occurs when the place is exactly a local, and does not involve any projections or so?

• In early passes of MIR construction (before drop flag elaboration), Drop is actually a conditional drop. Does the StorageDead only occur if the Drop happens, or does it always occur? Are we sure that drop flag elaboration is semantics-preserving under whatever choice we make here (and document, I hope)?

• What about DropAndReplace?

However, this interacts poorly with drop elaboration, which can turn a drop of a structure into drops of its fields.

To elaborate, the problem here is that this removes a StorageDead, and it is not clear that that is a correct program transformation.

[RalfJung]

Cc @rust-lang/wg-unsafe-code-guidelines

[RalfJung]

Regarding DropAndReplace, @eddyb writes

Also, I thought DropAndReplace expanded to a Drop and an assignment, wouldn't this make that assignment UB?

@tmandry that's why I asked about DropAndReplace above. What is your analysis doing for them? Also can someone reading this show an example where DropAndReplace gets used? Miri never sees them AFAIK, so they presumably get lowered away to something else at some point.

[eddyb]

@RalfJung Any potentially-drop-needing source-level assignment of an already-initialized place should result in a DropAndReplace - e.g. let mut s = String::new(); s = String::from("foo");.
AFAIK (but @arielb1 / @pnkfelix might know better), the DropAndReplace is rewritten in drop elaboration to a Drop (of the previous value) followed by an Assign (of the new value).

[eddyb]

Oh, also, #61060 is open now (and @RalfJung already left a comment on it: #61060 (comment)).

[RalfJung]

Any potentially-drop-needing source-level assignment of an already-initialized place should result in a DropAndReplace - e.g. let mut s = String::new(); s = String::from("foo");.
AFAIK (but @arielb1 / @pnkfelix might know better), the DropAndReplace is rewritten in drop elaboration to a Drop (of the previous value) followed by an Assign (of the new value).

I see, thanks. I guess we do guarantee that this keeps the address stable? Seems strange not to. And indeed see this example for a function where making the Drop implicitly deallocate the storage would be rather fatal!

In that case, a drop generated from a DropAndReplace should not be considered as implicitly doing StorageDead, it seems. So maybe the Drop terminator should carry a bool indicating whether the storage is doing to be reused, and @tmandry's analysis only looks for those that say it will not get reused?

[eddyb]

I talked some more with @tmandry and it seems like this entire direction arose from the assumption that it would be easier than inserting StorageDeads on the unwind path.
But StorageDead in unwind (aka "cleanup") blocks is much easier to ignore (e.g. in LLVM codegen), if necessary, compared to ignoring the extra semantics attached to Drops when they don't apply.

So I think we should just insert StorageDead in the cleanup path as well.

[RalfJung]

I'm certainly in favor of explicit StorageDead over implicit side-effects. :)

[arielb1]

I also think that a Drop should be just-a-Drop and you'll need a StorageDead to be able to recycle the address.