Page MenuHomePhabricator

[cmake] Expose the dependencies of ExecutionEngine as PUBLIC
ClosedPublic

Authored by mgorny on Aug 1 2017, 11:12 PM.

Details

Summary

Expose the dependencies of LLVMExecutionEngine library as PUBLIC rather
than PRIVATE when building a shared library. This is necessary because
the library is not contained but exposes API of other LLVM libraries via
its headers.

This causes other libraries to fail to link if the linker verifies for
correctness of -l flags (i.e. fails on indirect dependencies). This e.g.
happens when building LLDB against shared LLVM:

lib64/liblldbExpression.a(IRExecutionUnit.cpp.o):(.data.rel.ro._ZTIN4llvm18MCJITMemoryManagerE[_ZTIN4llvm18MCJITMemoryManagerE]+0x10): undefined reference to `typeinfo for llvm::RuntimeDyld::MemoryManager'
lib64/liblldbExpression.a(IRExecutionUnit.cpp.o):(.data.rel.ro._ZTVN4llvm18MCJITMemoryManagerE[_ZTVN4llvm18MCJITMemoryManagerE]+0x60): undefined reference to `llvm::RuntimeDyld::MemoryManager::anchor()'
lib64/liblldbExpression.a(IRExecutionUnit.cpp.o):(.data.rel.ro._ZTVN12lldb_private15IRExecutionUnit13MemoryManagerE[_ZTVN12lldb_private15IRExecutionUnit13MemoryManagerE]+0x48): undefined reference to `llvm::RTDyldMemoryManager::deregisterEHFrames()'
lib64/liblldbExpression.a(IRExecutionUnit.cpp.o):(.data.rel.ro._ZTVN12lldb_private15IRExecutionUnit13MemoryManagerE[_ZTVN12lldb_private15IRExecutionUnit13MemoryManagerE]+0x60): undefined reference to `llvm::RuntimeDyld::MemoryManager::anchor()'
lib64/liblldbExpression.a(IRExecutionUnit.cpp.o):(.data.rel.ro._ZTVN12lldb_private15IRExecutionUnit13MemoryManagerE[_ZTVN12lldb_private15IRExecutionUnit13MemoryManagerE]+0xd0): undefined reference to `llvm::JITSymbolResolver::anchor()'
collect2: error: ld returned 1 exit status

Declaring the dependencies as PUBLIC guarantees that any package using
the ExecutionEngine library will also get explicit -l flags for
the dependent libraries guaranteeing that the symbols exposed in headers
could be resolved.

The patch was originally written for me by @beanz but it seems that he
never submitted it.

Diff Detail

Event Timeline

mgorny created this revision.Aug 1 2017, 11:12 PM

I think, in this case, lldbExpression is responsible to link also LLVMRuntimeDyld.

  • lldb/Expression/IRExecutionUnit.h
  • llvm/ExecutionEngine/SectionMemoryManager.h

They depend on RuntimeDyld.

It'd be an easier way to expose dependent libs with PUBLIC, but I am afraid if it would hide missing deps.
I have been watching library dependencies as "Link just used libraries".

mgorny removed a subscriber: chapuni.Aug 2 2017, 1:04 AM

Well, that was my original idea but @beanz wanted it the other way around ;-). I'm fine either way, as long as it works.

beanz edited edge metadata.Aug 3 2017, 1:56 PM

@chapuni, the reason that I wanted to have this done with PUBLIC interface specifications is because lldbExpression does't actually use anything from the RuntimeDYLD library. The problem is LLVMExecutionEngine's headers are not clean, and they reference RuntimeDYLD directly and result in needing to bring it in.

Longer term we should work with Lang to figure out a better solution, but this is the most reasonable way to allow downstream users of ExecutionEngine to only reference the libraries that they are directly using.

beanz accepted this revision.Aug 3 2017, 1:56 PM
This revision is now accepted and ready to land.Aug 3 2017, 1:56 PM
chapuni requested changes to this revision.Aug 3 2017, 4:02 PM

In another perspective, it might be inappropriate that SectionMemoryManager is here.

SectionMemoryManager depends on RuntimeDyld, but doesn't depend no other compilation units in ExecutionEngine.
Users of SectionMemoryManager don't require LLVMRuntimeDyld.
As I mentioned above, the implementer of derived class of SectionMemoryManager depend on LLVMRuntimeDyld.

It might be an option to move SectionMemoryManager into RuntimeDyld. I am not sure if it were proper, though.

Anyways, I think such a hack should be introduced if it were the last resort, or you might abandon using PRIVATE for SHARED to use PUBLIC to entire SHARED.
I strongly suggest to fix lldb.

This revision now requires changes to proceed.Aug 3 2017, 4:02 PM

I talked to @beanz in the irc and I understood the background.

At the moment, we may introduce a tweak to expose RuntimeDyld as convenience for external LLVM users.
Based on @beanz's P8013. It can be simplified since the keyword can be overridden.
@mgorny, could you confirm it works?

--- a/llvm/lib/ExecutionEngine/CMakeLists.txt
+++ b/llvm/lib/ExecutionEngine/CMakeLists.txt
@@ -14,6 +14,10 @@ add_llvm_library(LLVMExecutionEngine
   intrinsics_gen
   )

+if(BUILD_SHARED_LIBS)
+  target_link_libraries(LLVMExecutionEngine PUBLIC LLVMRuntimeDyld)
+endif()
+
 add_subdirectory(Interpreter)
 add_subdirectory(MCJIT)
 add_subdirectory(Orc)

I strongly hope this tweak should be removed in the future.
I think we can make that llvm/ExecutionEngine/*.h (not implementations) doesn't emit implementations referring to RuntimeDyld.
At a glance, we may avoid "=default" ctor/dtor and stick them into certain modules w/o anchor().
@lhames, do you think we will do?

mgorny added a comment.Aug 5 2017, 2:57 AM

@chapuni, this fixes the issue for me as well. Will you commit it or should I update this revision with your patch?

mgorny updated this revision to Diff 110705.Aug 11 2017, 6:11 AM
mgorny edited edge metadata.

Updated with @chapuni's patch. Please confirm it's good to go.

chapuni accepted this revision.Aug 11 2017, 6:18 AM

Sorry, I was away.

This revision is now accepted and ready to land.Aug 11 2017, 6:18 AM

No problem at all. Thanks a lot.

This revision was automatically updated to reflect the committed changes.