Clarify MIR semantics of storage statements #99050
Conversation
rustbot
added
the
T-compiler
|
r? @cjgillot (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Hmm, rustbot seems to have a better sleep schedule than I do. Manually tagging @RalfJung @oli-obk @davidtwco and @celinval for the MIR semantics change. Also going to proactively rename the function in the docs so that it'll be correct once #99022 lands. |
|
Thanks! @bors r+ rollup |
bors
added
S-waiting-on-bors
Clarify MIR semantics of storage statements Seems worthwhile to start closing out some of the less controversial open questions about MIR semantics. Hopefully this is fairly non-controversial - it's what we implement already, and I see no reason to do anything more restrictive. cc `@tmiasko` who commented on this when it was discussed in the original PR that added these docs.
| /// computing these locals. | ||
| /// | ||
| /// If the local is already allocated, calling `StorageLive` again is UB. However, for an | ||
| /// unallocated local an additional `StorageDead` all is simply a nop. |
There was a problem hiding this comment.
| /// unallocated local an additional `StorageDead` all is simply a nop. | |
| /// unallocated local an additional `StorageDead` call is simply a nop. |
(probably too late for this PR though, it's already rolled up)
| /// computing these locals. | ||
| /// | ||
| /// If the local is already allocated, calling `StorageLive` again is UB. However, for an | ||
| /// unallocated local an additional `StorageDead` all is simply a nop. |
There was a problem hiding this comment.
I wonder if we should consider double-free to also be UB, just for consistency? The only reason I allow this is that we do generate redundant StorageDead in MIR currently. (Or at least we did last time I checked this.)
MiniRust says double-dead is UB: I think I'd prefer that given that double-free of a heap allocation is also UB.
There was a problem hiding this comment.
Recently reported #98896 indicates this is still an issue.
There was a problem hiding this comment.
I was actually going to suggest that we make the StorageLive version of this not be UB either. I don't see what we gain from adding the UB
There was a problem hiding this comment.
How would you suggest translating StorageLive to LLVM ir if double StorageLive isn't UB? I believe StorageLive(_1); _1 = 42; StorageLive(_1); _2 = _1; is UB in LLVM ir.
There was a problem hiding this comment.
That should be UB in MIR too - the semantics I am suggesting for StorageLive are that calling it when the local is already live is allowed but still fully reallocates the local
There was a problem hiding this comment.
Still we should get confirmation from LLVM that lifetime.start will not have any effects that flow "backwards" in the execution and make preceding accesses UB -- ideally, to explicitly state in their docs "doing this on an already live local is fine (but reset its contents to be uninitialized)".
There was a problem hiding this comment.
If ptr is a non-stack-allocated object, it does not point to the first byte of the object or it is a stack object that is already alive, it simply fills all bytes of the object with poison.
https://llvm.org/docs/LangRef.html#llvm-lifetime-start-intrinsic
Seems like they already do.
There was a problem hiding this comment.
Oh, I didn't see the "already alive" part. Good point. And for lifetime.end it says
Calling llvm.lifetime.end on an already dead alloca is no-op.
So yeah this removes my main motivation for making redundant annotations UB in the first place. That means I do not have a strong opinion either way. I don't know if there are any good motivations for doing one or the other, or any optimizations/transformations that would be helped by either choice.
There was a problem hiding this comment.
Let's discuss this in a new issue: #99160
…askrgr Rollup of 3 pull requests Successful merges: - rust-lang#99022 (MIR dataflow: Rename function to `always_storage_live_locals`) - rust-lang#99050 (Clarify MIR semantics of storage statements) - rust-lang#99067 (Intra-doc-link-ify reference to Clone::clone_from) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Seems worthwhile to start closing out some of the less controversial open questions about MIR semantics. Hopefully this is fairly non-controversial - it's what we implement already, and I see no reason to do anything more restrictive. cc @tmiasko who commented on this when it was discussed in the original PR that added these docs.