Don't use -ldl on BSD

[dim]

Summary

While testing for the 6.0.0 release candidates, I had many link failures during the build of the test-suite, similar to:

[ 78%] Linking CXX executable simd_ops_test_op_pmulld_275
Scanning dependencies of target simd_ops_test_op_vpmulld_390
/usr/local/bin/ld: cannot find -ldl

This is because some of the Bitcode tests unconditionally add -ldl to LDFLAGS. On FreeBSD, NetBSD and OpenBSD there is no libdl, as the required functionality is already built into libc.

Fix it by only adding -ldl for non-BSD systems.

[krytarowski]

Can we use CMAKE_DL_LIBS?

[dim]

In D43168#1004532, @krytarowski wrote:

Can we use CMAKE_DL_LIBS?

We could, but this CMakeLists.txt does not really seem to care about such niceties; I don't see the added value of the complexity. What would we gain by using CMAKE_DL_LIBS?

[dim]

One other problem is that CMAKE_DL_LIBS is meant for adding to cmake targets, e.g. it only contains the string "dl" on Linux. This means that you would have to add some sort of transformation to change it into "-ldl"... Way too complicated, if you ask me. :)

[krytarowski]

CMAKE_DL_LIBS allows us to fix it once for everybody and forever.

CMAKE_DL_LIBS is already used in llvm, e.g.

if( HAVE_LIBDL )
  set(system_libs ${system_libs} ${CMAKE_DL_LIBS})
endif()

in llvm/lib/Support/CMakeLists.txt

I don't want to repeat this process for AIX, SmartOS (they are now migrating to Clang at least for pkgsrc) etc.

[dim]

In D43168#1004536, @krytarowski wrote:

CMAKE_DL_LIBS allows us to fix it once for everybody and forever.

CMAKE_DL_LIBS is already used in llvm, e.g.

if( HAVE_LIBDL )
  set(system_libs ${system_libs} ${CMAKE_DL_LIBS})
endif()

in llvm/lib/Support/CMakeLists.txt

I don't want to repeat this process for AIX, SmartOS (they are now migrating to Clang at least for pkgsrc) etc.

