Page MenuHomePhabricator

Export LLVM_DYLIB_COMPONENTS in LLVMConfig.cmake
ClosedPublic

Authored by labath on Mar 12 2018, 9:12 AM.

Details

Summary

This is needed so that external projects (e.g. a standalone build of
lldb) can link to the LLVM shared library via the USE_SHARED argument of
llvm_config. Without this, llvm_config would add LLVM to the link list,
but then also add the constituent static libraries, resulting in
multiply defined symbols.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Mar 12 2018, 9:12 AM

I don't see why not but I'm not sure if it has any value by itself. FWICS, LLVM_DYLIB_COMPONENTS is not used anywhere in cmake/*.

It's used in cmake/modules/LLVM-Config.cmake, and that's the part I need. Without this, the llvm_config macro will behave differently in standalone vs. in-tree builds (i.e., it will be mostly broken in the out-of-tree case as this variable will be empty).

mgorny accepted this revision.Mar 12 2018, 10:06 AM

Aye, sorry, I confused it with generated config ;-). It looks sane to me.

This revision is now accepted and ready to land.Mar 12 2018, 10:06 AM

This change seems to make llvm_config() usable for out-of-tree builds, neat! I tried a standalone build of LLDB and it works fine after having rebuilt llvm with this change.

It should also make the example given on llvm-dev behave correctly, with one caveat:

This works:

set(USE_SHARED USE_SHARED)
llvm_config(foo support)

This doesn't (still chooses static libs):

llvm_config(foo USE_SHARED support)

The above is driving me nuts! I don't know enough about CMake to figure it out myself. :(

Scratch that, I see what's going on! The body of the llvm_config macro needs to check if(ARG_USE_SHARED) instead of if(USE_SHARED).

Could you please add this related fix to this change?

One more observation: It might be preferable to avoid defining set(LLVM_DYLIB_COMPONENTS all) in non-DYLIB builds. It prevents llvm_config(foo USE_SHARED ...) from opportunistically linking to the DYLIB while still having the static libraries as a fallback.

I really want projects linking to LLVM to be able to use llvm_config(foo USE_SHARED ...) and not worry about the detail of whether it's a static or dylib-enabled build.

The ARG_USE_SHARED thingy looks like a typo. I'll make a separate patch for that.

The LLVM_DYLIB_COMPONENTS "all" idea also sounds reasonable, but is also quite orthogonal to what I'm trying to do here (making standalone and in-tree builds behave the same way). I don't have time to look into that now, but I do encourage you to propose a patch for that yourself.

You have a point that any further tweaks to llvm_config should be created and reviewed separately. What I described above is not too great either; it shouldn't be required to specify USE_SHARED.

Ideally llvm_config(foo <list-of-components>) would work similar to llvm-config --libs <list-of-components. The latter defaults to using the dylib if LLVM is built with LLVM_LINK_LLVM_DYLIB=ON.

I'm not sure I can come up with a proper patch for this. With three build modes and in/out-of-tree builds, it gets confusing quite fast. ๐Ÿ˜•

In order to not derail this much further, I'll proceed to follow bug 35245 and post there if I manage to cobble something together.

This revision was automatically updated to reflect the committed changes.