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

[foutrelis]

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).

[foutrelis]

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]

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]

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.

[foutrelis]

In D50416#1192106, @mgorny wrote:

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).

[foutrelis]

In D50416#1192161, @mgorny wrote:

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]

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

[hans]

In D50416#1192246, @mgorny wrote:

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]

Adding @tejohnson as owner of gold plugin.

[tejohnson]

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]

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.

[hans]

Sounds good to me.

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

[foutrelis]

Yes, someone please commit this for me.

[foutrelis]

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.

[hans]

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

[hans]

In D50416#1202548, @hans wrote:

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

Merged in r339993.

cmake/modules/HandleLLVMOptions.cmake

 # is unloaded.
 if(${CMAKE_SYSTEM_NAME} MATCHES "Linux")
   set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,-z,nodelete")
+  set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} -Wl,-z,nodelete")
 endif()