Skip to content
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

What about: StorageDead/StorageLive? #129

Closed
RalfJung opened this issue May 20, 2019 · 10 comments · Fixed by rust-lang/rust#126154
Closed

What about: StorageDead/StorageLive? #129

RalfJung opened this issue May 20, 2019 · 10 comments · Fixed by rust-lang/rust#126154
Labels
A-storage-liveness Topic: Related to storage liveness C-open-question Category: An open question that we should revisit S-pending-design Status: Resolving this issue requires addressing some open design questions

Comments

@RalfJung
Copy link
Member

RalfJung commented May 20, 2019

We should properly document the semantics of StroageDead/StorageLive. One interesting aspect here is that this includes documenting when they are emitted! In some sense, this is observable via UB, and as such subject to stabilization guarantees.

Related issues/posts:

@JakobDegen
Copy link
Contributor

Closing this issue. It's unclear what exactly this is tracking. The Mir semantics of storage statements are relatively fixed nowadays, which seems to be what this is mostly about.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 1, 2023

I think there are still some open questions:

@RalfJung RalfJung reopened this Aug 1, 2023
@RalfJung RalfJung added the S-pending-design Status: Resolving this issue requires addressing some open design questions label Aug 8, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Aug 9, 2023

@JakobDegen you had objections yesterday against claiming that this is pending-design, that I didn't fully understand.

I think one thing we have to do, at least, is decide whether rust-lang/rust#98896 is a bug in MIR building or not. We have some consumers preferring if it was a bug (Prusti, MiniRust), but it is very possible that MIR optimizations would prefer this to be allowed. I also don't know how hard it would be to have MIR building not generate double-free of locals; that could also be prohibitive.

@chorman0773
Copy link
Contributor

IDK about Jake, but my objection was that StorageDead/StorageLive generation in rustc MIR building is very much a rustc implementation detail, since how it's generated and it's rules are going to be very rustc-specific. Rustc could also change those rules as it sees fit.

If there's desire to translate MIR to MiniRust, the translation tool can handle that if the rules for rustc-mir-storagedead is different than the minirust-storagedead.

@JakobDegen
Copy link
Contributor

@chorman0773 I don't think that's quite correct. Where exactly in control flow locals are allocated and deallocated is visible to users by looking at addresses. Although the particular details of storage statements might be rustc internals (see below), their general existence and semantics is definitely not.

That being said, @RalfJung , my concern is that it's not clear to me that these need to be minirust concerns. ie, why does the minirust answer to your question need to be consisent with the rustc answer?

@RalfJung
Copy link
Member Author

IDK about Jake, but my objection was that StorageDead/StorageLive generation in rustc MIR building is very much a rustc implementation detail, since how it's generated and it's rules are going to be very rustc-specific. Rustc could also change those rules as it sees fit.

There is clearly something here that is not just an implementation detail. The following code has UB:

let x = {
  let y = 0;
  addr_of!(y)
};
x.read();

The reason this has UB, in my view, is that the lowering from surface Rust to the lower-level spec language (a la MiniRust) has a StorageDead(y) at the end of the scope of y. But where and when exactly are these StorageDead introduced? Clearly we need to spec this.

why does the minirust answer to your question need to be consisent with the rustc answer?

I guess it doesn't. Though so far MiniRust is very consistent with MIR (on the syntactic fragment they share), so any deviation needs justification IMO.

@tmiasko
Copy link

tmiasko commented Nov 10, 2023

Making StorageLive on already live local not legal is also problematic for inlining (cc rust-lang/rust#117733).

Consider a callee located in a loop, and which might return while some locals are still live. Inlining would have to insert code that executes StorageDead for locals that are live when callee returns.

Or alternatively it invites a further restriction that makes it not legal to return while there are locals that are still live.

@RalfJung
Copy link
Member Author

Or alternatively it invites a further restriction that makes it not legal to return while there are locals that are still live.

I had considered that for MiniRust, basically making everything fully explicit.

Good point about inlining though!

Personally I don't really care either way, I just don't like the current inconsistency where "StorageLive on live local" is forbidden but "StorageDead on dead local" is allowed. We do have some MIR consumers that would appreciate the strict semantics. I have no idea how hard it would be to get MIR building to always emit storage annotations in well-scoped pairs. Why does it currently sometimes have too many / too few StorageDead? Doesn't it have to have the scope information already anyway?

@tmiasko
Copy link

tmiasko commented Nov 20, 2023

Personally, I think all those should be legal:

  • StorageDead on already dead local
  • StorageLive on already live local
  • Return when they are still live locals

For the most part, most violations have been quite harmless. With restrictions they become critical issues.

Why does it currently sometimes have too many / too few StorageDead? Doesn't it have to have the scope information already anyway?

jieyouxu added a commit to jieyouxu/rust that referenced this issue Jun 19, 2024
…errors

StorageLive: refresh storage (instead of UB) when local is already live

Blocked on [this FCP](rust-lang#99160 (comment)), which also contains the motivation.

Fixes rust-lang#99160
Fixes rust-lang#98896 (by declaring it not-a-bug)
Fixes rust-lang#119366
Fixes rust-lang/unsafe-code-guidelines#129
fmease added a commit to fmease/rust that referenced this issue Jun 19, 2024
…errors

StorageLive: refresh storage (instead of UB) when local is already live

Blocked on [this FCP](rust-lang#99160 (comment)), which also contains the motivation.

Fixes rust-lang#99160
Fixes rust-lang#98896 (by declaring it not-a-bug)
Fixes rust-lang#119366
Fixes rust-lang/unsafe-code-guidelines#129
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 19, 2024
Rollup merge of rust-lang#126154 - RalfJung:storage-live, r=compiler-errors

StorageLive: refresh storage (instead of UB) when local is already live

Blocked on [this FCP](rust-lang#99160 (comment)), which also contains the motivation.

Fixes rust-lang#99160
Fixes rust-lang#98896 (by declaring it not-a-bug)
Fixes rust-lang#119366
Fixes rust-lang/unsafe-code-guidelines#129
@RalfJung
Copy link
Member Author

Resolved by rust-lang/rust#126154

github-actions bot pushed a commit to rust-lang/miri that referenced this issue Jun 21, 2024
StorageLive: refresh storage (instead of UB) when local is already live

Blocked on [this FCP](rust-lang/rust#99160 (comment)), which also contains the motivation.

Fixes rust-lang/rust#99160
Fixes rust-lang/rust#98896 (by declaring it not-a-bug)
Fixes rust-lang/rust#119366
Fixes rust-lang/unsafe-code-guidelines#129
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-storage-liveness Topic: Related to storage liveness C-open-question Category: An open question that we should revisit S-pending-design Status: Resolving this issue requires addressing some open design questions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants