Keep dependencies separated between static and dynamic libraries. Fix for bug #28127.

[• pablooliveira]

Summary

This review proposes a fix for bug 28127 https://llvm.org/bugs/show_bug.cgi?id=28127.

LLDB maintainers require lldb-server to be built statically, so it can be quickly copied to a remote target for debugging without having to deal with dependency problems.
When LLVM is built using -DLLVM_LINK_LLVM_DYLIB=ON -DLLVM_BUILD_LLVM_DYLIB=ON, lldb-server breaks because a CommandLine option is registered twice.
Those are the options used when building LLVM's debian packages.

To achieve a static executable, lldb-server is linked only against .a libraries such as the LLVM .a individual components and the clang .a static libraries.
The problem is that when using -DLLVM_LINK_LLVM_DYLIB=ON -DLLVM_BUILD_LLVM_DYLIB=ON, clang .a static libraries introduce a cmake transitive dependency with target_link_libraries to a single libLLVM.so dynamic library which contains all the llvm components symbols.
This results in symbols defined twice in lldb-server: through the static .a llvm components and transitively through the .a clang libraries which require libLLVM.so.

This patch resolves the issue by ensuring that when static .a libraries are build they only introduce cmake transitive dependencies to other .a components.
Because Polly uses its own macros for adding libraries dependencies and does not go through the llvm_add_library_name, this patch also changes Polly build process.

The patch has been tested on the debian snapshot package of LLVM and fixes the issue.
I add Pavel, Brad and Sylvestre as initial reviewers since they have been involved in the discussion and fix. Tobias could you please have a look at the Polly part of the fix ? I'm not sure who should review the changes to AddLLVM.cmake.

Thanks,

[labath]

@beanz, could you look at the llvm side of this?

[brad.king]

ensuring that when static .a libraries are build they only introduce cmake transitive dependencies to other .a components.

This sounds good from a design level.

As for the implementation, I'm not particularly familiar with llvm_add_library and all its options and use cases so I'll have to defer to other reviewers for that.

[beanz]

Making static archives only have dependencies on other static archives has serious implications and fundamentally breaks LLVM_LINK_LLVM_DYLIB. For example:

libClangDriver.a depends on libLLVMDriver.a.

In the current build with the LLVM_LINK_LLVM_DYLIB libClangDriver.a depends on libLLVM, which means clang depends on libLLVM.

If you make static archives only depend on static archives now libClangDriver.a depends on libLLVMDriver.a, which means clang would link libLLVMDriver.a in addition to getting libLLVM. Standard unix linker semantics will result in code from libLLVMDriver being linked into libClang, which is explicitly what LLVM_LINK_LLVM_DYLIB is intended to prevent.

[grosser]

Pablo, thank you for taking Polly into account as well. I add Michael, who probably knows our cmake build system best.

[Meinersbur]

The description of LLVM_LINK_LLVM_DYLIB reads:

Link tools against the libllvm dynamic library

that is, its intend is what this patch would disable (for dynamic modules). I assume that you would like lldb-server to work that way even if LLVM_LINK_LLVM_DYLIB=ON. I suggest to document instead that lldb-server depends on dynamic objects in that configuration and therefore cannot be moved to other computer as stand-alone; their architecture might mismatch anyway.

Other solutions I can think of:

• The LLVM modules don't depend on libraries themselves, only the tools (opt, clang, ...) link against libllvm (note that this probably has a large impact on the build system)
• Like llvm-tblgen, clang-tblgen with LLVM_OPTIMIZED_TABLEGEN and cross-compilation, use a sub-buildsystem that builds lldb-server with the right options (and the right target architecture)

For the Polly part, you cannot just remove the BUILD_SHARED_LIBS condition. It will cause Polly to have the same problem of multiple command-line registrations you are observing when using -load LLVMPolly.so. However, I already suggested a redesign of that part in D24442.

[labath]

@pablooliveira @sylvestre.ledru :

If you are fine with having lldb-server link against libLLVM in the LLVM_LINK_LLVM_DYLIB case, I can make an lldb-only change that makes it work. It won't affect our use case as we don't use that flag.

[• pablooliveira]

If you are fine with having lldb-server link against libLLVM in the LLVM_LINK_LLVM_DYLIB case, I can make an lldb-only change that makes it work. It won't affect our use case as we don't use that flag.

@labath, I think that would be fine in the context of the Debian package. Thanks !

[sylvestre.ledru]

Can we land this patch now? Thanks

[labath]

Several people have expressed objections to this patch, so I guess the answer is no. :)

However, I have made some changes to lldb's build system (D25680) which should fix the original issue you were experiencing (Sorry, I forgot to loop you in on that). Can you check if you are still experiencing this problem with the latest trunk ?

[mehdi_amini]

Please also close https://llvm.org/bugs/show_bug.cgi?id=28127 if @labath fix is enough.

cmake/modules/AddLLVM.cmake

if(NOT APPLE AND ARG_SONAME)
         ALWAYS_GENERATE)
     endif()
   endif()
 
   if(ARG_MODULE AND LLVM_EXPORT_SYMBOLS_FOR_PLUGINS AND ARG_PLUGIN_TOOL AND (WIN32 OR CYGWIN))
     # On DLL platforms symbols are imported from the tool by linking against it.
     set(llvm_libs ${ARG_PLUGIN_TOOL})
   elseif (DEFINED LLVM_LINK_COMPONENTS OR DEFINED ARG_LINK_COMPONENTS)
-    if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
+    if (NOT ARG_STATIC AND LLVM_LINK_LLVM_DYLIB AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
       set(llvm_libs LLVM)
     else()
       llvm_map_components_to_libnames(llvm_libs
        ${ARG_LINK_COMPONENTS}
        ${LLVM_LINK_COMPONENTS}
        )
     endif()
   else()

tools/polly/lib/CMakeLists.txt

add_polly_library(Polly
   )
 
 if (GPU_CODEGEN)
   target_link_libraries(Polly PollyPPCG)
 endif (GPU_CODEGEN)
 
 target_link_libraries(Polly PollyISL)
 
-if (BUILD_SHARED_LIBS)
-  target_link_libraries(Polly
-    LLVMSupport
-    LLVMCore
-    LLVMScalarOpts
-    LLVMInstCombine
-    LLVMTransformUtils
-    LLVMAnalysis
-    LLVMipo
-    LLVMMC
 # The libraries below are required for darwin: http://PR26392
-    LLVMBitReader
-    LLVMMCParser
-    LLVMObject
-    LLVMProfileData
-    LLVMTarget
-    LLVMVectorize
-  )
-  link_directories(
-    ${LLVM_LIBRARY_DIR}
-  )
-elseif (LLVM_LINK_LLVM_DYLIB)
-  target_link_libraries(Polly
-    LLVM
-  )
-  link_directories(
-    ${LLVM_LIBRARY_DIR}
-  )
-endif()
 
 # Build a monolithic Polly.a and a thin module LLVMPolly.moduleext that links to
 # that static library.
 if (MSVC)
   # Add dummy target, because loadable modules are not supported on Windows
   add_custom_target(LLVMPolly)
   set_target_properties(LLVMPolly PROPERTIES FOLDER "Polly")
 else ()