-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Undefined Symbol Linking ThinLTO on Fuchsia #53794
Comments
@cramertj are you able to reduce the test case down to one that we can reproduce? That would make it much easier to track down and confirm a fix. |
@alexcrichton Unfortunately I don't believe y'all have access to an updated sysroot for Fuchsia anyways (without cloning and building the fuchsia repo) so I don't know of a minimal way for you to get something you could link against, even with a working rustc. Do you have a copy of the fuchsia sysroot you can link against for testing, or would you be willing to check out and build fuchsia's kernel in order to get one? |
Scratch that-- I asked around and there's a way to pull down a recent sysroot using some scripts. I'll post here when I've gotten a small repro. |
will get you a sysroot, libs dir, and our version of clang, which you can use to compile w/
I'll work on tracking down a minimal repro using this approach. |
Okay, it's not exactly minimal, but I've put together this repo containing some scripts and an example project that will reproduce the error. To download the fuchsia SDK, run Note that small changes to |
Ping @michaelwoerister have you had any luck trying to track this down? If not, is there any possibility we could roll back the original change? This is blocking a number of different issues in Fuchsia. If there's any more information or support I can provide, let me know. |
I'm getting linker errors when trying to build your repo, but not the expected ones:
Looks like a problem with the SDK? Ran the following commands after cloning:
My OS is Ubuntu 16.04. |
@michaelwoerister Ugh... I've spent several hours trying to track this down. It seems that something has changed now since I originally tested, and I get the same error unless I change |
Encountered #54042 when working towards a repro. It may be related. |
Ok I've done some digging with @cramertj and here's some things we've discovered. I was never able to reproduce the link error seen here exactly but I was able to get pretty close: https://gist.github.com/alexcrichton/c79ba388aeb2d94e1ac22592e1bd9562 The bug appears to be because this comment is violated, namely the same CGU name is used in two different crates for different data. This later causes issues with ThinLTO which is where that requirement stems from. The reproduction above shows that if you have two crates then the "volatile" CGUs which contain instantiations from upstream crates will end up having the same CGU name as the CGU name for a local crate doesn't consider the local crate's crate name and crate disambiguator, only the upstream one (which is now duplicated between two CGUs). That brings up a whole bunch of questions of "why don't we see this everywhere?!" which I believe is all answered by:
So this bug only shows up when we're doing whole-crate-graph ThinLTO (a rarity) and some dependencies were compiled incrementally. You can't even express this in Cargo because this invocation is invalid: rustc foo.rs -C incremental=out -C lto=thin as rustc prints "error: can't perform LTO when compiling incrementally" (as we haven't implemented it!). All that to say that this all makes sense that it's only Fuchsia's own build system, no one else can even encounter this with Cargo! @michaelwoerister does that make sense? I think the fix here is to factor in the local crate name and crate disambiguator into "volatile" codegen units, but I'd want to confirm with your before sending a PR. |
Oh yes, that makes sense! Always adding the instantiating crate ID to the CGU name would indeed be a valid fix for the problem. |
@alexcrichton, it sounds like you already have a PR ready? Otherwise, let me know and I'll create one. |
Oh I don't have a patch yet myself, was gonna confirm first. if you'd like to make the patch though feel free! |
Sure, I'll do a PR tomorrow. It's me who introduced the regression after all |
Really make CGU names unique across crates. This will hopefully fix issue #53794. r? @alexcrichton
I've confirmed that #54152 fixed this. Thanks so much @alexcrichton and @michaelwoerister! |
Great to hear! Credit goes to @alexcrichton for identifying the underlying problem. The fix was pretty simple after that |
After #53356, all Rust binaries fail to link when using ThinLTO on Fuchsia (at least, all the admittedly non-minimal examples I have tried do). The full error is here. TL;DR:
ld.lld: error: undefined symbol: _$LT$$u5b$A$u5d$$u20$as$u20$core..slice..SlicePartialEq$LT$A$GT$$GT$::equal::hcbd9a5cdc751fed8
.Most binaries mentioned
PartialEq
-related things as the symbol that was failing to link, which is maybe coincidental (due to the order in which function references occur or something) but I figured it was worth mentioning.The error does not occur before #53356, and does not occur even post-#53356 when disabling LTO or using fat LTO.
cc @michaelwoerister
The text was updated successfully, but these errors were encountered: