[PATCH] D12488: Enable linking tools, shared libraries against libLLVM

[Andrew Wilkins via llvm-commits]

axw added inline comments.

================
Comment at: cmake/modules/AddLLVM.cmake:448
@@ +447,3 @@
+  if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_STATIC)
+    set(llvm_libs LLVM)
+  else()
----------------
beanz wrote:
> If we're going to implement this, we should generalize the code from llvm-c-test/CMakeLists.txt to ensure that the dylib contains all the libraries that the library or tool we're linking requires. That will avoid difficult to debug linker errors.
I think it's unlikely that anyone will use LLVM_LINK_LLVM_DYLIB, want to specify the components, and build only a subset of tools/shared libraries that matches them. So instead I've modified llvm-shlib so that if LLVM_LINK_LLVM_DYLIB is set:
 - LLVM_DYLIB_COMPONENTS must not be set, and doing so will cause an error
 - LLVM_DYLIB_EXPORT_ALL must be set, and not doing so will cause an error (it's defaulted to ON in the top-level CMakeLists.txt when LLVM_LINK_LLVM_DYLIB is enabled)
 - Force linking in all components (except TableGen, as noted in summary and code comments)

Let me know if you think this isn't reasonable.

================
Comment at: cmake/modules/TableGen.cmake:74
@@ -73,2 +73,3 @@
 macro(add_tablegen target project)
+  set(LLVM_LINK_LLVM_DYLIB OFF) # dylib depends on tblgen
   set(${target}_OLD_LLVM_LINK_COMPONENTS ${LLVM_LINK_COMPONENTS})
----------------
beanz wrote:
> I'd prefer to see this reworked as an option to add_llvm_utility, add_llvm_tool, etc. That makes it less magic.
Done: added DISABLE_LLVM_LINK_LLVM_DYLIB option to add_llvm_library and add_llvm_executable (add_llvm_{utility, tool} etc. all devolve into the latter). Suggestions for less wordy option name are welcome.

================
Comment at: tools/llvm-config/CMakeLists.txt:2
@@ -1,2 +1,3 @@
 set(LLVM_LINK_COMPONENTS support)
+set(LLVM_LINK_LLVM_DYLIB OFF)
 
----------------
beanz wrote:
> rnk wrote:
> > Maybe throw a comment in here? I'm assuming this is because we want llvm-config to be a tiny statically linked binary for portability.
> Same as my comment on tablegen. I'd rather see this as an overridden option on add_llvm_tool than turning the setting off for the sub directory.
@rnk actually this change is unnecessary, I've reverted it. I must have made a mistake earlier on which caused a cyclic dependency, but it's not there anymore.

================
Comment at: tools/llvm-shlib/CMakeLists.txt:71
@@ -48,2 +70,3 @@
+
 if(NOT DEFINED LLVM_DYLIB_EXPORTED_SYMBOL_FILE)
 
----------------
rnk wrote:
> beanz wrote:
> > rnk wrote:
> > > @beanz Why is this code even generating export lists if it wants to export everything? The default Unix behavior is to export all default visibility symbols from shared objects. Here's how I'd structure it:
> > > ```
> > > set(LLVM_EXPORTED_SYMBOL_FILE)
> > > if (DEFINED LLVM_LLVM_DYLIB_EXPORTED_SYMBOL_FILE)
> > >   set(LLVM_EXPORTED_SYMBOL_FILE ${LLVM_DYLIB_EXPORTED_SYMBOL_FILE})
> > >   add_custom_target(libLLVMExports DEPENDS ${LLVM_EXPORTED_SYMBOL_FILE})
> > > elseif (NOT LLVM_DYLIB_EXPORT_ALL)
> > >   # ... C API generation code here
> > > endif()
> > > ```
> > > 
> > > If LLVM_EXPORTED_SYMBOL_FILE is empty, add_llvm_library won't pass it in.
> > In my mind 'all' was to export everything LLVM, and we have some random things that aren't in the llvm namespace that I wanted to exclude.
> > 
> > If that doesn't make sense to other people, feel free to change it.
> I'd rather go with the tool defaults over custom shell commands. :)
@pcc pointed out these changes are no longer necessary, so I have reverted them.


http://reviews.llvm.org/D12488