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

[Chris Bieneman via llvm-commits]

beanz 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()
----------------
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.

================
Comment at: cmake/modules/AddLLVM.cmake:612
@@ +611,3 @@
+  if (LLVM_LINK_LLVM_DYLIB)
+    target_link_libraries(${name} LLVM)
+  else()
----------------
See comment above.

================
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})
----------------
I'd prefer to see this reworked as an option to add_llvm_utility, add_llvm_tool, etc. That makes it less magic.

================
Comment at: tools/llvm-config/CMakeLists.txt:2
@@ -1,2 +1,3 @@
 set(LLVM_LINK_COMPONENTS support)
+set(LLVM_LINK_LLVM_DYLIB OFF)
 
----------------
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.

================
Comment at: tools/llvm-shlib/CMakeLists.txt:71
@@ -48,2 +70,3 @@
+
 if(NOT DEFINED LLVM_DYLIB_EXPORTED_SYMBOL_FILE)
 
----------------
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.


http://reviews.llvm.org/D12488