Hm, well my CMake knowledge is limited, so if you could show me how to do this, please let me know. Otherwise I will have to drop this revision, and keep on patching manually :(

[dim]

It looks like test-suite has some sort of special handling for test executables, where it uses LDFLAGS instead of target_link_libraries():

# Creates a new executable build target. Use this instead of `add_executable`.
# It applies CFLAGS, CPPFLAGS, CXXFLAGS and LDFLAGS. Creates a .test file if
# necessary, registers the target with the TEST_SUITE_TARGETS list and makes
# sure we build the required dependencies for compiletime measurements
# and support the TEST_SUITE_PROFILE_USE mode.
macro(llvm_test_executable target)
  add_executable(${target} ${ARGN})
  append_target_flags(COMPILE_FLAGS ${target} ${CFLAGS})
  append_target_flags(COMPILE_FLAGS ${target} ${CPPFLAGS})
  append_target_flags(COMPILE_FLAGS ${target} ${CXXFLAGS})
  # Note that we cannot use target_link_libraries() here because that one
  # only interprets inputs starting with '-' as flags.
  append_target_flags(LINK_LIBRARIES ${target} ${LDFLAGS})

I am unsure what the reason is that add_executable and target_link_libraries cannot be used. So maybe there is an easy way in CMake to transform a list of libraries in the form of e.g. CMAKE_DL_LIBS to linker flags? Or must we do that manually with some sort of for loop?

[krytarowski]

Something along these lines should do the trick:

foreach(lib ${CMAKE_DL_LIBS})
  list(APPEND LDFLAGS -l${lib})
endforeach()

CMAKE_DL_LIBS can contain 0-2 elements on existing OSes. For NetBSD it's 0, for GNU/Linux it's 1, for Haiku it's 2.

[krytarowski]

I've tested this in a hello world application on NetBSD and a remote Linux shell and it worked for me.

[dim]

Update to use a for loop for iterating over CMAKE_DL_LIBS, and while we are here, also use CMake's FindThreads module and CMAKE_THREAD_LIBS_INIT (this will contain -lpthread, if it is necessary for the target system).

[krytarowski]

Please update description.

[asbirlea]

FWIW, LGTM. Thank you for the patch!

[MatzeB]

The CMAKE_DL_LIBS change LGTM.

and while we are here, also use CMake's FindThreads module and CMAKE_THREAD_LIBS_INIT

Note that there are a bunch more places in the test-suite that just use -pthread instead of $(CMAKE_THREAD_LIBS_INIT) so now we are inconsistent...

In D43168#1004539, @dim wrote:

It looks like test-suite has some sort of special handling for test executables, where it uses LDFLAGS instead of target_link_libraries():

# Creates a new executable build target. Use this instead of `add_executable`.
# It applies CFLAGS, CPPFLAGS, CXXFLAGS and LDFLAGS. Creates a .test file if
# necessary, registers the target with the TEST_SUITE_TARGETS list and makes
# sure we build the required dependencies for compiletime measurements
# and support the TEST_SUITE_PROFILE_USE mode.
macro(llvm_test_executable target)
  add_executable(${target} ${ARGN})
  append_target_flags(COMPILE_FLAGS ${target} ${CFLAGS})
  append_target_flags(COMPILE_FLAGS ${target} ${CPPFLAGS})
  append_target_flags(COMPILE_FLAGS ${target} ${CXXFLAGS})
  # Note that we cannot use target_link_libraries() here because that one
  # only interprets inputs starting with '-' as flags.
  append_target_flags(LINK_LIBRARIES ${target} ${LDFLAGS})

I am unsure what the reason is that add_executable and target_link_libraries cannot be used. So maybe there is an easy way in CMake to transform a list of libraries in the form of e.g. CMAKE_DL_LIBS to linker flags? Or must we do that manually with some sort of for loop?

I think I did this because annoyingly target_link_libraries() interprets everything not starting with a - as a library name so it failed for -Xlinker -stack_size -Xlinker 0x800000 :-/

[krytarowski]

For such options there are designed other options, like CMAKE_EXE_LINKER_FLAGS. target_link_libraries() should be used for libraries.

[MatzeB]

In D43168#1005292, @krytarowski wrote:

For such options there are designed other options, like CMAKE_EXE_LINKER_FLAGS. target_link_libraries() should be used for libraries.

Well the target_link_libraries() docu says "Specify libraries or flags to use when linking a given target." and there is no target_exe_linker_flags() for that matter. Also at least in principle instead of EXE_ we would need the STATIC_, SHARED_ variant in some cases.

I'm certainly not proud of how the test-suite uses cmake and am happy to accept patches as long as they work.

[MatzeB]

@asbirlea: Why is -dl and -pthread needed at all? Glancing at the code I don't see any reference to pthreads or dlopen/dlsym...

[dim]

In D43168#1005331, @MatzeB wrote:

@asbirlea: Why is -dl and -pthread needed at all? Glancing at the code I don't see any reference to pthreads or dlopen/dlsym...

Hmm that is a good one, indeed! I have locally tried deleting the blocks that add -lpthread and -ldl, and at least on Linux, the programs in question all appear to link just fine.

On FreeBSD, I do get errors about pthread functions, though:

[789/4485] Linking CXX executable Bitcode/Benchmarks/Halide/bilateral_grid/halide_bilateral_grid
FAILED: Bitcode/Benchmarks/Halide/bilateral_grid/halide_bilateral_grid
: && /home/dim/obj/test-suite/build/tools/timeit --summary Bitcode/Benchmarks/Halide/bilateral_grid/halide_bilateral_grid.link.time /share/dim/llvm/324090-rel60-freebsd12-amd64-ninja-rel-1/bin/clang++  -O3 -DNDEBUG   Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/driver.cpp.o Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/__/common/x86_halide_runtime.bc.o Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/bilateral_grid.bc.o  -o Bitcode/Benchmarks/Halide/bilateral_grid/halide_bilateral_grid   && :
Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/__/common/x86_halide_runtime.bc.o: In function `halide_spawn_thread':
posix_allocator.cpp:(.text+0x3ab): undefined reference to `pthread_create'
clang-6.0: error: linker command failed with exit code 1 (use -v to see invocation)
[790/4485] Linking CXX executable Bitcode/Regression/vector_widen/widen_bug
FAILED: Bitcode/Regression/vector_widen/widen_bug
: && /home/dim/obj/test-suite/build/tools/timeit --summary Bitcode/Regression/vector_widen/widen_bug.link.time /share/dim/llvm/324090-rel60-freebsd12-amd64-ninja-rel-1/bin/clang++  -O3 -DNDEBUG   Bitcode/Regression/vector_widen/CMakeFiles/widen_bug.dir/driver.cpp.o Bitcode/Regression/vector_widen/CMakeFiles/widen_bug.dir/halide_runtime.bc.o Bitcode/Regression/vector_widen/CMakeFiles/widen_bug.dir/vector_widen.bc.o  -o Bitcode/Regression/vector_widen/widen_bug   && :
Bitcode/Regression/vector_widen/CMakeFiles/widen_bug.dir/halide_runtime.bc.o: In function `Halide::Runtime::Internal::default_do_par_for(void*, int (*)(void*, int, unsigned char*), int, int, unsigned char*)':
simd_op_check_runtime.ll:(.text+0x1dd): undefined reference to `pthread_create'
Bitcode/Regression/vector_widen/CMakeFiles/widen_bug.dir/halide_runtime.bc.o: In function `halide_spawn_thread':
simd_op_check_runtime.ll:(.text+0x651): undefined reference to `pthread_create'
clang-6.0: error: linker command failed with exit code 1 (use -v to see invocation)
[794/4485] Building CXX object Bitcode/Benchmarks/Halide/local_laplacian/CMakeFiles/halide_local_laplacian.dir/local_laplacian.bc.o
ninja: build stopped: subcommand failed.

Apparently there is a halide_spawn_thread function in the bitcode. I am unsure if there is any source for that bitcode?

[MatzeB]

In D43168#1005380, @dim wrote:

In D43168#1005331, @MatzeB wrote:

@asbirlea: Why is -dl and -pthread needed at all? Glancing at the code I don't see any reference to pthreads or dlopen/dlsym...

Hmm that is a good one, indeed! I have locally tried deleting the blocks that add -lpthread and -ldl, and at least on Linux, the programs in question all appear to link just fine.

On FreeBSD, I do get errors about pthread functions, though:

[789/4485] Linking CXX executable Bitcode/Benchmarks/Halide/bilateral_grid/halide_bilateral_grid
FAILED: Bitcode/Benchmarks/Halide/bilateral_grid/halide_bilateral_grid
: && /home/dim/obj/test-suite/build/tools/timeit --summary Bitcode/Benchmarks/Halide/bilateral_grid/halide_bilateral_grid.link.time /share/dim/llvm/324090-rel60-freebsd12-amd64-ninja-rel-1/bin/clang++  -O3 -DNDEBUG   Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/driver.cpp.o Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/__/common/x86_halide_runtime.bc.o Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/bilateral_grid.bc.o  -o Bitcode/Benchmarks/Halide/bilateral_grid/halide_bilateral_grid   && :
Bitcode/Benchmarks/Halide/bilateral_grid/CMakeFiles/halide_bilateral_grid.dir/__/common/x86_halide_runtime.bc.o: In function `halide_spawn_thread':
posix_allocator.cpp:(.text+0x3ab): undefined reference to `pthread_create'
clang-6.0: error: linker command failed with exit code 1 (use -v to see invocation)
[790/4485] Linking CXX executable Bitcode/Regression/vector_widen/widen_bug
FAILED: Bitcode/Regression/vector_widen/widen_bug
: && /home/dim/obj/test-suite/build/tools/timeit --summary Bitcode/Regression/vector_widen/widen_bug.link.time /share/dim/llvm/324090-rel60-freebsd12-amd64-ninja-rel-1/bin/clang++  -O3 -DNDEBUG   Bitcode/Regression/vector_widen/CMakeFiles/widen_bug.dir/driver.cpp.o Bitcode/Regression/vector_widen/CMakeFiles/widen_bug.dir/halide_runtime.bc.o Bitcode/Regression/vector_widen/CMakeFiles/widen_bug.dir/vector_widen.bc.o  -o Bitcode/Regression/vector_widen/widen_bug   && :
Bitcode/Regression/vector_widen/CMakeFiles/widen_bug.dir/halide_runtime.bc.o: In function `Halide::Runtime::Internal::default_do_par_for(void*, int (*)(void*, int, unsigned char*), int, int, unsigned char*)':
simd_op_check_runtime.ll:(.text+0x1dd): undefined reference to `pthread_create'
Bitcode/Regression/vector_widen/CMakeFiles/widen_bug.dir/halide_runtime.bc.o: In function `halide_spawn_thread':
simd_op_check_runtime.ll:(.text+0x651): undefined reference to `pthread_create'
clang-6.0: error: linker command failed with exit code 1 (use -v to see invocation)
[794/4485] Building CXX object Bitcode/Benchmarks/Halide/local_laplacian/CMakeFiles/halide_local_laplacian.dir/local_laplacian.bc.o
ninja: build stopped: subcommand failed.

Apparently there is a halide_spawn_thread function in the bitcode. I am unsure if there is any source for that bitcode?

Ah indeed that is where they are referenced. Seems like grep didn't find the references in a .bc file.

It worked anyway on linux (and mac for that matter) because libpthread and libdl are usually just empty libraries there because the functionality has been moved to libc (or libSystem)...

[dim]

In D43168#1005380, @dim wrote:

Apparently there is a halide_spawn_thread function in the bitcode. I am unsure if there is any source for that bitcode?

Indeed, pthread_create is called here:

https://github.com/halide/Halide/blob/master/src/runtime/posix_threads.cpp#L54

and dlopen here:

https://github.com/halide/Halide/blob/master/src/runtime/posix_get_symbol.cpp#L17

[krytarowski]

NetBSD's libc has stubs for threading functions (weak references), but it cannot work as long as there is no linking with -lpthread (with GCC syntax: -pthread).
The -lpthread form shouldn't be used as is, in FreeBSD the threading function is called something like -lthr (in NetBSD it's called libpthread.{so,a}).
-lstdc++ on NetBSD is required to get linked with -lpthread in order to get threading functions functional (without them, there is abort with a message).

