This is an archive of the discontinued LLVM Phabricator instance.

[cmake] Fix standalone+LLVM_LINK_LLVM_DYLIB builds (pr36687)
ClosedPublic

Authored by labath on Mar 12 2018, 5:36 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Mar 12 2018, 5:36 AM

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

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...

Yes, full standalone.

krytarowski added inline comments.Mar 12 2018, 8:08 AM
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 added inline comments.Mar 12 2018, 8:17 AM
cmake/modules/LLDBConfig.cmake
354 ↗(On Diff #137990)

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

krytarowski added inline comments.Mar 12 2018, 8:21 AM
cmake/modules/LLDBConfig.cmake
354 ↗(On Diff #137990)

No.

labath planned changes to this revision.Mar 12 2018, 8:38 AM

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 added inline comments.Mar 12 2018, 8:50 AM
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 added inline comments.Mar 12 2018, 8:52 AM
cmake/modules/LLDBConfig.cmake
354 ↗(On Diff #137990)

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

labath added inline comments.Mar 12 2018, 9:22 AM
cmake/modules/LLDBConfig.cmake
354 ↗(On Diff #137990)

I like that: :D

labath updated this revision to Diff 138034.Mar 12 2018, 9:26 AM

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 added inline comments.Mar 12 2018, 9:52 AM
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 added inline comments.Mar 12 2018, 10:09 AM
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 accepted this revision.Mar 12 2018, 10:27 AM

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.

This revision is now accepted and ready to land.Mar 12 2018, 10:27 AM
labath added inline comments.Mar 14 2018, 3:09 AM
cmake/modules/LLDBConfig.cmake
349 ↗(On Diff #138034)

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

This revision was automatically updated to reflect the committed changes.