Skip to content

Commit

Permalink
Auto merge of rust-lang#105800 - lqd:dylib-thinlto, r=bjorn3
Browse files Browse the repository at this point in the history
Don't copy symbols from dylibs with `-Zdylib-lto`

When `rustc_driver` started being built with `-Zdylib-lto -Clto=thin`, some libstd symbols were copied by the LTO process into the dylib. That causes duplicate local symbols that are not present otherwise.

Depending on the situation (lib loading order apparently), the duplicated symbols could cause issues: `rustc_driver` overrode the panic hook, but it didn't apply to rustc main's hook (the default from libstd). This is the cause of rust-lang#105637, in some situations the panic hook installed by `rustc_driver` isn't executed, and only libstd's backtrace is shown (and a double panic). The query stack, as well as the various notes to open a GH about the ICE, don't appear.

It's not clear exactly what is needed to trigger the issue, but I have simulated a reproducer [here](https://github.com/lqd/issue-105637) with cargo involved, the incorrect panic hook is executed on my machine. It is hard to reproduce in a unit test: `cargo run` + `rustup` involves LD_LIBRARY_PATH, which is not the case for `compiletest`. cargo also adds unconditional flags that are then overridden in [`bootstrap` when building rustc with `rust.lto = thin`](https://github.com/rust-lang/rust/blob/9c07efe84f28a44f3044237696acc295aa407ee5/src/bootstrap/compile.rs#L702-L714) as done on CI).

All this to say the compilation and execution environment in `bootstrap` leading to the bug building `rustc_driver` is different from our UI tests, and I believe one of the reasons it's hard to make an exact reproducer test. Thankfully there's _still_ a difference in the behavior though: although in the unit test the correct panic hook seems to be executed compared to my repro and the current nightly, only the fix removes the double panic here.

The `7e8277aefa12f1469fb1df01418ff5846a7854a9` `try` build:
- fixes the reproducer repo linked above
- restores the ICE messages from rust-lang#105321 back to the state in its OP compared to the description in rust-lang#105637
- restores the ICE message and the query stack from rust-lang#105777 compared to nightly

While I believe this technically fixes the P-critical issue rust-lang#105637, I would not want to close it yet as we may want to backport to beta/stable (if a point release happens, it would fix the ICEs reported on 1.66.0, which is built with ThinLTO on linux). Once this PR lands, I'll also open another PR to re-enable ThinLTO on x64 darwin's dist builder.
  • Loading branch information
bors committed Dec 17, 2022
2 parents aef17b7 + be5685b commit 65c53c3
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 1 deletion.
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/back/link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ pub fn each_linked_rlib(
};
for &cnum in crates {
match fmts.get(cnum.as_usize() - 1) {
Some(&Linkage::NotLinked | &Linkage::IncludedFromDylib) => continue,
Some(&Linkage::NotLinked | &Linkage::Dynamic | &Linkage::IncludedFromDylib) => continue,
Some(_) => {}
None => return Err(errors::LinkRlibError::MissingFormat),
}
Expand Down
23 changes: 23 additions & 0 deletions src/test/ui/lto/auxiliary/thinlto-dylib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// Auxiliary crate for test issue-105637: the LTOed dylib which had duplicate symbols from libstd,
// breaking the panic hook feature.
//
// This simulates the `rustc_driver` crate, and the main crate simulates rustc's main binary hooking
// into this driver.

// compile-flags: -Zdylib-lto -C lto=thin

use std::panic;

pub fn main() {
// Install the hook we want to see executed
panic::set_hook(Box::new(|_| {
eprintln!("LTOed auxiliary crate panic hook");
}));

// Trigger the panic hook with an ICE
run_compiler();
}

fn run_compiler() {
panic!("ICEing");
}
28 changes: 28 additions & 0 deletions src/test/ui/lto/issue-105637.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Regression test for issue #105637: `-Zdylib-lto` with LTO duplicated symbols from other dylibs,
// in this case from libstd.
//
// That manifested as both `rustc_driver` and rustc's "main" (`compiler/rustc`) having their own
// `std::panicking::HOOK` static, and the hook in rustc's main (the default stdlib's) being executed
// when rustc ICEs, instead of the overriden hook from `rustc_driver` (which also displays the query
// stack and information on how to open a GH issue for the encountered ICE).
//
// In this test, we reproduce this setup by installing a panic hook in both the main and an LTOed
// dylib: the last hook set should be the one being executed, the dylib's.

// aux-build: thinlto-dylib.rs
// run-fail
// check-run-results

extern crate thinlto_dylib;

use std::panic;

fn main() {
// We don't want to see this panic hook executed
std::panic::set_hook(Box::new(|_| {
eprintln!("main crate panic hook");
}));

// Have the LTOed dylib install its own hook and panic, we want to see its hook executed.
thinlto_dylib::main();
}
1 change: 1 addition & 0 deletions src/test/ui/lto/issue-105637.run.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
LTOed auxiliary crate panic hook

0 comments on commit 65c53c3

Please sign in to comment.