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

MIR inlining does not handle SourceScopeLocalData #66137

Closed
Aaron1011 opened this issue Nov 5, 2019 · 7 comments
Closed

MIR inlining does not handle SourceScopeLocalData #66137

Aaron1011 opened this issue Nov 5, 2019 · 7 comments
Assignees
@Aaron1011
Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Nov 5, 2019 •
edited by rustbot
Loading

During the MIR inlining pass, we add scopes from the callee to Body.source_scopes in the caller. However, we do not update source_scope_local_data, which causes any inlined scopes to have no entry in the caller source_scope_local_data. This causes an ICE in Miri when we try to generate a stacktrace - however, the ICE occurs in rustc code, so it's possible that we might run across this without Miri.
@Aaron1011 Aaron1011 changed the title MIR inlinging does not handle SourceScopeLocalData MIR inlining does not handle SourceScopeLocalData Nov 5, 2019
@Aaron1011 Aaron1011 mentioned this issue Nov 5, 2019
Closed
@jonas-schievink jonas-schievink added A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 5, 2019
@RalfJung RalfJung added the A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) label Nov 6, 2019
@RalfJung RalfJung mentioned this issue Nov 6, 2019
Closed
2 tasks
@eddyb eddyb added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Nov 6, 2019
@eddyb
Copy link
Member

eddyb commented Nov 6, 2019

Sounds like a simple enough omission, but I'm surprised this doesn't break any of our mir-opt tests.

@Aaron1011
Copy link
Member Author

I don't think this is E-Easy - source_scope_local_data is crate local, and it's not immediately obvious to me what the correct beehvaior is for cross-crate inlining.

@Aaron1011 Aaron1011 mentioned this issue Nov 8, 2019
Closed
@Dylan-DPC-zz
Copy link

@rustbot assign @Aaron1011

Aaron1011 added a commit to Aaron1011/rust that referenced this issue Nov 13, 2019
@Aaron1011
Fix generating CTFE backtrace on optimized MIR
dadd817 
During MIR optimization, we may inline function calls acrross crates.
While we can inline the corresponding scopes into `Body.source_scopes`,
we cannot inline the corresponding data from `source_scope_local_data`,
since it references crate-local data.

This commit makes us fall back to the root lint level when generating a
CTFE backtrace, if it was not possible to find crate-local data for a
scope in `source_scope_local_data`.

Fixes rust-lang#66137
@eddyb
Copy link
Member

eddyb commented Nov 13, 2019

@Aaron1011 Oh, I thought the concern was entirely about same-crate, sorry!

I think the fix in that case is to move the ClearCrossCrate inward, i.e. change SourceScopeData to contain a ClearCrossCrate<SourceScopeLocalData> - I don't think the size of SourceScopeData is critical (but we should run perf anyway).

@Aaron1011
Copy link
Member Author

@eddyb: SourceScopeData doesn't contain SourceScopeLocalData. Your change would require wrapping both fields of SourceScopeLocalData in ClearCrossCrate, since SourceScopeLocalData is stored in an IndexVec in Body.source_scope_local_data

@Aaron1011
Copy link
Member Author

Oops, I misread your proposal - you were talking about changing SourceScopeData to contain a SourceScopeLocalData. I'll try that.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 29, 2019

This was fixed in #66789 as far as I can tell

@oli-obk oli-obk closed this as completed Dec 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Aaron1011 Aaron1011

Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix generating CTFE backtrace on optimized MIR Aaron1011/rust
6 participants
@eddyb @RalfJung @oli-obk @Aaron1011 @jonas-schievink @Dylan-DPC-zz