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

Fix generating CTFE backtrace on optimized MIR #66203

Closed
wants to merge 1 commit into from
Closed

Fix generating CTFE backtrace on optimized MIR #66203

wants to merge 1 commit into from

Conversation

Aaron1011
Copy link
Member

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
Collaborator

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
Contributor

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
@@ -105,6 +105,14 @@ pub struct Body<'tcx> {

/// 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

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

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

@Aaron1011
Copy link
Member Author

I'm implementing the change suggested by @eddyb

@Aaron1011
Copy link
Member Author

@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

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
Contributor

bors commented Dec 2, 2019

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

@Aaron1011
Copy link
Member Author

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