Skip to content

Commit

Permalink
Auto merge of #48209 - kennytm:try-fix-48116, r=alexcrichton
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
bors committed Feb 14, 2018
2 parents 4d2d3fc + e0da990 commit 3ec5a99
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 34 deletions.
27 changes: 5 additions & 22 deletions src/bootstrap/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -600,25 +600,9 @@ impl<'a> Builder<'a> {
//
// FIXME: the guard against msvc shouldn't need to be here
if !target.contains("msvc") {
let ccache = self.config.ccache.as_ref();
let ccacheify = |s: &Path| {
let ccache = match ccache {
Some(ref s) => s,
None => return s.display().to_string(),
};
// FIXME: the cc-rs crate only recognizes the literal strings
// `ccache` and `sccache` when doing caching compilations, so we
// mirror that here. It should probably be fixed upstream to
// accept a new env var or otherwise work with custom ccache
// vars.
match &ccache[..] {
"ccache" | "sccache" => format!("{} {}", ccache, s.display()),
_ => s.display().to_string(),
}
};
let cc = ccacheify(&self.cc(target));
cargo.env(format!("CC_{}", target), &cc)
.env("CC", &cc);
let cc = self.cc(target);
cargo.env(format!("CC_{}", target), cc)
.env("CC", cc);

let cflags = self.cflags(target).join(" ");
cargo.env(format!("CFLAGS_{}", target), cflags.clone())
Expand All @@ -633,9 +617,8 @@ impl<'a> Builder<'a> {
}

if let Ok(cxx) = self.cxx(target) {
let cxx = ccacheify(&cxx);
cargo.env(format!("CXX_{}", target), &cxx)
.env("CXX", &cxx)
cargo.env(format!("CXX_{}", target), cxx)
.env("CXX", cxx)
.env(format!("CXXFLAGS_{}", target), cflags.clone())
.env("CXXFLAGS", cflags);
}
Expand Down
24 changes: 15 additions & 9 deletions src/libcore/slice/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1246,15 +1246,18 @@ macro_rules! iterator {
{
// The addition might panic on overflow
// Use the len of the slice to hint optimizer to remove result index bounds check.
let n = make_slice!(self.ptr, self.end).len();
let _n = make_slice!(self.ptr, self.end).len();
self.try_fold(0, move |i, x| {
if predicate(x) { Err(i) }
else { Ok(i + 1) }
}).err()
.map(|i| {
unsafe { assume(i < n) };
i
})
// // FIXME(#48116/#45964):
// // This assume() causes misoptimization on LLVM 6.
// // Commented out until it is fixed again.
// .map(|i| {
// unsafe { assume(i < n) };
// i
// })
}

#[inline]
Expand All @@ -1271,10 +1274,13 @@ macro_rules! iterator {
if predicate(x) { Err(i) }
else { Ok(i) }
}).err()
.map(|i| {
unsafe { assume(i < n) };
i
})
// // FIXME(#48116/#45964):
// // This assume() causes misoptimization on LLVM 6.
// // Commented out until it is fixed again.
// .map(|i| {
// unsafe { assume(i < n) };
// i
// })
}
}

Expand Down
15 changes: 13 additions & 2 deletions src/librustc_resolve/resolve_imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1026,6 +1026,8 @@ fn import_path_to_string(names: &[SpannedIdent],
if names.is_empty() {
import_directive_subclass_to_string(subclass)
} else {
// FIXME: Remove this entire logic after #48116 is fixed.
//
// 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.
Expand All @@ -1034,8 +1036,17 @@ fn import_path_to_string(names: &[SpannedIdent],
names_to_string(names),
import_directive_subclass_to_string(subclass),
);
assert!(!names.is_empty());
assert!(!x.starts_with("::"));
if names.is_empty() || x.starts_with("::") {
span_bug!(
span,
"invalid name `{}` at {:?}; global = {}, names = {:?}, subclass = {:?}",
x,
span,
global,
names,
subclass
);
}
return x
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/tools/compiletest/src/runtest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ impl<'test> TestCx<'test> {
fn run_cfail_test(&self) {
let proc_res = self.compile_test();
self.check_if_test_should_compile(&proc_res);
self.check_no_compiler_crash(&proc_res);

let output_to_check = self.get_output(&proc_res);
let expected_errors = errors::load_errors(&self.testpaths.file, self.revision);
Expand All @@ -262,7 +263,6 @@ impl<'test> TestCx<'test> {
self.check_error_patterns(&output_to_check, &proc_res);
}

self.check_no_compiler_crash(&proc_res);
self.check_forbid_output(&output_to_check, &proc_res);
}

Expand Down

0 comments on commit 3ec5a99

Please sign in to comment.