[dsymutil] Explicitly link against libatomic when necessary

[gokturk]

Summary

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

[gokturk]

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]

In D69003#1710088, @gokturk wrote:

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.

[davezarzycki]

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

[beanz]

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

[gokturk]

In D69003#1711697, @beanz wrote:

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.

[compnerd]

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]

In D69003#1720595, @compnerd wrote:

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.

[beanz]

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.

[gokturk]

In D69003#1721908, @beanz wrote:

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]

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.

[gokturk]

In D69003#1721924, @gokturk wrote:

In D69003#1721908, @beanz wrote:

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]

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

[gokturk]

In D69003#1721908, @beanz wrote:

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]

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.

[beanz]

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

llvm/tools/dsymutil/CMakeLists.txt

add_llvm_tool(dsymutil
   DEPENDS
   intrinsics_gen
   ${tablegen_deps}
   )
 
 if(APPLE)
   target_link_libraries(dsymutil PRIVATE "-framework CoreFoundation")
 endif(APPLE)
+
+if(HAVE_CXX_ATOMICS_WITH_LIB OR HAVE_CXX_ATOMICS64_WITH_LIB)
+  target_link_libraries(dsymutil PRIVATE atomic)
+endif()