Regarding refactoring CMake's usage, I'm not sure it's worth the churn to move things around unless we address real bugs.

[emaste]

In FreeBSD libpthread.so is a symlink to libthr; -pthread and -lpthread should both work.

% readelf -d /usr/lib/libpthread.so | grep SONAME
 0x000000000000000e SONAME               Library soname: [libthr.so.3]

[krytarowski]

-pthreads defines additional OS specific flags like _REENTRANT or _PTHREADS and this is the form that should be used.

[MatzeB]

In D43168#1012849, @krytarowski wrote:

-pthreads defines additional OS specific flags like _REENTRANT or _PTHREADS and this is the form that should be used.

+1 for using -pthread which should do the right thing for platforms supported by clang/gcc.

Regarding refactoring CMake's usage, I'm not sure it's worth the churn to move things around unless we address real bugs.

Well the rest of the test-suite currently uses -pthread while this switches to find_package(Threads) list(APPEND LDFLAGS ${CMAKE_THREAD_LIBS_INIT}) making things inconsistent with the rest (without addressing real bugs either :)

[krytarowski]

If I read the CMake's source code correctly, just including find_package(Threads) is enough to set OS specific flags.

https://github.com/Kitware/CMake/blob/9a509099f77ed32a0845e4e3fad7b8f1eb9be10b/Modules/FindThreads.cmake

[krytarowski]

In D43168#1012853, @krytarowski wrote:

If I read the CMake's source code correctly, just including find_package(Threads) is enough to set OS specific flags.

https://github.com/Kitware/CMake/blob/9a509099f77ed32a0845e4e3fad7b8f1eb9be10b/Modules/FindThreads.cmake

Or perhaps not. I agree that it's better to go for -pthread for now, at least for consistency with existing code.

Bitcode/Benchmarks/Halide/CMakeLists.txt

 if(NOT WIN32)
-  list(APPEND LDFLAGS -lpthread -ldl)
+  list(APPEND LDFLAGS -lpthread)
+  if(NOT ("${CMAKE_SYSTEM_NAME}" MATCHES "(Free|Net|Open)BSD"))
+    list(APPEND LDFLAGS -ldl)
+  endif()
 endif()
 if(NOT MSVC)
   list(APPEND CXXFLAGS "-std=c++11")
 endif()
 
 macro(test_img_input img)
   set(imgpath "${CMAKE_CURRENT_SOURCE_DIR}/../images/${img}")
   llvm_test_run(${imgpath}.bytes ${ARGN}

Bitcode/Regression/fft/CMakeLists.txt

 set(NO_REFERENCE_OUTPUT true)
 file(GLOB uosources ${CMAKE_CURRENT_SOURCE_DIR}/modules/*.bc)
 SET_SOURCE_FILES_PROPERTIES(${uosources} PROPERTIES LANGUAGE CXX)
 SET_SOURCE_FILES_PROPERTIES(${CMAKE_CURRENT_SOURCE_DIR}/x86_17.06.19_halide_runtime.bc PROPERTIES LANGUAGE CXX)
 
-list(APPEND LDFLAGS -lpthread -ldl)
+list(APPEND LDFLAGS -lpthread)
+if(NOT ("${CMAKE_SYSTEM_NAME}" MATCHES "(Free|Net|Open)BSD"))
+  list(APPEND LDFLAGS -ldl)
+endif()
 
 set(Source ${CMAKE_CURRENT_SOURCE_DIR}/driver.cpp ${CMAKE_CURRENT_SOURCE_DIR}/x86_17.06.19_halide_runtime.bc ${uosources})
 set(PROG fft)
 llvm_multisource()
 
 

Bitcode/Regression/vector_widen/CMakeLists.txt

 SET_SOURCE_FILES_PROPERTIES(${CMAKE_CURRENT_SOURCE_DIR}/halide_runtime.bc PROPERTIES LANGUAGE CXX)
 SET_SOURCE_FILES_PROPERTIES(${CMAKE_CURRENT_SOURCE_DIR}/vector_widen.bc PROPERTIES LANGUAGE CXX)
 
-list(APPEND LDFLAGS -lpthread -ldl)
+list(APPEND LDFLAGS -lpthread)
+if(NOT ("${CMAKE_SYSTEM_NAME}" MATCHES "(Free|Net|Open)BSD"))
+  list(APPEND LDFLAGS -ldl)
+endif()
 
 set(Source ${CMAKE_CURRENT_SOURCE_DIR}/driver.cpp ${CMAKE_CURRENT_SOURCE_DIR}/halide_runtime.bc ${CMAKE_CURRENT_SOURCE_DIR}/vector_widen.bc)
 set(PROG widen_bug)
 llvm_multisource()
 
 

Bitcode/simd_ops/CMakeLists.txt

 set(NO_REFERENCE_OUTPUT true)
 file(GLOB uosources ${CMAKE_CURRENT_SOURCE_DIR}/${ARCH}_tests/*.bc)
 SET_SOURCE_FILES_PROPERTIES(${uosources} PROPERTIES LANGUAGE CXX)
 file(GLOB scalar_sources ${CMAKE_CURRENT_SOURCE_DIR}/${ARCH}_scalar_tests/*.bc)
 SET_SOURCE_FILES_PROPERTIES(${scalar_sources} PROPERTIES LANGUAGE CXX)
 SET_SOURCE_FILES_PROPERTIES(${CMAKE_CURRENT_SOURCE_DIR}/${ARCH}_halide_runtime.bc PROPERTIES LANGUAGE CXX)
 
-list(APPEND LDFLAGS -lpthread -ldl)
+list(APPEND LDFLAGS -lpthread)
+if(NOT ("${CMAKE_SYSTEM_NAME}" MATCHES "(Free|Net|Open)BSD"))
+  list(APPEND LDFLAGS -ldl)
+endif()
 add_library(simd_ops STATIC simd_ops.cpp ${ARCH}_halide_runtime.bc)
 target_link_libraries(simd_ops)
 test_suite_add_build_dependencies(simd_ops)
 set_target_properties(simd_ops PROPERTIES LINKER_LANGUAGE CXX)
 
 foreach(sourcebc ${uosources})
   string(REGEX REPLACE ".[cp]+$" "" pathbc ${sourcebc})
   string(REGEX REPLACE ".*/" "" namebc ${pathbc})
   string(REPLACE "." "" namebc ${namebc})
   set(Source ${CMAKE_CURRENT_SOURCE_DIR}/${ARCH}_halide_runtime.bc ${CMAKE_CURRENT_SOURCE_DIR}/${ARCH}_tests/${namebc}.bc ${CMAKE_CURRENT_SOURCE_DIR}/${ARCH}_scalar_tests/scalar_${namebc}.bc)
   set(PROG simd_ops_${namebc})
   llvm_multisource(TARGET_VAR _target)
   target_link_libraries(${_target} simd_ops)
 endforeach()