[cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)

[labath]

Summary

This patch consists of two relatively independent changes:

• first, I teach the standalone build to detect the case when llvm was built in the shared mode and set the appropriate cmake variables to make the add_llvm_*** family of functions link against the shared library. Without these, the we would end up linking against the constituent .a files while other parts of the build (e.g. clang) would pull in the libllvm.so, which resulted in multiply defined symbols.

• second, I add detection code for pthread and dl libraries. This is necessary, because we have direct calls to these libraries (instead of going through llvm) and in the standalone build we cannot rely on llvm to detect these for us. In a standalone non-dylib build this was accidentaly working because these libraries were pulled in as an interface dependency of the .a files, but in a dylib build these are no longer part of the link interface, and so we need to add them explicitly.

[mgorny]

I can't test this right now but please make sure not to break linking to split shared libraries without dylib.

[labath]

Ah, yes, another build mode :/. Are you doing a standalone build or not?
I am pretty sure this will be fine for an integrated build, but I have no idea what will it do in the standalone mode...

[mgorny]

Yes, full standalone.

[krytarowski]

cmake/modules/LLDBConfig.cmake

354 ↗

(On Diff #137990)

A more portable form of this:

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

Haiku needs 2 libraries, other Unices can use -lldl or similar.

[mgorny]

cmake/modules/LLDBConfig.cmake

354 ↗

(On Diff #137990)

Didn't this raise the problem of cmake using full paths on some systems?

[krytarowski]

cmake/modules/LLDBConfig.cmake

354 ↗

(On Diff #137990)

No.

[labath]

Right, so this will not work for the BUILD_SHARED_LIBS case, and there doesn't seem to be an easy way to make it work from this end. I'm going to try fixing this from the llvm side and come back to this if we still need the pthread/dl fixes.

cmake/modules/LLDBConfig.cmake

354 ↗

(On Diff #137990)

Do you have some reference for the portability claim? (mainly because I'm trying to learn more about how cmake works, but also because I'd rather let cmake figure out when to prepend -l instead of doing it myself).

I don't see how the "two libraries" thingy comes into play here. It could be a problem if we are mismatching lists and strings but I don't see why that would necessitate manually tagging with -l.

[krytarowski]

cmake/modules/LLDBConfig.cmake

354 ↗

(On Diff #137990)

dlopen(3)-like functions in -ldl is Linux; HPUX uses -ldl, Darwin/BSDs no extra libraries, Haiku -lroot -lbe etc.

https://github.com/Kitware/CMake/search?p=2&q=CMAKE_DL_LIBS&type=&utf8=%E2%9C%93

I know that we don't support multiple OSes as of today, but we can make it compatible with every recognizable one with this change.

It looks like the proper syntax is to drop -l from my first proposal:

foreach(lib ${CMAKE_DL_LIBS})
list(APPEND system_libs ${lib})
endforeach()

[krytarowski]

cmake/modules/LLDBConfig.cmake

354 ↗

(On Diff #137990)

Or perhaps even unconditional: list(APPEND system_libs ${CMAKE_DL_LIBS})

[labath]

cmake/modules/LLDBConfig.cmake

354 ↗

(On Diff #137990)

I like that: :D

[labath]

This drops the LLDBStandalone changes (they are replaced by D44391 on the llvm
side), and simplifies the CMAKE_DL_LIBS logic.

I've tested this out both with BUILD_SHARED_LIBS and LLVM_LINK_LLVM_DYLIB builds
(for the latter, be sure to specify LLVM_LINK_LLVM_DYLIB also when configuring
lldb), but I'd appreciate more testing (I'm no hurry to land this).

[krytarowski]

cmake/modules/LLDBConfig.cmake

349 ↗

(On Diff #138034)

Why UNIX here?

Why CMAKE_THREAD_PREFER_PTHREAD? It looks like used only on IRIX and that one is not going anywhere nowadays. (And certainly similarly to other commercial OSes, due to legal work/removing not-owned code, it's not possible to push it to Open-Source).

Assuming that system_libs can accept "-pthreads", this patch looks good to me.

[labath]

cmake/modules/LLDBConfig.cmake

349 ↗

(On Diff #138034)

For the second part, I copied it out of the llvm's build scripts without looking at what it does (with the idea of trying to maintain a consistent build). However, if it's only used at irix, then I guess I can remove that.

The UNIX part is also inspired by llvm, but I simplified it a bit (they use CYGWIN OR NOT WINDOWS). I was assuming the idea was to make sure we use native thread support and not pthreads (which are present there sometimes, I think).

[krytarowski]

I cannot test it shortly on NetBSD, but if there are any issues left (probably none) - I will fix it in future.

cmake/modules/LLDBConfig.cmake

349 ↗

(On Diff #138034)

I see, I would use CYGWIN OR NOT WINDOWS without changing the logic.

Keeping here CMAKE_THREAD_PREFER_PTHREAD does not make harm. non-pthreading on UNIX systems is rather in extinct and remnant of 90ties (Minix has something like that.. and lack of pthreads).

The UNIX part looks correct.

[labath]

cmake/modules/LLDBConfig.cmake

349 ↗

(On Diff #138034)

Thanks, I'll update the logic to match and submit.

lldb/trunk/cmake/modules/LLDBConfig.cmake

else()
   if (LIBXML2_FOUND)
     add_definitions( -DLIBXML2_DEFINED )
     list(APPEND system_libs ${LIBXML2_LIBRARIES})
     include_directories(${LIBXML2_INCLUDE_DIR})
   endif()
 
 endif()
 
-if (HAVE_LIBPTHREAD)
-  list(APPEND system_libs pthread)
-endif(HAVE_LIBPTHREAD)
+if( WIN32 AND NOT CYGWIN )
+  set(PURE_WINDOWS 1)
+endif()
 
-if (HAVE_LIBDL)
-  list(APPEND system_libs ${CMAKE_DL_LIBS})
+if(NOT PURE_WINDOWS)
+  set(CMAKE_THREAD_PREFER_PTHREAD TRUE)
+  find_package(Threads REQUIRED)
+  list(APPEND system_libs ${CMAKE_THREAD_LIBS_INIT})
 endif()
 
+list(APPEND system_libs ${CMAKE_DL_LIBS})
+
 # Figure out if lldb could use lldb-server.  If so, then we'll
 # ensure we build lldb-server when an lldb target is being built.
 if (CMAKE_SYSTEM_NAME MATCHES "Android|Darwin|FreeBSD|Linux|NetBSD")
     set(LLDB_CAN_USE_LLDB_SERVER 1)
 else()
     set(LLDB_CAN_USE_LLDB_SERVER 0)
 endif()