-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify MIR semantics of storage statements #99050
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -237,19 +237,19 @@ pub enum StatementKind<'tcx> { | |
|
||
/// `StorageLive` and `StorageDead` statements mark the live range of a local. | ||
/// | ||
/// Using a local before a `StorageLive` or after a `StorageDead` is not well-formed. These | ||
/// statements are not required. If the entire MIR body contains no `StorageLive`/`StorageDead` | ||
/// statements for a particular local, the local is always considered live. | ||
/// | ||
/// More precisely, the MIR validator currently does a `MaybeStorageLiveLocals` analysis to | ||
/// check validity of each use of a local. I believe this is equivalent to requiring for every | ||
/// use of a local, there exist at least one path from the root to that use that contains a | ||
/// `StorageLive` more recently than a `StorageDead`. | ||
/// | ||
/// **Needs clarification**: Is it permitted to have two `StorageLive`s without an intervening | ||
/// `StorageDead`? Two `StorageDead`s without an intervening `StorageLive`? LLVM says poison, | ||
/// yes. If the answer to any of these is "no," is breaking that rule UB or is it an error to | ||
/// have a path in the CFG that might do this? | ||
/// At any point during the execution of a function, each local is either allocated or | ||
/// unallocated. Except as noted below, all locals except function parameters are initially | ||
/// unallocated. `StorageLive` statements cause memory to be allocated for the local while | ||
/// `StorageDead` statements cause the memory to be freed. Using a local in any way (not only | ||
/// reading/writing from it) while it is unallocated is UB. | ||
/// | ||
/// Some locals have no `StorageLive` or `StorageDead` statements within the entire MIR body. | ||
/// These locals are implicitly allocated for the full duration of the function. There is a | ||
/// convenience method at `rustc_mir_dataflow::storage::always_storage_live_locals` for | ||
/// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Recently reported #98896 indicates this is still an issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was actually going to suggest that we make the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would you suggest translating StorageLive to LLVM ir if double StorageLive isn't UB? I believe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That should be UB in MIR too - the semantics I am suggesting for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still we should get confirmation from LLVM that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
https://llvm.org/docs/LangRef.html#llvm-lifetime-start-intrinsic Seems like they already do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I didn't see the "already alive" part. Good point. And for
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's discuss this in a new issue: #99160 |
||
StorageLive(Local), | ||
|
||
/// See `StorageLive` above. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(probably too late for this PR though, it's already rolled up)