Fix the invalidation of the MIR early exit cache #35751
Conversation
|
I'm not seeing any invalidation at all for |
|
@nagisa Oops, I see. I would've expected |
|
@bors r+ |
|
📌 Commit e1749af has been approved by |
|
I would prefer to keep the cache through. |
nagisa
force-pushed
the
mir-scope-fix-again
branch
2 times, most recently
from
85c4e5d to
44955c7
Compare
|
Cache invalidation fixed. |
eddyb
changed the title
| // will stay correct, because the already generated unwind blocks cannot be influenced | ||
| // by just added drop. | ||
| let this_scope = scope.extent == extent; | ||
| scope.invalidate_cache(!this_scope); |
There was a problem hiding this comment.
This will invalidate the unwind cache in cases other than DropKind::Value, which is undesirable (as StorageDead doesn't end up on the unwind path at all).
eddyb
added
the
beta-nominated
nagisa
force-pushed
the
mir-scope-fix-again
branch
3 times, most recently
from
112a3e1 to
9983aef
Compare
| // If we’re scheduling cleanup for non-droppable type (i.e. DropKind::Storage), then we | ||
| // do not need to invalidate unwind branch, because DropKind::Storage does not end up | ||
| // built in the unwind branch currently. | ||
| let invalidate_unwind = needs_drop || !this_scope; |
nagisa
force-pushed
the
mir-scope-fix-again
branch
from
9983aef to
2d36642
Compare
|
@bors r+ |
|
📌 Commit 2d36642 has been approved by |
| if scope.extent == extent { | ||
| let this_scope = scope.extent == extent; | ||
| // We must invalidate all the caches leading up to the scope we’re looking for, because | ||
| // the cached blocks will branch into build of scope not containing the new drop. If we |
There was a problem hiding this comment.
I have a hard time following this comment, I'm afraid. Ideal would be a diagram, but maybe we can just tweak the wording...I don't quite know how since I don't understand it yet :)
There was a problem hiding this comment.
I’ll try to draw something up.
nagisa
force-pushed
the
mir-scope-fix-again
branch
from
e08d0cd to
2c3250a
Compare
|
@bors r+ |
|
📌 Commit 2c3250a has been approved by |
Fix the invalidation of the MIR early exit cache ~~The rust-lang#34307 introduced a cache for early exits in order to help with O(n*m) explosion of cleanup blocks but the cache is invalidated incorrectly and I can’t seem to figure out why (caching is hard!)~~ ~~Remove the cache for now to fix the immediate correctness issue and worry about the performance later.~~ Cache invalidation got fixed. Fixes rust-lang#35737 r? @nikomatsakis
nikomatsakis
added
the
beta-accepted
|
Accepting for beta: this was a serious correctness regression. |
brson
removed
the
beta-nominated
The #34307 introduced a cache for early exits in order to help with O(n*m) explosion of cleanup blocks but the cache is invalidated incorrectly and I can’t seem to figure out why (caching is hard!)Remove the cache for now to fix the immediate correctness issue and worry about the performance later.Cache invalidation got fixed.
Fixes #35737
r? @nikomatsakis