Page MenuHomePhabricator

Fix llvm-config --system-libs output on FreeBSD and NetBSD
ClosedPublic

Authored by dim on Jan 30 2018, 12:10 PM.

Details

Summary

For various reasons, CMake's detection mechanism for backtrace() returns an absolute path /usr/lib/libexecinfo.so on FreeBSD and NetBSD.

Since tools/llvm-config/CMakeLists.txt only checks if system libraries start with -, this causes llvm-config --system-libs to produce the following incorrect output:

-lrt -l/usr/lib/libexecinfo.so -ltinfo -lpthread -lz -lm

Fix it by checking for both - and /, assuming the latter indicates an absolute path to a library.

This also fixes the Bindings/Go/go.test test case, since that always died with "unable to find library -l/usr/lib/libexecinfo.so".

Diff Detail

Repository
rL LLVM

Event Timeline

dim created this revision.Jan 30 2018, 12:10 PM

-l/usr/lib/libexecinfo.so this should be replaced with -lexecinfo.

Does it work this way with this patch?

dim added a comment.Feb 1 2018, 10:18 AM

-l/usr/lib/libexecinfo.so this should be replaced with -lexecinfo.

As I said, this is something detected by CMake, not LLVM's scripts. We could attempt to submit a fix to CMake, but that will take quite some time to arrive, and some vendor versions (such as Red Hat's or Debian's) will not be fixed for years, if ever.

Does it work this way with this patch?

What this patch does, is pass through absolute library paths (even if those might be less desirable) through, unmodified, so programs using llvm-config --system-libs will not fail to link.

In D42702#995106, @dim wrote:

-l/usr/lib/libexecinfo.so this should be replaced with -lexecinfo.

As I said, this is something detected by CMake, not LLVM's scripts. We could attempt to submit a fix to CMake, but that will take quite some time to arrive, and some vendor versions (such as Red Hat's or Debian's) will not be fixed for years, if ever.

This means that we need to implement check manually, in worst case hardcode it under the FreeBSD|NetBSD OS.

Does it work this way with this patch?

What this patch does, is pass through absolute library paths (even if those might be less desirable) through, unmodified, so programs using llvm-config --system-libs will not fail to link.

static linking is still broken

dim added a comment.Feb 1 2018, 10:40 AM
In D42702#995106, @dim wrote:

-l/usr/lib/libexecinfo.so this should be replaced with -lexecinfo.

As I said, this is something detected by CMake, not LLVM's scripts. We could attempt to submit a fix to CMake, but that will take quite some time to arrive, and some vendor versions (such as Red Hat's or Debian's) will not be fixed for years, if ever.

This means that we need to implement check manually, in worst case hardcode it under the FreeBSD|NetBSD OS.

As a gross hack, we can put in a special case for s#/usr/lib/libexecinfo.so##-lexecinfo#. But I'd rather trust what CMake gives us.

Does it work this way with this patch?

What this patch does, is pass through absolute library paths (even if those might be less desirable) through, unmodified, so programs using llvm-config --system-libs will not fail to link.

static linking is still broken

Huh, why? (Not that I ever try to statically link...) I would expect CMake to give /usr/lib/libexecinfo.a in that case?

$ llvm-config --link-static --system-libs
-lz -lrt -l/usr/lib/libexecinfo.so -lterminfo -lpthread -lm
dim added a comment.Feb 1 2018, 11:31 AM

