Make sure libLLVM users link with libatomic if needed

[aaronpuchert]

Summary

64-bit atomics are used in llvm/ADT/Statistic.h, which means that users
of libLLVM.so might also have to link with libatomic. Ideally we would
properly distinguish between public and private dependencies on the
component level, but that's of course difficult. Collecting all
dependencies from e.g. Support might also be too much.

This fixes a build failure on PowerPC 32-bit.

[beanz]

We probably should replace atomic here with the LLVM_SYSTEM_LIBS property from the LLVMSupport target. That we should be able to rely on unconditionally, and you can read it with get_property(SUPPORT_SYSTEM_LIBS TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS).

[aaronpuchert]

In D132799#3759418, @beanz wrote:

We probably should replace atomic here with the LLVM_SYSTEM_LIBS property from the LLVMSupport target.

I wasn't quite sure if all of them are public dependencies, but if that's what they're meant to be, I'd go for that.

What made me suspicious is

if(LLVM_WITH_Z3)
  set(system_libs ${system_libs} ${Z3_LIBRARIES})
endif()

since the only use is in llvm/lib/Support/Z3Solver.cpp. But to be fair, that's the only one. The other libraries could all conceivably be public dependencies.

[aaronpuchert]

Add all dependencies from Support's LLVM_SYSTEM_LIBS as public link dependencies of libLLVM.

[aaronpuchert]

Ping.

[beanz]

Sorry, got distracted. The Z3 Solver is an external dependency that gets put on any user of libLLVM if they are building with Z3, so that's correct too.

This LGTM.

[mstorsjo]

This caused an unexpected issue in builds with dylibs enabled for me - see https://github.com/mstorsjo/llvm-mingw/runs/8296785193?check_suite_focus=true#logs.

CMake manages to find a custom-installed libzstd in a nondefault location (/home/linuxbrew/.linuxbrew/lib/libzstd.so.1.5.2), and for every other place that these dependencies are used, it's linked by just adding that absolute path to the linking command line. However after this commit, when linking, it links by passing both -lzstd and /home/linuxbrew/.linuxbrew/lib/libzstd.so.1.5.2, and linking fails since the -lzstd is unresolved.

[aaronpuchert]

In D132799#3783603, @mstorsjo wrote:

CMake manages to find a custom-installed libzstd in a nondefault location (/home/linuxbrew/.linuxbrew/lib/libzstd.so.1.5.2), and for every other place that these dependencies are used, it's linked by just adding that absolute path to the linking command line. However after this commit, when linking, it links by passing both -lzstd and /home/linuxbrew/.linuxbrew/lib/libzstd.so.1.5.2, and linking fails since the -lzstd is unresolved.

My understanding is that LLVM_SYSTEM_LIBS is also used as output of llvm-config --system-libs. Should this also print the full path? Perhaps you can check what it currently prints? Or have a look in the build directory at tools/llvm-config/BuildVariables.inc.

We likely get zstd in LLVM_SYSTEM_LIBS because lib/Support/CMakeLists.txt goes to some lengths to strip the path. But I'm not sure why.

[aaronpuchert]

Hmm, seems that since D79219 we use find_package, which has a habit of producing full paths instead of e.g. -lz for zlib, so for llvm-config we strip off the path and pretend we've been linking against the system library all along.

Seems like the easiest fix would be to stop the stripping and live with llvm-config printing a full path as well. As an alternative, we strip when the path is part of the standard linker search paths.

[aaronpuchert]

In D132799#3784103, @aaronpuchert wrote:

As an alternative, we strip when the path is part of the standard linker search paths.

This doesn't seem viable. Finding out the linker search paths requires finding out the linker (which we don't use directly, we use the compiler as linker driver), and then asking it with some flags and parse the output. Flags and output heavily depend on the linker being used.

[phosek]

I think a better solution would be to introduce imported CMake targets for system libraries like libatomic (and use the existing supports where it already exists like the threads library) and use CMake dependencies as needed.

The SYSTEM_LIBS concept in LLVM build is only used by llvm-config and I expect we'll remove it entirely once we finally manage to deprecate llvm-config.

[aaronpuchert]

In D132799#3779971, @beanz wrote:

The Z3 Solver is an external dependency that gets put on any user of libLLVM if they are building with Z3, so that's correct too.

Hmm, could you elaborate why? All I can find is

llvm/lib/Support/Z3Solver.cpp:#include <z3.h>

so that doesn't look like a public dependency to me. That's different from libatomic, which is used via llvm/ADT/Statistic.h, so users of libLLVM have to link with that directly if it's required.

Same goes for some other "system libs":

llvm/lib/Support/CRC.cpp:#include <zlib.h>
llvm/lib/Support/Compression.cpp:#include <zlib.h>

llvm/lib/Support/Compression.cpp:#include <zstd.h>

In D132799#3784308, @phosek wrote:

I think a better solution would be to introduce imported CMake targets for system libraries like libatomic (and use the existing supports where it already exists like the threads library) and use CMake dependencies as needed.

The issue isn't finding the library, but propagating it to libLLVM as public dependency, i.e. users of libLLVM will have to link with with libatomic. When it comes to linking libLLVM itself we already include it, but we don't use public dependencies on shared libraries at all:

if(ARG_STATIC)
  set(libtype PUBLIC)
else()
  # We can use PRIVATE since SO knows its dependent libs.
  set(libtype PRIVATE)
endif()

which seems like a faulty assumption to me. Directly using functionality from dependent libs still requires specifying them on the linker command line.

The SYSTEM_LIBS concept in LLVM build is only used by llvm-config and I expect we'll remove it entirely once we finally manage to deprecate llvm-config.

Perhaps SYSTEM_LIBS is also too big for this. It seems to contain a number of private dependencies. I'm thinking of reverting this change and going back to my original suggestion, but want to hear from @beanz first.

Ideally we would properly track private vs. public link dependencies, and then we could collect them from the component libraries into libLLVM. But that would be quite some work.

[danilaml]

I've experienced similar linking issues (-lz not resolved) when cross-compiling since ZLIB was initially picked from cross-build sysroot with a full path but simply using -lz is not enough to find a target zlib.

[aaronpuchert]

Go back to only adding libatomic. The LLVM_SYSTEM_LIBS contain many libraries that seem to be private dependencies only, and for now we don't properly track private vs public dependencies on component libraries.

[mstorsjo]

FWIW, here's another downstream issue relating to LLVM_SYSTEM_LIBS and how zstd behaves weirdly in it: https://github.com/msys2/MINGW-packages/issues/13108 That issue does seem to be kinda the opposite to what I experienced above, though.

[beanz]

This looks good. I think @phosek’s suggestion of imported targets would be nice too, but certainly not necessary to address the issue here.

llvm/tools/llvm-shlib/CMakeLists.txt

if (NOT MINGW)
       target_link_options(LLVM PRIVATE LINKER:-Bsymbolic-functions)
     endif()
   elseif("${CMAKE_SYSTEM_NAME}" STREQUAL "Darwin")
     set(LIB_NAMES -Wl,-all_load ${LIB_NAMES})
   endif()
 
   target_link_libraries(LLVM PRIVATE ${LIB_NAMES})
 
+  if(LLVM_ENABLE_THREADS AND NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB)
+    target_link_libraries(LLVM PUBLIC atomic)
+  endif()
+
   if (APPLE)
     set_property(TARGET LLVM APPEND_STRING PROPERTY
                 LINK_FLAGS
                 " -compatibility_version 1 -current_version ${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}")
   endif()
 
   if(TARGET libLLVMExports)
     add_dependencies(LLVM libLLVMExports)