cdylib link error after TLS unused -> used transition

[davidlattimore]

From nightly-2020-02-28 (nightly-2020-02-27 was fine), I get link errors if I compile a cdylib with a thread local that isn't used, then compile again after referencing the thread local.

To reproduce, create a.rs:

pub struct Foo {}
impl Drop for Foo {
    fn drop(&mut self) {
        println!("Dropping Foo");
    }
}
#[no_mangle]
pub extern "C" fn run() {
    thread_local! { pub static FOO : Foo = Foo { } ; }
    //FOO.with(|_f| ())
}

Then run:

rustc a.rs --crate-type cdylib --emit=dep-info,link -C opt-level=2 --out-dir target -C incremental=target/incremental

Uncomment the commented line and rerun the above command.

This produces the following link error:

1k5ednsjh2ku99ix:(.text._ZN3std6thread5local4fast12Key$LT$T$GT$14try_initialize17h998faaa5ce3b2835E.llvm.11804660923702021477+0x3a): undefined reference to `anon.043848776374366c7b657cba74aa2e83.1.llvm.9876575881142733961'
/usr/bin/ld: target/lib.1k5ednsjh2ku99ix.rcgu.o: relocation R_X86_64_PC32 against undefined hidden symbol `anon.043848776374366c7b657cba74aa2e83.1.llvm.9876575881142733961' can not be used when making a shared object

It's possible however that the underlying bug is actually older though, since at opt-level=1, I get link errors from 1.32.0 onwards (1.31.0 was fine). In that case, the Drop implementation isn't needed.

pub struct Foo {}
#[no_mangle]
pub extern "C" fn run() {
    thread_local! { pub static FOO : Foo = Foo { } ; }
    //FOO.with(|_f| ())
}

rustc a.rs --crate-type cdylib --emit=dep-info,link -C opt-level=1 --out-dir target -C incremental=target/incremental

Linking fails with: undefined reference to 'core::ptr::drop_in_place'

The exact symbol that's reported varies.

Versions:
rustc 1.43.0-nightly (2890b37 2020-03-06)
cc (Debian 9.2.1-16) 9.2.1 20191030
GNU ld (GNU Binutils for Debian) 2.33.1

[Aaron1011]

@rustbot claim

[Aaron1011]

Here's what appears to be happening:

1. We generate a CGU containing some drop glue, along with some globals (e.g. the "Dropping Foo" constant). When LTO runs, LLVM consults the ThinLTO index, and determines that we don't actually export anything from this CGU. This allows it to skip promoting the 'local' constant to a globally visible symbol. This allows said constant (e.g. anon.043848776374366c7b657cba74aa2e83.1.llvm.9876575881142733961) to be deleted entirely, since it cannot be accessed outside of the CGU.

2. We uncomment FOO.with(|_f| ()). Now, we end up needing to import this drop glue (since it gets called by the drop glue for Option<Foo>). We re-generate the module performing the import, but not the module we are importing from.

3. The freshly codegenned module ends up inlining a function from the re-used module, which references an anon.*.llvm.* constant from the re-used module. However, the post-optimization IR/ASM for the re-used module no longer contains this constant, since it was unused at the time the re-used module was optimized.

Note that exact names imported seem to be extremely sensitive to local changes, so you might get different constants/containing functions.

The general problem seems to be that LLVM looks 'backward' in the dependency graph during ThinLTO - that is, it looks at whether any modules import from the current module, not just what modules the current modules imports.

I think there are a few possible solutions:

1. When a module goes from exporting nothing to exporting something, rebuild any modules which now import it. This is the most targeted fix, but I'm worried that it could be quite fragile. As far as I can tell, LLVM assumes that post-LTO files are never reused. Theoretically, LLVM could perform a more precise check to see which functions are actually 'exported' (e.g. used elsewhere) from a given ThinLTO CGU.

2. Extend the check added in save LTO import info and check it when trying to reuse build products #67020 to rebuild both the 'importer' and 'importee' when the import map changes. That is, if module 'A' starts/stops importing from module 'B', we rebuild both A and B. This is a broader change than 1), and should hopefully continue to work even if LLVM gets 'smarter'. However, it could cause us to unnecessarily re-run codegen for some modules, which could have a significant performance impact.

3. Patch LLVM to be more conservative about deciding that a module has no exports. It's not clear to me how easy such a change would be, or whether upstream would accept it.

Someone who's more familiar with LLVM should weigh in here.

[spastorino]

@rustbot ping icebreakers-llvm

[rustbot]

Hey LLVM ICE-breakers! This bug has been identified as a good
"LLVM ICE-breaking candidate". In case it's useful, here are some
instructions for tackling these sorts of bugs. Maybe take a look?
Thanks! <3

cc @comex @DutchGhost @hanna-kruppe @hdhoang @heyrutvik @JOE1994 @jryans @mmilenko @nagisa @nikic @Noah-Kennedy @SiavoshZarrasvand @spastorino @vertexclique @vgxbj

[spastorino]

We have the first non working nightly which is 2020-02-28, although it would be nice to get the first regresed commit or PR using cargo-bisect-rustc.

[spastorino]

Tagging as P-high until we know how bad this issue is.

[davidlattimore]

From git log abc3073c9..6d69caba1, and without really knowing what's going on, 49c68bd seems likely to me. It mentions removing MIR Drop terminators that will never be reached.

[davidlattimore]

Seems my guess was wrong. I ran cargo-bisect-rustc (neat command BTW). It reported the regression with commit d28560e

[pnkfelix]

For reference, d28560e is part of PR #67332; i.e. the latter PR either injected this bug, or tickled a latent bug in LLVM. (Which of the latter two cases we are in depends in part on how we resolve the questions raised earlier by @Aaron1011 .)

[pnkfelix]

Of the options outlined by @Aaron1011 above, I think number 2 is best:

Extend the check added in #67020 to rebuild both the 'importer' and 'importee' when the import map changes. That is, if module 'A' starts/stops importing from module 'B', we rebuild both A and B. This is a broader change than 1), and should hopefully continue to work even if LLVM gets 'smarter'. However, it could cause us to unnecessarily re-run codegen for some modules, which could have a significant performance impact.

I have started prototyping it on my laptop, since I am familiar with the code I added in PR #67020

[pnkfelix]

My prototype is now done and appears to fix this case. I have not attempted to benchmark yet; I figure we can land it, or at least run it through perf.rlo, and see where it goes.

[pnkfelix]

marking as stable-to-beta regression. The underlying bug here is old, but the change that exposes the bug here (PR #67332) has not yet made it to stable.