This is an archive of the discontinued LLVM Phabricator instance.

Differential D42702

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

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

Details

Reviewers
chandlerc
emaste
joerg
krytarowski
Commits
rGfa5423e8242d: Merging r326358:
rGa73ed459bf14: Fix llvm-config --system-libs output on FreeBSD and NetBSD
rL333579: Merging r326358:
rL326358: Fix llvm-config --system-libs output on FreeBSD and NetBSD
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

Event Timeline

dim created this revision.Jan 30 2018, 12:10 PM
Harbormaster completed remote builds in B14414: Diff 132026.Jan 30 2018, 12:10 PM
Herald added subscribers: llvm-commits, hintonda, mgorny. · View Herald TranscriptJan 30 2018, 12:10 PM
krytarowski added a comment.Feb 1 2018, 8:48 AM

-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
In D42702#994985, @krytarowski 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.

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.

krytarowski added a comment.Feb 1 2018, 10:23 AM
In D42702#995106, @dim wrote:
In D42702#994985, @krytarowski 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#995109, @krytarowski wrote:
In D42702#995106, @dim wrote:
In D42702#994985, @krytarowski 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?

krytarowski added a comment.Feb 1 2018, 10:51 AM
$ 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?

krytarowski added a comment.Feb 1 2018, 11:37 AM

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

bdrewery added a subscriber: bdrewery.Feb 23 2018, 3:24 PM

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.

bdrewery added a comment.Feb 23 2018, 5:09 PM

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)
krytarowski added a comment.Feb 23 2018, 5:17 PM
In D42702#1018130, @bdrewery wrote:

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
bdrewery added a comment.Feb 23 2018, 6:16 PM
In D42702#1018139, @krytarowski wrote:
In D42702#1018130, @bdrewery wrote:

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
krytarowski added a comment.Feb 24 2018, 2:59 AM

This looks good to me!

krytarowski added a comment.Feb 24 2018, 3:33 AM
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
krytarowski added a subscriber: hans.Feb 26 2018, 4:04 AM

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
In D42702#1019141, @krytarowski wrote:

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
Harbormaster completed remote builds in B15519: Diff 136296.Feb 28 2018, 8:04 AM
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
Closed by commit rL326358: Fix llvm-config --system-libs output on FreeBSD and NetBSD (authored by dim). · Explain WhyFeb 28 2018, 12:06 PM
This revision was automatically updated to reflect the committed changes.
krytarowski added a comment.Mar 7 2018, 10:57 AM

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

Revision Contents

PathSize
tools/
llvm-config/
4 lines

Diff 132026

tools/llvm-config/CMakeLists.txt

set(LLVM_LINK_COMPONENTS support) set(LLVM_LINK_COMPONENTS support)
set(BUILDVARIABLES_SRCPATH ${CMAKE_CURRENT_SOURCE_DIR}/BuildVariables.inc.in) set(BUILDVARIABLES_SRCPATH ${CMAKE_CURRENT_SOURCE_DIR}/BuildVariables.inc.in)
set(BUILDVARIABLES_OBJPATH ${CMAKE_CURRENT_BINARY_DIR}/BuildVariables.inc) set(BUILDVARIABLES_OBJPATH ${CMAKE_CURRENT_BINARY_DIR}/BuildVariables.inc)
# Add the llvm-config tool. # Add the llvm-config tool.
add_llvm_tool(llvm-config add_llvm_tool(llvm-config
llvm-config.cpp llvm-config.cpp
) )
# Compute the substitution values for various items. # Compute the substitution values for various items.
get_property(LLVM_SYSTEM_LIBS_LIST TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS) get_property(LLVM_SYSTEM_LIBS_LIST TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS)
foreach(l ${LLVM_SYSTEM_LIBS_LIST}) foreach(l ${LLVM_SYSTEM_LIBS_LIST})
if(MSVC) if(MSVC)
set(SYSTEM_LIBS ${SYSTEM_LIBS} "${l}.lib") set(SYSTEM_LIBS ${SYSTEM_LIBS} "${l}.lib")
else() else()
if (l MATCHES "^-") if (l MATCHES "^[-/]")
# If it's an option, pass it without changes. # If it's an option, or an absolute pathname, pass it without changes.
set(SYSTEM_LIBS ${SYSTEM_LIBS} "${l}") set(SYSTEM_LIBS ${SYSTEM_LIBS} "${l}")
else() else()
# Otherwise assume it's a library name we need to link with. # Otherwise assume it's a library name we need to link with.
set(SYSTEM_LIBS ${SYSTEM_LIBS} "-l${l}") set(SYSTEM_LIBS ${SYSTEM_LIBS} "-l${l}")
endif() endif()
endif() endif()
endforeach() endforeach()
string(REPLACE ";" " " SYSTEM_LIBS "${SYSTEM_LIBS}") string(REPLACE ";" " " SYSTEM_LIBS "${SYSTEM_LIBS}")
▲ Show 20 Lines • Show All 54 Lines • Show Last 20 Lines