Page MenuHomePhabricator

[dsymutil] Explicitly link against libatomic when necessary
AcceptedPublic

Authored by gokturk on Oct 15 2019, 1:40 PM.

Details

Summary

In some systems, such as RISC-V, atomic support requires explicit
linking against '-latomic' (see
https://github.com/riscv/riscv-gcc/issues/12).

Diff Detail

Event Timeline

gokturk created this revision.Oct 15 2019, 1:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2019, 1:40 PM

I've started a build on the affected VM (riscv64gc) to see if it solved the problem. It typically takes ~20 hours until the error is produced.

Also see https://reviews.llvm.org/D68964, related to this issue.

gokturk added a comment.EditedWed, Oct 16, 11:56 AM

I've started a build on the affected VM (riscv64gc) to see if it solved the problem. It typically takes ~20 hours until the error is produced.

Also see https://reviews.llvm.org/D68964, related to this issue.

I can confirm that this patch combined with D68964 resolves the build issue for me.

This seems reasonable to me but I'm not an expert at LLVM's CMake setup.

I'm not sure this is the correct fix.

Probably a more correct fix would be to change line 29 of llvm/lib/Support/CMakeLists.txt to:

if( LLVM_ENABLE_THREADS AND (HAVE_LIBATOMIC OR HAVE_CXX_LIBATOMICS64))

I'm not sure this is the correct fix.

Probably a more correct fix would be to change line 29 of llvm/lib/Support/CMakeLists.txt to:

if( LLVM_ENABLE_THREADS AND (HAVE_LIBATOMIC OR HAVE_CXX_LIBATOMICS64))

Would that propagate the flag to tools/dsymutil ? The line you suggested already checks for HAVE_LIBATOMIC, but dsymutil still fails to link against libatomic.

Also, why HAVE_LIBATOMIC? There's another test right after it that checks whether the compiler works with -latomic, and sets HAVE_CXX_ATOMICS_WITH_LIB. Isn't that a better variable to check for?

My main issue with my patch is that it adds -latomic to every tool compilation, whereas only dsymutil seems to need it.

lenary added a subscriber: lenary.Thu, Oct 17, 9:24 AM

I think that @beanz is right about that. The reason for the HAVE_LIBATOMIC is because it is managing the current configuration checks. It may be possible to simplify it further in the future, but that area of the code has a HAVE_LIBATOMIC. That will setup system_libs properly and dsymutil can use system_libs as a parameter to LINK_LIBS option.

gokturk updated this revision to Diff 226481.Fri, Oct 25, 12:33 PM
gokturk retitled this revision from cmake/modules/AddLLVM.cmake: pass '-latomic' to the linker if necessary to [dsymutil] Add ${system_libs} to target link libraries.
gokturk edited the summary of this revision. (Show Details)

I think that @beanz is right about that. The reason for the HAVE_LIBATOMIC is because it is managing the current configuration checks. It may be possible to simplify it further in the future, but that area of the code has a HAVE_LIBATOMIC. That will setup system_libs properly and dsymutil can use system_libs as a parameter to LINK_LIBS option.

dsymutil is created using add_llvm_tool(), which doesn't take a LINK_LIBS option. I see that llvm_add_library() supports this but I wasn't sure how to take advantage of that, so I went with target_link_libraries().

I also have another patch to establish:

-  if( LLVM_ENABLE_THREADS AND HAVE_LIBATOMIC )
+  if( LLVM_ENABLE_THREADS AND (HAVE_LIBATOMIC OR HAVE_CXX_LIBATOMICS64) )

but I'll submit that as another revision if that's alright.

I'm in the process of testing if this patch works, which takes about 20 hours on a VM. I'll post here the result when it's done.

JDevlieghere accepted this revision.Fri, Oct 25, 2:36 PM
This revision is now accepted and ready to land.Fri, Oct 25, 2:36 PM
beanz requested changes to this revision.Fri, Oct 25, 2:50 PM

You don't need to add the library to dsymutil. It is not code in dsymutil that depends on libatomic, it is code in LLVMSupport that dsymutil is using. By fixing this in LLVMSupport, any library linked against LLVMSupport will have a linkage to libatomic added.

Please don't spend 20 hours testing this patch. I'm very pretty sure this isn't the correct fix.

This revision now requires changes to proceed.Fri, Oct 25, 2:50 PM

You don't need to add the library to dsymutil. It is not code in dsymutil that depends on libatomic, it is code in LLVMSupport that dsymutil is using. By fixing this in LLVMSupport, any library linked against LLVMSupport will have a linkage to libatomic added.

Please don't spend 20 hours testing this patch. I'm very pretty sure this isn't the correct fix.

dsymutil makes explicit use of libatomic (https://github.com/llvm/llvm-project/blob/release/9.x/llvm/tools/dsymutil/dsymutil.cpp#L542):

    std::atomic_char AllOK(1);
...
      auto LinkLambda = [&,
                         OutputFile](std::shared_ptr<raw_fd_ostream> Stream) {
        AllOK.fetch_and(linkDwarf(*Stream, BinHolder, *Map, *OptionsOrErr));
        Stream->flush();
        if (Verify && !NoOutput)
          AllOK.fetch_and(verify(OutputFile, Map->getTriple().getArchName()));
      };

Doesn't this create a link time dependency on libatomic?

gokturk updated this revision to Diff 226505.Fri, Oct 25, 3:15 PM

I'm updating the patch for completeness. system_libs doesn't propagate, so I had to get the system libs property of Support in dsymutil:

get_property(system_libs TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS)

I understand that there is already an objection to this but like I said, I'm updating it for completeness.

You don't need to add the library to dsymutil. It is not code in dsymutil that depends on libatomic, it is code in LLVMSupport that dsymutil is using. By fixing this in LLVMSupport, any library linked against LLVMSupport will have a linkage to libatomic added.

Please don't spend 20 hours testing this patch. I'm very pretty sure this isn't the correct fix.

dsymutil makes explicit use of libatomic (https://github.com/llvm/llvm-project/blob/release/9.x/llvm/tools/dsymutil/dsymutil.cpp#L542):

    std::atomic_char AllOK(1);
...
      auto LinkLambda = [&,
                         OutputFile](std::shared_ptr<raw_fd_ostream> Stream) {
        AllOK.fetch_and(linkDwarf(*Stream, BinHolder, *Map, *OptionsOrErr));
        Stream->flush();
        if (Verify && !NoOutput)
          AllOK.fetch_and(verify(OutputFile, Map->getTriple().getArchName()));
      };

Doesn't this create a link time dependency on libatomic?

@beanz, the following code snippet:

#include <atomic>

int main()
{
	std::atomic_char AllOK(1);
	AllOK.fetch_and(0);

	return 0;
}

fails to compile on RISC-V using gcc 9.2.0 with the following:

/usr/lib/gcc/riscv64-unknown-linux-gnu/9.2.0/../../../../riscv64-unknown-linux-gnu/bin/ld: /tmp/ccVjOu0P.o: in function `.L0 ':
AllOK.cpp:(.text+0x38): undefined reference to `__atomic_fetch_and_1'
collect2: error: ld returned 1 exit status
make: *** [<builtin>: AllOK] Error 1

The fix is to link against latomic. That leads me to think that dsymutil also requires '-latomic' irrespective of whether it depends on Support or not. Am I missing something?

beanz accepted this revision.Tue, Nov 5, 10:51 AM

Sorry, I had missed the atomic use directly in the tool. Sorry for my incorrect statements. This seems like a reasonable fix.

This revision is now accepted and ready to land.Tue, Nov 5, 10:51 AM

You don't need to add the library to dsymutil. It is not code in dsymutil that depends on libatomic, it is code in LLVMSupport that dsymutil is using. By fixing this in LLVMSupport, any library linked against LLVMSupport will have a linkage to libatomic added.

Please don't spend 20 hours testing this patch. I'm very pretty sure this isn't the correct fix.

I've debugged why the '-latomic' flag is not propagating from Support to dsymutil. When LLVM_DYLIB_COMPONENTS is set to all, dsymutil is set to link against libLLVM as opposed to Support (https://github.com/llvm/llvm-project/blob/5be949e3d007ea0bf1979a483ce558d33eca5d6a/llvm/cmake/modules/LLVM-Config.cmake#L84):

-- target_link_libraries name: LLVMSupport libtype: INTERFACE ARG_LINK_LIBS: z;rt;dl;tinfo;atomic;-lpthread;m lib_deps: LLVMDemangle llvm_libs: 
-- Targeting RISCV
-- add_llvm_executable() name: dsymutil USE_SHARED LLVM_LINK_COMPONENTS: AllTargetsAsmPrinters;AllTargetsCodeGens;AllTargetsDescs;AllTargetsInfos;AsmPrinter;DebugInfoDWARF;MC;Object;Option;Support;Target
-- llvm_config() executable: dsymutil ARG_USE_SHARED: TRUE link_components: AllTargetsAsmPrinters;AllTargetsCodeGens;AllTargetsDescs;AllTargetsInfos;AsmPrinter;DebugInfoDWARF;MC;Object;Option;Support;Target ARGN: USE_SHARED;AllTargetsAsmPrinters;AllTargetsCodeGens;AllTargetsDescs;AllTargetsInfos;AsmPrinter;DebugInfoDWARF;MC;Object;Option;Support;Target
-- llvm_config() LLVM_DYLIB_COMPONENTS: all
-- explicit_llvm_config() executable: dsymutil type: EXECUTABLE LIBRARIES:  link_components: 

LINK_LIBRARIES = -Wl,-rpath,"\$$ORIGIN/../lib64/lp64d" lib64/lp64d/libLLVM-10svn.so -lpthread

and the resulting LINK_LIBRARIES doesn't contain the '-latomic' that should have been propagated through Support. If I forcefully inject Support back into link_components in (https://github.com/llvm/llvm-project/blob/5be949e3d007ea0bf1979a483ce558d33eca5d6a/llvm/cmake/modules/LLVM-Config.cmake#L84), it does propagate correctly:

-- target_link_libraries name: LLVMSupport libtype: INTERFACE ARG_LINK_LIBS: z;rt;dl;tinfo;atomic;-lpthread;m lib_deps: LLVMDemangle llvm_libs: 
-- Targeting RISCV
-- add_llvm_executable() name: dsymutil USE_SHARED LLVM_LINK_COMPONENTS: AllTargetsAsmPrinters;AllTargetsCodeGens;AllTargetsDescs;AllTargetsInfos;AsmPrinter;DebugInfoDWARF;MC;Object;Option;Support;Target
-- llvm_config() executable: dsymutil ARG_USE_SHARED: TRUE link_components: AllTargetsAsmPrinters;AllTargetsCodeGens;AllTargetsDescs;AllTargetsInfos;AsmPrinter;DebugInfoDWARF;MC;Object;Option;Support;Target ARGN: USE_SHARED;AllTargetsAsmPrinters;AllTargetsCodeGens;AllTargetsDescs;AllTargetsInfos;AsmPrinter;DebugInfoDWARF;MC;Object;Option;Support;Target
-- llvm_config() LLVM_DYLIB_COMPONENTS: all
-- explicit_llvm_config() executable: dsymutil type: EXECUTABLE LIBRARIES: LLVMSupport link_components: Support

LINK_LIBRARIES = -Wl,-rpath,"\$$ORIGIN/../lib64/lp64d" lib64/lp64d/libLLVM-10svn.so lib64/lp64d/libLLVMSupport.a -lpthread -lz -lrt -ldl -ltinfo -latomic -lpthread -lm lib64/lp64d/libLLVMDemangle.a

I don't understand why the flag doesn't propagate from Support to libLLVM to dsymutil.

@beanz which fix is being requested? I think the current patch is incorrect because -latomic propagates if not dynamically linking against libLLVM.

gokturk updated this revision to Diff 227964.Tue, Nov 5, 2:20 PM
gokturk retitled this revision from [dsymutil] Add ${system_libs} to target link libraries to [dsymutil] Explicitly link against libatomic when necessary.
gokturk edited the summary of this revision. (Show Details)

Here's the proposed patch. I think this is the least invasive of all. Just check if passing '-latomic' is required inside dsymutil's cmake file.

gokturk requested review of this revision.Tue, Nov 5, 2:20 PM
beanz accepted this revision.Tue, Nov 5, 2:27 PM

Yea, that makes sense. Sorry again for the run around.

This revision is now accepted and ready to land.Tue, Nov 5, 2:27 PM