-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Out of storage use of local for temporary caused by label break #104736
Comments
This regressed at some point in the last 90 days, so needs a bisect. Someone should also test this under cc @jsgf who found this |
searched nightlies: from nightly-2022-01-01 to nightly-2022-11-22 bisected with cargo-bisect-rustc v0.6.4Host triple: x86_64-unknown-linux-gnu cargo bisect-rustc --access github --script ./script So this was regressed by #103172 |
I've dug into this a bit. It turns out that the path this takes to an ICE is complicated and rather subtle, so I'm not going to explain it in this message (if you'd like to know, feel free to ping me on Zulip). Instead, I'll just explain how this can be fixed. Two options:
Code: #![feature(try_blocks)]
struct Response {
bookmarks: String,
continue_after: String,
}
#[inline(never)]
fn new_string() -> String {
String::new()
}
#[inline(never)]
fn def() -> Response {
Response {
bookmarks: String::new(),
continue_after: String::new(),
}
}
fn format_response(page: Result<String, String>) -> Result<Response, String> {
try {
Response {
bookmarks: new_string(),
continue_after: page?,
..def()
}
}
}
fn format_response_no_try(page: Result<String, String>) -> Result<Response, String> {
Ok(
Response {
bookmarks: new_string(),
continue_after: page?,
..def()
}
)
} Invocation on a stage 1 build of 80a9646:
Yields Despite the images looking different, the structure is actually quite similar. Basic blocks 6 plays the same role in both versions of the function. Basic blocks 17 in What we're interested in is the unwind paths. Bb 6 has the same unwind path (ie same sequence of locals dropped) in both versions of the function. However, bb 17/20 have different unwind paths. For some reason, bb17's unwind path in |
Worth noting also that I have no idea why this started to ICE as a result of Patrick's PR (maybe an extra MIR validation call for some reason?). In any case, the MIR building behavior does not seem to be a regression. It's been this way since at least 5c8bff7 . Unassigning myself because I have no time to dig into this |
I think I have enough context here to be able to @rustbot label +E-mentor +E-hard |
Seems that @rustbot claim |
@JakobDegen Actually can you explain why this ICEs? #![feature(try_blocks)]
struct Response {
bookmarks: String,
continue_after: String,
}
#[inline(never)]
fn def() -> Response {
Response {
bookmarks: String::new(),
continue_after: String::new(),
}
}
fn format_response(page: Result<String, String>) -> Result<Response, String> {
try {
Response {
continue_after: page?,
..def()
}
}
} In this program we also have a drop of the local |
@b-naber this might turn into quite an essay, but more time and shorter letters and all that. I think the better question isn't why that version doesn't ICE, but why the original version does. There are two important things to remember:
What this all means is there are two ways that we can get ICEs from code that is not a miscompilation. This pre-drop elaboration MIR will ICE in the validator: // _5 is not initialized here, so this drop does not actually execute at runtime,
// but still it is an ICE because of a use outside of storage liveness
Drop(_5);
StorageLive(_5);
_5 = something;
other_thing = _5;
StorageDead(_5); And similarly, this will ICE after drop elaboration: if false {
// This drop now does run "unconditionally" in the sense of there being no more
// initializedness check, but of course it does not *actually* run because of `if false`
Drop(_5);
}
StorageLive(_5);
_5 = something;
other_thing = _5;
StorageDead(_5); Let's take a look at how this comes up in the The bad drop that we're looking at here is now in bb 21. It is "reachable" via a path like 5 -> 6 -> 12 -> 21 although it should not be (that is the bug). However, this does not immediately ICE, because the validator sees that storage could be live there, via a path like 7 -> 8 -> 9 -> 10 -> 20 -> 21. Drop elaboration now runs and after some simplification, we get this ( The bad drop we care about is still there, at bb 17. We can also still find both the paths I mentioned above. The path on which the drop is reachable even though it shouldn't be is there in the form of 5 -> 6 -> 18 -> 17. Note that the What actually ends up setting off the ICE is inlining ( The MIR has changed a lot, but we can do our best to still keep track of all the things we identified above. The bad drop is now at bb 14. The If we try and find the other path we cared about, we run into trouble. bb7 has turned into bb6 with This is what sets off the ICE. The validator now detects that So why doesn't this happen in your example? Well, we can take a look ( |
Thank you for the great answer. I believe the cause of the bug is that we assign a wrong region scope to the try block during thir construction. We create some new I haven't implemented this yet, but have used a hack where in Does that fix sound correct to you? EDIT: Actually I think this might require a more nuanced solution, we only need the next outer breakable scope when we actually encounter any break statements, more specifically inside |
The CFG looks like what we should be generating, and what you said in the edit sounds vaguely correct. What would be really helpful for debugging this is if we had a way to get a proper tree-like view of the THIR, to determine if what we're generating there is what we expect. Unfortunately |
That approach does not work, since if we e.g. have multiple I'm fairly certain that there actually isn't anything wrong with either thir construction or the way we create drops and the unwind paths. I guess one could argue that we could shrink the lifetime scope of an unused "struct base" in thir construction, but I believe the proper fix here is to actually never construct the place for that base in the first place if all fields have been provided. If we only create this |
Unless I'm misunderstanding something, that can't work because this still ICEs: #![feature(try_blocks)]
struct Response {
bookmarks: String,
continue_after: String,
f: String,
}
#[inline(never)]
fn new_string() -> String {
String::new()
}
#[inline(never)]
fn def() -> Response {
Response {
bookmarks: String::new(),
continue_after: String::new(),
f: String::new(),
}
}
fn format_response(page: Result<String, String>) -> Result<Response, String> {
try {
Response {
bookmarks: new_string(),
continue_after: page?,
..def()
}
}
} |
You are right though, this is unrelated to try blocks. Still reproduces as: struct Response {
f: String,
}
#[inline(always)]
fn ident(a: Response) -> Response {
a
}
fn format_response(a: Response, b: Response, c: String, d: String, x: bool) -> Response {
'scope: {
ident(Response {
f: {
let _z = d;
if x {
c
} else {
break 'scope b;
}
},
..{ a }
})
}
}
|
@rustbot label -F-try_blocks |
I did a bunch more investigation and have updated the main comment with a much better reproducer. |
Thanks for investigating further, I'll try to look more into your updated problem statement (might take me a bit though since I'm not that familiar with mir optimization). What I find really weird though is that your first example and the new one only ICE with |
I don't think it's an optimization issue, it's still cleanly within MIR building.
If it's in a binary rustc realizes that all the functions are dead and doesn't bother optimizing them (hence no ICE). If you actually call the function and force it to get codegened the ICE is back: playground |
So I think the cause of this is the way in which we're caching the unwind drops, which is done here. If we ever encounter a scope corresponding to the scope of a label in this iteration: Later when building the exit tree for the break scope we call The problem is that those drops have a lifetime scope greater than the scope of the break scope (because they appear in the expression of the label block). I can't think of a fix that would fit into the current design, we would need some way to track which drops follow a certain break statement I would imagine and based on that decide whether to cache a drop for the break scope. I haven't had time to properly think this through though. |
Following #107270 the example now needs |
Updated stacktrace for findability #104736 (comment)
|
Rollup merge of rust-lang#119077 - tmiasko:lint, r=cjgillot Separate MIR lints from validation Add a MIR lint pass, enabled with -Zlint-mir, which identifies undefined or likely erroneous behaviour. The initial implementation mostly migrates existing checks of this nature from MIR validator, where they did not belong (those checks have false positives and there is nothing inherently invalid about MIR with undefined behaviour). Fixes rust-lang#104736 Fixes rust-lang#104843 Fixes rust-lang#116079 Fixes rust-lang#116736 Fixes rust-lang#118990
Code
Playground. Needs release
Meta
Version: 0d5573e and all other recent versions
Explanation
This causes an ICE in the MIR validator. The reason is an out of storage use of the local that is the temporary for
{ a }
. This local has a couple interesting properties. The most important of these is that MIR building thinks that it may need to be dropped while unwinding in two scenarios:_z
unwinds.no_unwind()
unwinds.Neither of these are actually possible.
First, drop elaboration comes along, sees that only the second of these is possible. It reacts to this by inserting a drop flag for the local. Then, inlining comes along and inlines
no_unwind()
. The potential unwind edge is now gone.The result is that the cleanup drop is actually completely unreachable, and this ends up causing an ICE.
Curiously, this does not reproduce if one replaces
break 'scope nothing;
withreturn nothing;
. In the case of the return, MIR building correctly realizes that the temporary has not been created yet and hence does not need to be dropped. The best solution to this is likely to make thebreak
do the same thing - I'm not sure how difficult that is.The text was updated successfully, but these errors were encountered: