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.

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

Try to fix 48116 and 48192 #48209

Merged
merged 3 commits into from
Feb 14, 2018
Merged

Try to fix 48116 and 48192 #48209

merged 3 commits into from
Feb 14, 2018

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented Feb 14, 2018

The bug #48116 happens because of a misoptimization of the import_path_to_string function, where a names slice is empty but the !names.is_empty() branch is executed.

fn import_path_to_string(names: &[SpannedIdent],
subclass: &ImportDirectiveSubclass,
span: Span) -> String {
let pos = names.iter()
.position(|p| span == p.span && p.node.name != keywords::CrateRoot.name());
let global = !names.is_empty() && names[0].node.name == keywords::CrateRoot.name();
if let Some(pos) = pos {
let names = if global { &names[1..pos + 1] } else { &names[..pos + 1] };
names_to_string(names)
} else {
let names = if global { &names[1..] } else { names };
if names.is_empty() {
import_directive_subclass_to_string(subclass)
} else {
// Note that this code looks a little wonky, it's currently here to
// hopefully help debug #48116, but otherwise isn't intended to
// cause any problems.
let x = format!(
"{}::{}",
names_to_string(names),
import_directive_subclass_to_string(subclass),
);
assert!(!names.is_empty());
assert!(!x.starts_with("::"));
return x
}
}
}

Yesterday, @eddyb had locally reproduced the bug, and came across the position function where the assume() call is found to be suspicious. We have not concluded that this assume() causes #48116, but given the reputation of assume(), this seems higher relevant. Here we try to see if commenting it out can fix the errors.

Later @alexcrichton has bisected and found a potential bug in the LLVM side. We are currently testing if reverting that LLVM commit is enough to stop the bug. If true, this PR can be reverted (keep the assume()) and we could backport the LLVM patch instead.

(This PR also includes an earlier commit from #48127 for help debugging ICE happening in compile-fail/parse-fail tests.)

The PR also reverts #48059, which seems to cause #48192.

r? @alexcrichton
cc @eddyb, @arthurprs (#47333)

1. When the invalid condition is hit, write out the relevant variables too
2. In compile-fail/parse-fail tests, check for ICE first, so the invalid
   error patterns won't mask our ICE output.
@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 14, 2018
@alexcrichton
Copy link
Member

@bors: r+ p=100

@bors
Copy link
Contributor

bors commented Feb 14, 2018

📌 Commit 553f82d has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 14, 2018
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Feb 14, 2018

📌 Commit 71269a5 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Feb 14, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 14, 2018
kennytm and others added 2 commits February 15, 2018 00:04
Removed the `assume()` which we assumed is the cause of misoptimization in
issue rust-lang#48116.
@kennytm
Copy link
Member Author

kennytm commented Feb 14, 2018

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Feb 14, 2018

📌 Commit e0da990 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 14, 2018
@bors
Copy link
Contributor

bors commented Feb 14, 2018

⌛ Testing commit e0da990 with merge 3ec5a99...

bors added a commit that referenced this pull request Feb 14, 2018
Try to fix 48116 and 48192

The bug #48116 happens because of a misoptimization of the `import_path_to_string` function, where a `names` slice is empty but the `!names.is_empty()` branch is executed.

https://github.com/rust-lang/rust/blob/4d2d3fc5dadf894a8ad709a5860a549f2c0b1032/src/librustc_resolve/resolve_imports.rs#L1015-L1042

Yesterday, @eddyb had locally reproduced the bug, and [came across the `position` function](https://mozilla.logbot.info/rust-infra/20180214#c14296834) where the `assume()` call is found to be suspicious. We have *not* concluded that this `assume()` causes #48116, but given [the reputation of `assume()`](#45501 (comment)), this seems higher relevant. Here we try to see if commenting it out can fix the errors.

Later @alexcrichton has bisected and found a potential bug [in the LLVM side](#48116 (comment)). We are currently testing if reverting that LLVM commit is enough to stop the bug. If true, this PR can be reverted (keep the `assume()`) and we could backport the LLVM patch instead.

(This PR also includes an earlier commit from #48127 for help debugging ICE happening in compile-fail/parse-fail tests.)

The PR also reverts #48059, which seems to cause #48192.

r? @alexcrichton
cc @eddyb, @arthurprs (#47333)
@bors
Copy link
Contributor

bors commented Feb 14, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 14, 2018
@kennytm
Copy link
Member Author

kennytm commented Feb 14, 2018

@bors retry #46903

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 14, 2018
@kennytm
Copy link
Member Author

kennytm commented Feb 14, 2018

(Manually merged since everything else passed)

@kennytm kennytm merged commit e0da990 into rust-lang:master Feb 14, 2018
@kennytm kennytm deleted the try-fix-48116 branch February 14, 2018 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants