Page MenuHomePhabricator

[cmake] Prevent LLVMgold.so from being unloaded on Linux
ClosedPublic

Authored by foutrelis on Aug 7 2018, 3:54 PM.

Details

Summary

Extend the fix from D40459 to also apply to modules such as the LLVM
gold plugin. This is needed because current binutils master (and future
binutils 2.32) calls dlclose() on bfd plugins as part of a recent fix
for PR23460 (https://sourceware.org/bugzilla/show_bug.cgi?id=23460).

Diff Detail

Repository
rL LLVM

Event Timeline

foutrelis created this revision.Aug 7 2018, 3:54 PM

Does this make sense or could it cause issues with other modules? I haven't thought of possible implications, but it seems to work OK when tested on top of LLVM 6.0.1.

I also added a comment to the binutils issue, asking if removing the call to dlclose() might be an option.

mgorny added a comment.Aug 8 2018, 5:23 AM

I don't really know BFD plugin API, so I don't know if this could have any implications (I suppose upstream's answer may be helpful there). However, I don't think it likely to cause any real trouble.

Just to confirm, have you tested it thoroughly?

mgorny added a comment.Aug 8 2018, 5:52 AM

Also, are you maybe aware of how soon is this going to hit binutils users? It kinda looks like a ticking time bomb, so maybe it would be worth fixing it for LLVM 7 release as well.

I don't really know BFD plugin API, so I don't know if this could have any implications (I suppose upstream's answer may be helpful there). However, I don't think it likely to cause any real trouble.

Certainly, we can wait for a reply from binutils first. If the dlclose() stays, "-z nodelete" seems like a good way to ensure LLVMgold.so won't be loaded twice.

The patch here also means that BugpointPasses.so gets the NODELETE flag. I'm not familiar with that part of LLVM and haven't tested it with this change.

Just to confirm, have you tested it thoroughly?

Not an exhaustive test but:

I hit the binutils issue when building Chromium after binutils 2.31.1 landed in Arch Linux.

After patching binutils to not leave open files, the Chromium build would fail immediately when ar was run with:

CommandLine Error: Option 'asm-instrumentation' registered more than once!

After adding the NODELETE flag to LLVMgold.so (through this patch), the Chromium build completed successfully (using clang w/ ThinLTO and also LLD for linking).

Also, are you maybe aware of how soon is this going to hit binutils users? It kinda looks like a ticking time bomb, so maybe it would be worth fixing it for LLVM 7 release as well.

It only exists in master and wasn't merged to binutils-2_31-branch, so looks like it'll be in binutils 2.32 (~January 2019, based on the time previous releases were made).

Some distros might cherry pick it into their binutils 2.31 package in order to fix issues where ar would create incomplete archives when adding 1000+ object files in one go.

mgorny added a reviewer: hans.Aug 8 2018, 6:43 AM
mgorny added a subscriber: hans.

Let's query @hans 's opinion on this. ~Jan 2019 sounds like it may still be relevant to LLVM 7.

hans added a comment.Aug 9 2018, 5:34 AM

Let's query @hans 's opinion on this. ~Jan 2019 sounds like it may still be relevant to LLVM 7.

Yes, if we decide this is the correct fix, merging it to LLVM 7 makes sense.

I don't know enough about this area to know if it could cause any bad effects though :-/

mgorny added a subscriber: tejohnson.

Adding @tejohnson as owner of gold plugin.

No objections from the gold plugin side, but like Hans I don't feel I know much about all the implications. Perhaps wait for the answer on the binutils thread first, but if they can't remove the dlclose there then this seems ok to me.

mgorny accepted this revision.Aug 14 2018, 9:36 AM

Given the answer on binutils thread, let's commit this. If it causes any unexpected issues, hopefully we'll learn about them during the RC testing.

This revision is now accepted and ready to land.Aug 14 2018, 9:36 AM
hans added a comment.Aug 16 2018, 2:24 AM

Sounds good to me.

foutrelis: Do you have commit access or do you need someone to commit this for you?

Yes, someone please commit this for me.

This proposed change might avoid triggering the reloading issue with the LLVM gold plugin. (At least the simple ar r ab.a a.o b.o from my previous comment doesn't trigger it.)

However, it still feels like a good idea to have the NODELETE flag set on LLVMgold.so, since we know it will throw an error if reloaded, so might as well guard against that.

This revision was automatically updated to reflect the committed changes.
hans added a comment.Aug 16 2018, 8:13 AM

I'll merge this to 7.0 after it's baked in trunk a little.

hans added a comment.Aug 17 2018, 12:26 AM

I'll merge this to 7.0 after it's baked in trunk a little.

Merged in r339993.