Skip to content

Fix generating CTFE backtrace on optimized MIR#66203

Closed
Aaron1011 wants to merge 1 commit intorust-lang:masterfrom
Aaron1011:fix/miri-backtrace-opt
Closed

Fix generating CTFE backtrace on optimized MIR#66203
Aaron1011 wants to merge 1 commit intorust-lang:masterfrom
Aaron1011:fix/miri-backtrace-opt

Conversation

@Aaron1011
Copy link
Contributor

@Aaron1011 Aaron1011 commented Nov 8, 2019

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 #66137

@rust-highfive
Copy link
Contributor

rust-highfive commented Nov 8, 2019

r? @zackmdavis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 8, 2019
@bors
Copy link
Collaborator

bors commented Nov 13, 2019

☔ The latest upstream changes (presumably #60026) made this pull request unmergeable. Please resolve the merge conflicts.

@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
@Aaron1011 Aaron1011 force-pushed the fix/miri-backtrace-opt branch from 4a9908c to dadd817 Compare November 13, 2019 02:20
bjorn3
bjorn3 reviewed Nov 13, 2019
View reviewed changes
src/librustc/mir/mod.rs
/// Crate-local information for each source scope, that can't (and
/// needn't) be tracked across crates.
///
/// Before optimizations run, every scope in `source_scopes` is guarnateed
Copy link
Member

@bjorn3 bjorn3 Nov 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*guaranteed

@eddyb
Copy link
Member

eddyb commented Nov 13, 2019

I've proposed an alternative in #66137 (comment), FWIW.

@joelpalmer
Copy link

joelpalmer commented Nov 18, 2019

Ping from Triage: any update @bjorn3? @zackmdavis @Aaron1011

@Aaron1011
Copy link
Contributor Author

Aaron1011 commented Nov 18, 2019

I'm implementing the change suggested by @eddyb

@Aaron1011
Copy link
Contributor Author

Aaron1011 commented Nov 18, 2019

@eddyb: Your proposed change results in a large amount of churn across several files. In particular, it complicates code that initially creates the SourceScopeLocalData.

I think it's simpler to just adjust the only code that needs to use source_scope_local_data after optimizations.

@eddyb
Copy link
Member

eddyb commented Nov 19, 2019

I think it's simpler to just adjust the only code that needs to use source_scope_local_data after optimizations.

I'm not sure it's possible to have correct source_scope_local_data after inlining because they're either not preserved at all right now (bad) or their indices get mismatched (worse), as there is no way to represent a missing entry.

You'd at the very least need to put an Option around the element type of source_scope_local_data, at which point it feels like my proposal but more contrived. I can open a separate PR with my proposal, I want to see how much churn there is.

@Aaron1011 Aaron1011 mentioned this pull request Nov 23, 2019
Closed
@JohnCSimon
Copy link
Member

JohnCSimon commented Nov 24, 2019

Ping from triage: @Aaron1011 can you address the comments from @eddyb ? Thanks

@eddyb eddyb mentioned this pull request Nov 26, 2019
Merged
@eddyb
Copy link
Member

eddyb commented Nov 26, 2019

I can open a separate PR with my proposal, I want to see how much churn there is.

One week later: #66789.

@JohnCSimon This was actually waiting for me, sorry.

RalfJung added a commit to RalfJung/rust that referenced this pull request Nov 29, 2019
@RalfJung
Rollup merge of rust-lang#66789 - eddyb:mir-source-scope-local-data, …
b3d835f 
…r=oli-obk

rustc: move mir::SourceScopeLocalData to a field of SourceScopeData.

By having one `ClearCrossCrate<SourceScopeLocalData>` for each scope, as opposed to a single `ClearCrossCrate` for all the `SourceScopeLocalData`s, we can represent the fact that some scopes have `SourceScopeLocalData` associated with them, and some don't.

This is useful when doing MIR inlining across crates, because the `ClearCrossCrate` will be `Clear` for the cross-crate MIR scopes and `Set` for the local ones.

Also see rust-lang#66203 (comment) for some context around this approach.

Fixes rust-lang#51314.
Centril added a commit to Centril/rust that referenced this pull request Dec 1, 2019
@Centril
Rollup merge of rust-lang#66789 - eddyb:mir-source-scope-local-data, …
455fdb4 
…r=oli-obk

rustc: move mir::SourceScopeLocalData to a field of SourceScopeData.

By having one `ClearCrossCrate<SourceScopeLocalData>` for each scope, as opposed to a single `ClearCrossCrate` for all the `SourceScopeLocalData`s, we can represent the fact that some scopes have `SourceScopeLocalData` associated with them, and some don't.

This is useful when doing MIR inlining across crates, because the `ClearCrossCrate` will be `Clear` for the cross-crate MIR scopes and `Set` for the local ones.

Also see rust-lang#66203 (comment) for some context around this approach.

Fixes rust-lang#51314.
Centril added a commit to Centril/rust that referenced this pull request Dec 2, 2019
@Centril
Rollup merge of rust-lang#66789 - eddyb:mir-source-scope-local-data, …
fd09fad 
…r=oli-obk

rustc: move mir::SourceScopeLocalData to a field of SourceScopeData.

By having one `ClearCrossCrate<SourceScopeLocalData>` for each scope, as opposed to a single `ClearCrossCrate` for all the `SourceScopeLocalData`s, we can represent the fact that some scopes have `SourceScopeLocalData` associated with them, and some don't.

This is useful when doing MIR inlining across crates, because the `ClearCrossCrate` will be `Clear` for the cross-crate MIR scopes and `Set` for the local ones.

Also see rust-lang#66203 (comment) for some context around this approach.

Fixes rust-lang#51314.
@bors
Copy link
Collaborator

bors commented Dec 2, 2019

☔ The latest upstream changes (presumably #66944) made this pull request unmergeable. Please resolve the merge conflicts.

@Aaron1011
Copy link
Contributor Author

Aaron1011 commented Dec 2, 2019

Superseded by #66789

@Aaron1011 Aaron1011 closed this Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@zackmdavis zackmdavis

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MIR inlining does not handle SourceScopeLocalData

8 participants

@Aaron1011 @rust-highfive @bors @eddyb @joelpalmer @JohnCSimon @bjorn3 @zackmdavis