-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Kill moved locals in borrowed locals analysis #110420
Conversation
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
Unfortunately, we can't do this. Putting aside for a minute that post-drop elab Mir is not expected to pass borrowck (and doesn't), this is also wrong in other cases: async {
let x = String::new();
let p = &x as *const _;
let _y = x;
something.await;
dbg!(*p);
} There's a UCG issue tracking whether such code is UB or not, but we should not be assuming otherwise in the compiler |
Thanks for pointing this out.
Can you maybe elaborate on this, please (sorry not really that familiar with mir optimizations)?
Do you happen to have a link for that issue? Is this the one you're referring to (this doesn't seem to touch on whether the provided example is UB or not, but I couldn't find any other issue that seemed more relevant). I only skimmed your proposal, but it seems to me that something like a generator table would solve the 'future sizes' problem?! Alternatively (though I haven't had time to properly think this through): What if we were to track "live" borrows and raw pointers (and keep a borrow/raw ptr -> local map) in the dataflow analysis for borrowed locals? That should also provide sufficient information to let us infer whether to keep a local across a yield point I think... Is there a flaw in that thinking? |
rust-lang/unsafe-code-guidelines#188 is the issue.
There's nothing inherently flawed about this - doing more careful analysis lets us do more complicated optimizations. The difficulty is only in doing that analysis correctly. I'd invite you to share on Zulip if you have some concrete ideas for what you want to try |
Thanks for the answer, I'll ping you on zulip. I'll go ahead and close this PR. |
Stop considering moved-out locals when computing auto traits for generators Addresses rust-lang#94067 (but does not fix it since drop-tracking-mir is unstable) This PR, unlike rust-lang#110420 or rust-lang#112050, does not attempt to reduce the number of live locals across suspension points. It however ignores moved-out locals for trait purposes. So this PR solves the non-send issue, but not the size issue. Suggested by `@RalfJung` in [rust-lang/unsafe-code-guidelines#188](rust-lang/unsafe-code-guidelines#188 (comment)) cc `@b-naber` who's working on this from a different perspective. r? `@cjgillot`
We currently solely rely on
StorageDead
statements when killing locals in the borrowed locals analysis. Killing locals when they're moved should be fine since the borrow checker guarantees that there aren't any used borrows after the move. Currently this change only kills locals corresponding toPlace
s that don't have any projections, though we could in principle also track moves of projections here. Not sure whether this is worth it given that we will be move todrop-tracking
andmir-drop-tracking
at some point. Still, the current change seems like an improvement to me until then.Fixes #96084
Fixes #94067 (I think? I believe the example by @eholk was missing a
Send
impl onAgent
)r? @tmandry maybe?