Hmm, apparently CMake *always* returns a full path for find_library(), but we seem to ignore those, by doing (for example in libclang's CMakeLists.txt):

find_library(DL_LIBRARY_PATH dl)
if (DL_LIBRARY_PATH)
  list(APPEND LIBS dl)
endif()

The FindBacktrace.cmake module is a little different, though, since it can return either a custom define, an empty string, or either of libexecinfo or libbacktrace, depending on the OS.

Translating this into a -l option must probably be done manually, and it is likely better to do it in lib/Support/CMakeLists.txt, or even cmake/config-ix.cmake. Any suggestions?

I propose to stop using FindBacktrace.cmake and replace it with a manual check.

It would be nice to get a fix in for this, I even ran into the problem compiling Rust on FreeBSD.

bdrewery added a comment.EditedFeb 23 2018, 4:06 PM

I'm not sure this even works as now I get: -llibexecinfo.so. Although this may be some rust-specific issue. I am not yet sure.

This fixes it for me in the spirit of other library lookups (in that it doesn't bother with -L, but that could easily be added in). Despite Backtrace_LIBRARIES being plural I don't think it ever will be looking over FindBacktrace.cmake.

I have no clue about static linkage here.

https://people.freebsd.org/~bdrewery/patches/cmake-libexecinfo.diff

diff --git lib/Support/CMakeLists.txt lib/Support/CMakeLists.txt
index 5723f8fcf5b..dfbdd70701f 100644
--- lib/Support/CMakeLists.txt
+++ lib/Support/CMakeLists.txt
@@ -13,7 +13,9 @@ elseif( CMAKE_HOST_UNIX )
     set(system_libs ${system_libs} ${CMAKE_DL_LIBS})
   endif()
   if( HAVE_BACKTRACE )
-    set(system_libs ${system_libs} ${Backtrace_LIBRARIES})
+    get_filename_component(Backtrace_LIBFILE ${Backtrace_LIBRARIES} NAME_WE)
+    STRING(REGEX REPLACE "^lib" "" Backtrace_LIBFILE ${Backtrace_LIBFILE})
+    set(system_libs ${system_libs} ${Backtrace_LIBFILE})
   endif()
   if(LLVM_ENABLE_TERMINFO)
     if(HAVE_TERMINFO)

This fixes it for me in the spirit of other library lookups (in that it doesn't bother with -L, but that could easily be added in). Despite Backtrace_LIBRARIES being plural I don't think it ever will be looking over FindBacktrace.cmake.

I have no clue about static linkage here.

https://people.freebsd.org/~bdrewery/patches/cmake-libexecinfo.diff

diff --git lib/Support/CMakeLists.txt lib/Support/CMakeLists.txt
index 5723f8fcf5b..dfbdd70701f 100644
--- lib/Support/CMakeLists.txt
+++ lib/Support/CMakeLists.txt
@@ -13,7 +13,9 @@ elseif( CMAKE_HOST_UNIX )
     set(system_libs ${system_libs} ${CMAKE_DL_LIBS})
   endif()
   if( HAVE_BACKTRACE )
-    set(system_libs ${system_libs} ${Backtrace_LIBRARIES})
+    get_filename_component(Backtrace_LIBFILE ${Backtrace_LIBRARIES} NAME_WE)
+    STRING(REGEX REPLACE "^lib" "" Backtrace_LIBFILE ${Backtrace_LIBFILE})
+    set(system_libs ${system_libs} ${Backtrace_LIBFILE})
   endif()
   if(LLVM_ENABLE_TERMINFO)
     if(HAVE_TERMINFO)

What's the result of:

$ llvm-config --link-static --system-libs

This fixes it for me in the spirit of other library lookups (in that it doesn't bother with -L, but that could easily be added in). Despite Backtrace_LIBRARIES being plural I don't think it ever will be looking over FindBacktrace.cmake.

I have no clue about static linkage here.

https://people.freebsd.org/~bdrewery/patches/cmake-libexecinfo.diff

diff --git lib/Support/CMakeLists.txt lib/Support/CMakeLists.txt
index 5723f8fcf5b..dfbdd70701f 100644
--- lib/Support/CMakeLists.txt
+++ lib/Support/CMakeLists.txt
@@ -13,7 +13,9 @@ elseif( CMAKE_HOST_UNIX )
     set(system_libs ${system_libs} ${CMAKE_DL_LIBS})
   endif()
   if( HAVE_BACKTRACE )
-    set(system_libs ${system_libs} ${Backtrace_LIBRARIES})
+    get_filename_component(Backtrace_LIBFILE ${Backtrace_LIBRARIES} NAME_WE)
+    STRING(REGEX REPLACE "^lib" "" Backtrace_LIBFILE ${Backtrace_LIBFILE})
+    set(system_libs ${system_libs} ${Backtrace_LIBFILE})
   endif()
   if(LLVM_ENABLE_TERMINFO)
     if(HAVE_TERMINFO)

What's the result of:

$ llvm-config --link-static --system-libs

I'm only testing from the bundled LLVM 6 in rust:

~/git/rust/rust # build/x86_64-unknown-freebsd/llvm/bin/llvm-config --system-libs
-lrt -lexecinfo -lpthread -lm
~/git/rust/rust # build/x86_64-unknown-freebsd/llvm/bin/llvm-config --link-static --system-libs
-lrt -lexecinfo -lpthread -lm
~/git/rust/rust # uname -sr
FreeBSD 12.0-CURRENT

This looks good to me!

141 kamil@chieftec /public/llvm-build $ ./bin/llvm-config --link-static --system-libs                                                
-lz -lrt -lexecinfo -lterminfo -lpthread -lm
142 kamil@chieftec /public/llvm-build $ ./bin/llvm-config --system-libs               
-lz -lrt -lexecinfo -lterminfo -lpthread -lm
143 kamil@chieftec /public/llvm-build $ uname -rms
NetBSD 8.99.12 amd64

I think that this should be a release blocker for 6.0.0. CC: @hans

hans added a comment.Feb 26 2018, 6:28 AM

I think that this should be a release blocker for 6.0.0. CC: @hans

I'm sorry, I think it's too late. I'll put it on the list for 6.0.1.

dim updated this revision to Diff 136296.Feb 28 2018, 8:04 AM

Update using @bdrewery's suggestion, with one small fix for Linux, where
Backtrace_LIBRARIES is usually an empty string.

Resulting output on FreeBSD:

$ ./bin/llvm-config --system-libs
-lz -lrt -lexecinfo -ltinfo -lpthread -lm
$ ./bin/llvm-config --system-libs --link-static
-lz -lrt -lexecinfo -ltinfo -lpthread -lm

On Ubuntu 17.10:

$ ./bin/llvm-config --system-libs
-lz -lrt -ldl -ltinfo -lpthread -lm
$ ./bin/llvm-config --system-libs --link-static
-lz -lrt -ldl -ltinfo -lpthread -lm
krytarowski accepted this revision.Feb 28 2018, 9:35 AM

I would add an inline comment what's the purpose of this operation in CMake. Otherwise it looks good to me!

This revision is now accepted and ready to land.Feb 28 2018, 9:35 AM
This revision was automatically updated to reflect the committed changes.

While there. There is feature in Clang/LLVM to print stacktrace on a crash. This is unusable on BSDs, as it prints signal handler's stacktrace instead of the program one. So regardless of cargo culting backtrace(3) it does not work as intended.

I'm not aware what's the proper solution to this, how to print the backtrace beyond the signal trampoline... are you interested to have a look? @bdrewery / @dim