This is an archive of the discontinued LLVM Phabricator instance.

[cmake] rework LLVM_LINK_LLVM_DYLIB option handling
ClosedPublic

Authored by axw on Sep 3 2015, 12:32 AM.

Details

Summary

This diff attempts to address the concerns raised in
http://reviews.llvm.org/D12488.

We introduce a new USE_SHARED option to llvm_config,
which, if set, causes the target to be linked against
libLLVM.

add_llvm_utility now uniformly disables linking against
libLLVM. These utilities are not intended for distribution,
and this keeps the option handling more centralised.

llvm-shlib is now processes before any other "tools"
subdirectories, ensuring the libLLVM target is defined
before its dependents.

One main difference from what was requested: llvm_config
does not prune LLVM_DYLIB_COMPONENTS from the components
passed into explicit_llvm_config. This is because the "all"
component does something special, adding additional
libraries (namely libLTO). Adding the component libraries
after libLLVM should not be a problem, as symbols will be
resolved in libLLVM first.

Finally, I'm not really happy with the
DISABLE_LLVM_LINK_LLVM option, but I'm not sure of a
better way to get the following:

  • link all tools and shared libraries to libLLVM if LLVM_LINK_LLVM_DYLIB is set
  • some way of explicitly *not* doing so for utilities and libLLVM itself

Suggestions for improvement here are particularly welcome.

Diff Detail

Repository
rL LLVM

Event Timeline

axw updated this revision to Diff 33905.Sep 3 2015, 12:32 AM
axw retitled this revision from to [cmake] rework LLVM_LINK_LLVM_DYLIB option handling.
axw updated this object.
axw added a reviewer: beanz.
axw added a subscriber: llvm-commits.
beanz accepted this revision.Sep 4 2015, 9:31 AM
beanz edited edge metadata.

Thank you for doing this. LGTM. A few comments below.

cmake/modules/LLVM-Config.cmake
45 ↗(On Diff #33905)

I do think we should strip out the components based on LLVM_DYLIB_COMPONENTS to make it more explicit, but this is fine, we can do that later.

tools/CMakeLists.txt
35 ↗(On Diff #33905)

You were correct that this isn't actually needed. Feel free to strip this out.

This revision is now accepted and ready to land.Sep 4 2015, 9:31 AM
axw marked an inline comment as done.Sep 5 2015, 1:25 AM

Thanks for the review.

cmake/modules/LLVM-Config.cmake
45 ↗(On Diff #33905)

I've added a TODO. The main complication is in handling the case of LLVM_DYLIB_COMPONETS=all. I think it just means ~replicating what's done in llvm-shlib: convert all to libnames and only retain the shared library ones, which won't be included in libLLVM.

axw closed this revision.Sep 5 2015, 1:28 AM
This revision was automatically updated to reflect the committed changes.