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.

Sign up for GitHub

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

Jump to bottom

StorageLive of live local when building tokio with -Zcross-crate-inline-threshold=always + high MIR inlining threshold #117733

Closed
saethlin opened this issue Nov 9, 2023 · 1 comment · Fixed by #118797
Closed

StorageLive of live local when building tokio with -Zcross-crate-inline-threshold=always + high MIR inlining threshold #117733

saethlin opened this issue Nov 9, 2023 · 1 comment · Fixed by #118797
Labels
-Zvalidate-mir Unstable option: MIR validation A-coroutines Area: Coroutines A-mir-opt Area: MIR optimizations A-mir-opt-inlining Area: MIR inlining C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️

Comments

@saethlin
Copy link
Member

saethlin commented Nov 9, 2023

Reproduces with https://github.com/tokio-rs/tokio on commit 19d96c067410e30da29978be3513824645902a84

cd tokio/tokio
RUSTFLAGS="-Ztreat-err-as-bug -Zcross-crate-inline-threshold=always -Zinline-mir-hint-threshold=10000 -Zinline-mir-threshold=10000" cargo b --release --features sync

error: internal compiler error: broken MIR in Item(DefId(0:1070 ~ tokio[56a2]::sync::broadcast::{impl#14}::blocking_recv)) (after phase change to runtime-optimized) at bb140[16]:
                                StorageLive(_326) which already has storage here
    --> tokio/src/sync/broadcast.rs:1255:13
     |
1255 |         fut.await
     |             ^^^^^

Remove -Ztreat-err-as-bug and add -Zvalidate-mir to get a lot of ICEs like this:
rustc-ice-2023-11-09T01_05_17-2728619.txt
@saethlin saethlin added I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ C-bug Category: This is a bug. A-mir-opt Area: MIR optimizations A-mir-opt-inlining Area: MIR inlining labels Nov 9, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 9, 2023
@tmiasko tmiasko added A-coroutines Area: Coroutines and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 10, 2023
@tmiasko tmiasko mentioned this issue Nov 10, 2023
Closed
@RalfJung
Copy link
Member

So, looks like we should either allow StorageLive on an already live local, or we need to ensure that all locals are explicitly marked StorageDead before returning. No idea how realistic it is to achieve the latter.

Cc @cjgillot @matthewjasper

@tmiasko tmiasko self-assigned this Dec 9, 2023
@tmiasko tmiasko added the -Zvalidate-mir Unstable option: MIR validation label Dec 9, 2023
@tmiasko tmiasko mentioned this issue Dec 10, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 11, 2023
@matthiaskrgr
Rollup merge of rust-lang#118797 - tmiasko:dead-coro, r=davidtwco
9137927 
End locals' live range before suspending coroutine

State transforms retains storage statements for locals that are not
stored inside a coroutine. It ensures those locals are live when
resuming by inserting StorageLive as appropriate. It forgot to end the
storage of those locals when suspending, which is fixed here.

While the end of live range is implicit when executing return, it is
nevertheless useful for inliner which would otherwise extend the live
range beyond return.

Fixes rust-lang#117733
compiler-errors added a commit to compiler-errors/rust that referenced this issue Dec 12, 2023
@compiler-errors
Rollup merge of rust-lang#118797 - tmiasko:dead-coro, r=davidtwco
8f117dc 
End locals' live range before suspending coroutine

State transforms retains storage statements for locals that are not
stored inside a coroutine. It ensures those locals are live when
resuming by inserting StorageLive as appropriate. It forgot to end the
storage of those locals when suspending, which is fixed here.

While the end of live range is implicit when executing return, it is
nevertheless useful for inliner which would otherwise extend the live
range beyond return.

Fixes rust-lang#117733
@bors bors closed this as completed in b862e7e Dec 12, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 12, 2023
@rust-timer
Unrolled build for rust-lang#118797
f1d97ac 
Rollup merge of rust-lang#118797 - tmiasko:dead-coro, r=davidtwco

End locals' live range before suspending coroutine

State transforms retains storage statements for locals that are not
stored inside a coroutine. It ensures those locals are live when
resuming by inserting StorageLive as appropriate. It forgot to end the
storage of those locals when suspending, which is fixed here.

While the end of live range is implicit when executing return, it is
nevertheless useful for inliner which would otherwise extend the live
range beyond return.

Fixes rust-lang#117733
@tmiasko tmiasko removed their assignment Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
-Zvalidate-mir Unstable option: MIR validation A-coroutines Area: Coroutines A-mir-opt Area: MIR optimizations A-mir-opt-inlining Area: MIR inlining C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

End locals' live range before suspending coroutine tmiasko/rust
4 participants
@RalfJung @saethlin @rustbot @tmiasko