[build] Link main executable with libpthread

[tra]

Summary

Otherwise it gets linked in by one of the dependencies of shared
libraries which may be too late and we end up with weird crashes in
std::call_once().

[• rafael]

Is that a documented restriction?

We should have a comment saying why we need to do this.

Cheers,
Rafael

[tra]

In D21478#461228, @rafael wrote:

Is that a documented restriction?

I couldn't find it stated explicitly anywhere. "add -pthread (no 'l' after dash) and magic will happen" is pretty much it.

On the other hand, it sort of makes sense.
libpthread overrides number of weak symbols in standard c library and we do want *all* symbols in our executable either use all those symbols from standard library or use all of them from libpthread. With shared liking (and lazy library loading and symbol binding?) we may end up with some symbols getting resolved from libc which is already loaded and available and others from libpthread when it gets loaded by some other shared library.

We should have a comment saying why we need to do this.

OK

[tra]

Added comments.

[• rafael]

This is fine by me. Adding Chris to check if this is the right cmake way to do it.

[beanz]

One comment below that needs attention. Otherwise LGTM.

cmake/config-ix.cmake

114 ↗

(On Diff #61129)

I'm a little concerned about the use of CMAKE_THREAD_LIBS_INIT here. That isn't guaranteed to be pthreads even if we have libpthread. On systems with other thread libraries I believe it prefers the system native library.

I think if you set CMAKE_THREAD_PREFER_PTHREAD that will force it to prefer pthreads over a system threading library.

[tra]

cmake/config-ix.cmake

114 ↗

(On Diff #61129)

Will do.

[tra]

Argh. CMAKE_THREAD_LIBS_INIT contains library name prefixed with -l. It's fine to use with target_link_libraries because it checks for prefixed names, but results in "-l-lpthread" because in some places we assume that PTHREAD_LIB is an unprefixed library name.
I'll update the patch once I know how to deal with this.

[tra]

Strip -l prefix from the library name in PTHREAD_LIB.

[tra]

Oops. Removed debugging message that should not be committed.

[beanz]

cmake/config-ix.cmake

115 ↗

(On Diff #61320)

Rather than this, try setting THREADS_PREFER_PTHREAD_FLAG. That will prefer the -pthreds flag if the compiler supports it.

Even with that you can't actually assume that the flag returned here will be -pthread instead of -lpthread because I'm pretty sure we support using pthreads library with compilers that don't support -pthread.

cmake/modules/AddLLVM.cmake

677 ↗

(On Diff #61320)

Since this would be a flag rather than a library, maybe you should change your calls to target_link_libraries to something more like this:

set_property(TARGET ${name} APPEND_STRING PROPERTY
             LINK_FLAGS "${PTHREAD_LIB}")

I'd also suggest changing PTHREAD_LIB to PTHREAD_FLAG because that is more representative of what you're actually using here.

[tra]

cmake/config-ix.cmake

115 ↗

(On Diff #61320)

That would need more changes. Currently we add PTHREAD_LIB to system_libs, which eventually ends up in LLVM_SYSTEM_LIBS property which is in turn used by llvm-config for figuring out what to print for --system-libs. The way it currently does that by prepending everything in LLVM_SYSTEM_LIBS with '-l' and that breaks with both -lpthread and -pthread.

I guess we need to check for prefixed names there and only append -l to items without leading dash. Stay tuned for updated patch.

[beanz]

cmake/config-ix.cmake

115 ↗

(On Diff #61320)

I didn't think about the extra complication, if we want to force -lpthread to be the return here to be consistent you can set THREADS_HAVE_PTHREAD_ARG=Off. That should make this safer, and you can save the added complexity of supporting -pthread for another time.

Thoughts?

[tra]

Force pthread detection to report it as -l<library>
Deal with -l<library> in llvm-config.

[tra]

cmake/config-ix.cmake

115 ↗

(On Diff #61412)

OK. We'll stick with -l<pthread variant> for now.

[beanz]

LGTM.

Thanks!
-Chris

[amadio]

llvm/trunk/tools/llvm-config/CMakeLists.txt

22

When building against system ZLIB, ${l} here resolves to /usr/lib/libz.so, which then causes the error below:

/usr/lib/gcc/x86_64-pc-linux-gnu/6.4.0/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -l/usr/lib/libz.so
collect2: error: ld returned 1 exit status

The symptom is visible in tools/llvm-config/BuildVariables.inc, where I see the following line:

#define LLVM_SYSTEM_LIBS "-lrt -ldl -lcurses -lpthread -l/usr/lib/libz.so -lm"

The correct way to link against zlib since CMake 3.1 is to use the target ZLIB::ZLIB defined in FindZLIB.cmake that comes with CMake.

[davidlt]

The following caused error while building LLVM on riscv64 system. I think, you must use -pthread and never use -lpthread. A few points:

• -pthread also modified CPP flags and defines _REENTRANT, which is required by standard;
• Based on above -pthread also needs to be in your CPP/CXX FLAGS;
• On riscv64 -pthread also implies libatomic (might change in the future).

TL;DR -pthread needs to be used and needs to be passed to compile and link flags.

llvm/trunk/cmake/config-ix.cmake

if (PTHREAD_IN_LIBC)
       check_library_exists(c pthread_mutex_lock "" HAVE_PTHREAD_MUTEX_LOCK)
     endif()
   endif()
   check_library_exists(dl dlopen "" HAVE_LIBDL)
   check_library_exists(rt clock_gettime "" HAVE_LIBRT)
 endif()
 
 if(HAVE_LIBPTHREAD)
-  set(PTHREAD_LIB pthread)
+  # We want to find pthreads library and at the moment we do want to
+  # have it reported as '-l<lib>' instead of '-pthread'.
+  # TODO: switch to -pthread once the rest of the build system can deal with it.
+  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
+  set(THREADS_HAVE_PTHREAD_ARG Off)
+  find_package(Threads REQUIRED)
+  set(PTHREAD_LIB ${CMAKE_THREAD_LIBS_INIT})
 endif()
 
 # Don't look for these libraries on Windows. Also don't look for them if we're
 # using MSan, since uninstrmented third party code may call MSan interceptors
 # like strlen, leading to false positives.
 if( NOT PURE_WINDOWS AND NOT LLVM_USE_SANITIZER MATCHES "Memory.*")
   if (LLVM_ENABLE_ZLIB)
     check_library_exists(z compress2 "" HAVE_LIBZ)

llvm/trunk/cmake/modules/AddLLVM.cmake

macro(add_llvm_executable name)
   llvm_config( ${name} ${USE_SHARED} ${LLVM_LINK_COMPONENTS} )
   if( LLVM_COMMON_DEPENDS )
     add_dependencies( ${name} ${LLVM_COMMON_DEPENDS} )
   endif( LLVM_COMMON_DEPENDS )
 
   if(NOT ARG_IGNORE_EXTERNALIZE_DEBUGINFO)
     llvm_externalize_debuginfo(${name})
   endif()
+  if (PTHREAD_LIB)
+    # libpthreads overrides some standard library symbols, so main
+    # executable must be linked with it in order to provide consistent
+    # API for all shared libaries loaded by this executable.
+    target_link_libraries(${name} ${PTHREAD_LIB})
+  endif()
 endmacro(add_llvm_executable name)
 
 function(export_executable_symbols target)
   if (LLVM_EXPORTED_SYMBOL_FILE)
     # The symbol file should contain the symbols we want the executable to
     # export
     set_target_properties(${target} PROPERTIES ENABLE_EXPORTS 1)
   elseif (LLVM_EXPORT_SYMBOLS_FOR_PLUGINS)
function(add_unittest test_suite test_name)
   endif ()
 
   set(LLVM_REQUIRES_RTTI OFF)
 
   list(APPEND LLVM_LINK_COMPONENTS Support) # gtest needs it for raw_ostream
   add_llvm_executable(${test_name} IGNORE_EXTERNALIZE_DEBUGINFO ${ARGN})
   set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR})
   set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
-  target_link_libraries(${test_name} gtest_main gtest)
+  # libpthreads overrides some standard library symbols, so main
+  # executable must be linked with it in order to provide consistent
+  # API for all shared libaries loaded by this executable.
+  target_link_libraries(${test_name} gtest_main gtest ${PTHREAD_LIB})
 
   add_dependencies(${test_suite} ${test_name})
   get_target_property(test_suite_folder ${test_suite} FOLDER)
   if (NOT ${test_suite_folder} STREQUAL "NOTFOUND")
     set_property(TARGET ${test_name} PROPERTY FOLDER "${test_suite_folder}")
   endif ()
 endfunction()
 

llvm/trunk/tools/llvm-config/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 set(BUILDVARIABLES_SRCPATH ${CMAKE_CURRENT_SOURCE_DIR}/BuildVariables.inc.in)
 set(BUILDVARIABLES_OBJPATH ${CMAKE_CURRENT_BINARY_DIR}/BuildVariables.inc)
 
 # Add the llvm-config tool.
 add_llvm_tool(llvm-config
   llvm-config.cpp
   )
 
 # Compute the substitution values for various items.
 get_property(LLVM_SYSTEM_LIBS_LIST TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS)
 foreach(l ${LLVM_SYSTEM_LIBS_LIST})
   if(MSVC)
     set(SYSTEM_LIBS ${SYSTEM_LIBS} "${l}.lib")
   else()
+    if (l MATCHES "^-")
+      # If it's an option, pass it without changes.
+      set(SYSTEM_LIBS ${SYSTEM_LIBS} "${l}")
+    else()
+      # Otherwise assume it's a library name we need to link with.
       set(SYSTEM_LIBS ${SYSTEM_LIBS} "-l${l}")
     endif()
+  endif()
 endforeach()
 string(REPLACE ";" " " SYSTEM_LIBS "${SYSTEM_LIBS}")
 
 # Fetch target specific compile options, e.g. RTTI option
 get_property(COMPILE_FLAGS TARGET llvm-config PROPERTY COMPILE_FLAGS)
 
 # Use configure_file to create BuildVariables.inc.
 set(LLVM_SRC_ROOT ${LLVM_MAIN_SRC_DIR})