Page MenuHomePhabricator

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

Authored by pablooliveira on Sep 23 2016, 6:52 AM.

Details

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,

Diff Detail

Repository
rL LLVM

Event Timeline

pablooliveira retitled this revision from to Keep dependencies separated between static and dynamic libraries. Fix for bug #28127. .
pablooliveira updated this object.
pablooliveira set the repository for this revision to rL LLVM.
pablooliveira added a project: Restricted Project.
labath resigned from this revision.Sep 23 2016, 7:01 AM
labath edited reviewers, added: beanz; removed: labath.

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

brad.king edited edge metadata.Sep 23 2016, 7:08 AM

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 requested changes to this revision.Sep 23 2016, 4:01 PM
beanz edited edge metadata.

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.

This revision now requires changes to proceed.Sep 23 2016, 4:01 PM

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

Meinersbur requested changes to this revision.Sep 28 2016, 1:34 AM
Meinersbur edited edge metadata.

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 added a subscriber: labath.Oct 3 2016, 6:35 PM

@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.

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 !

Can we land this patch now? Thanks

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 ?

sylvestre.ledru resigned from this revision.Jan 17 2019, 5:41 AM