Expression: add missing linkage to RuntimeDyld component

[mgorny]

Summary

Add missing linkage from lldbExpression library to LLVMRuntimeDyld.
Otherwise the build against shared LLVM libraries fails with:

lib64/liblldbExpression.a(IRExecutionUnit.cpp.o):IRExecutionUnit.cpp:function llvm::RTDyldMemoryManager::deregisterEHFrames(unsigned char*, unsigned long, unsigned long): error: undefined reference to 'llvm::RTDyldMemoryManager::deregisterEHFramesInProcess(unsigned char*, unsigned long)'

[labath]

We don't depend on the RuntimeDyld component of llvm directy -- we only use it indirectly through the ExecutionEngine component. Shouldn't that be reflected as a dependency in the build system somehow, so that the former can be pulled in directly ?
RuntimeDyld is listed as a dependency of ExecutionEngine in lib/ExecutionEngine/LLVMBuild.txt, but that does not seem to be reflected in the cmake? Is that a bug on the llvm side?

[mgorny]

In D31367#713472, @labath wrote:

We don't depend on the RuntimeDyld component of llvm directy -- we only use it indirectly through the ExecutionEngine component. Shouldn't that be reflected as a dependency in the build system somehow, so that the former can be pulled in directly ?
RuntimeDyld is listed as a dependency of ExecutionEngine in lib/ExecutionEngine/LLVMBuild.txt, but that does not seem to be reflected in the cmake? Is that a bug on the llvm side?

I think it's not that simple. If that was a plain missing dependency in ExecutionEngine, then linking of shared ExecutionEngine would fail due to missing symbols. Since it doesn't, and it makes LLDB libraries fail, I presume this is the kind of indirect dependency that somehow forces a direct dependency via the headers. If you can confirm it's that, and that we want to implicitly force linking RuntimeDyld to all ExecutionEngine consumers, then I'll look if we can do that properly within LLVM CMake files or extend our support for dependencies for that.

However, I think that only makes sense if the dependency is indeed frequently indirectly exposed and not e.g. dependent on use of some uncommon thingy.

[beanz]

This is definitely not the right fix. Please revert.

ExecutionEngine's LLVMBuild.txt file explicitly lists that RuntimeDyld is a required library of ExecutionEngine (as well as a few others). LLVM's CMake should be connecting that as an explicit dependency, and for some reason it isn't. Adding over-specified dependencies in LLDB to workaround bugs in LLVM is not the right approach, and masks problems instead of resolving them.

[beanz]

Please revert your patch. It is *not* the right solution and is masking underlying problems.

ExecutionEngine headers directly reference symbols from RuntimeDyld, so we should enforce a requirement that anyone using ExeuctionEngine also link RuntimeDyld. This works today as expected for static archive builds. It is only broken with BUILD_SHARED_LIBS. I suspect the problem is that when built as a DSO ExecutionEngine has no unresolved symbols against RuntimeDyld, and the linker probably isn't including the reference to RuntimeDyld in the produced ExecutionEngine DSO.

Regardless of the underlying cause, this is not the correct solution, it merely hides a problem that could occur in other consumers of ExecutionEngine.

[mgorny]

In D31367#714305, @beanz wrote:

Please revert your patch. It is *not* the right solution and is masking underlying problems.

Reverted in r299095. Now I'd really appreciate some help in figuring out how to fix it properly.

ExecutionEngine headers directly reference symbols from RuntimeDyld, so we should enforce a requirement that anyone using ExeuctionEngine also link RuntimeDyld. This works today as expected for static archive builds. It is only broken with BUILD_SHARED_LIBS. I suspect the problem is that when built as a DSO ExecutionEngine has no unresolved symbols against RuntimeDyld, and the linker probably isn't including the reference to RuntimeDyld in the produced ExecutionEngine DSO.

The DSO is linking to RuntimeDyld. However, obviously the PRIVATE linkage forced in llvm_add_library is not the correct solution when the symbols are explicitly used in the headers. It should be PUBLIC for that particular component. Any suggestion on how to fix that API? Should I add an additional PUBLIC_LINK_COMPONENTS (and PUBLIC_LINK_LIBS)?

[beanz]

@mgorny, because of differences in linker semantics between Darwin and ELF, I can't reproduce the failure you have locally. I think that the patch below works around it in a more-portable way.

I've engaged with @lhames about an architectural solution to the problem, because I think we do need to change how the ExecutionEngine sub-libraries are intertwined, but that is a longer-term problem.

Can you please test this patch and see if it resolves your problem?

diff --git a/cmake/modules/AddLLVM.cmake b/cmake/modules/AddLLVM.cmake
index 7f7608cff33..1f27517c2df 100644
--- a/cmake/modules/AddLLVM.cmake
+++ b/cmake/modules/AddLLVM.cmake
@@ -342,7 +342,7 @@ endfunction(set_windows_version_resource_properties)
 function(llvm_add_library name)
   cmake_parse_arguments(ARG
     "MODULE;SHARED;STATIC;OBJECT;DISABLE_LLVM_LINK_LLVM_DYLIB;SONAME"
-    "OUTPUT_NAME;PLUGIN_TOOL"
+    "OUTPUT_NAME;PLUGIN_TOOL;DEPENDENCY_LINK_TYPE"
     "ADDITIONAL_HEADERS;DEPENDS;LINK_COMPONENTS;LINK_LIBS;OBJLIBS"
     ${ARGN})
   list(APPEND LLVM_COMMON_DEPENDS ${ARG_DEPENDS})
@@ -520,14 +520,16 @@ function(llvm_add_library name)
     get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIB_DEPS_${name})
   endif()
 
-  if(ARG_STATIC)
-    set(libtype INTERFACE)
-  else()
-    # We can use PRIVATE since SO knows its dependent libs.
-    set(libtype PRIVATE)
+  if(NOT ARG_DEPENDENCY_LINK_TYPE)
+    if(ARG_STATIC)
+      set(ARG_DEPENDENCY_LINK_TYPE INTERFACE)
+    else()
+      # We can use PRIVATE since SO knows its dependent libs.
+      set(ARG_DEPENDENCY_LINK_TYPE PRIVATE)
+    endif()
   endif()
 
-  target_link_libraries(${name} ${libtype}
+  target_link_libraries(${name} ${ARG_DEPENDENCY_LINK_TYPE}
       ${ARG_LINK_LIBS}
       ${lib_deps}
       ${llvm_libs}
diff --git a/lib/ExecutionEngine/CMakeLists.txt b/lib/ExecutionEngine/CMakeLists.txt
index 2d9337bbefd..37a57eeaa79 100644
--- a/lib/ExecutionEngine/CMakeLists.txt
+++ b/lib/ExecutionEngine/CMakeLists.txt
@@ -1,4 +1,9 @@
-
+# Execution engine is not neat and contained like other LLVM libraries. To work
+# around this if BUILD_SHARED_LIBS is set we need to force the linkage type of
+# LLVMExecutionEngine's dependencies to PUBLIC.
+if(BUILD_SHARED_LIBS)
+  set(dependency_hack DEPENDENCY_LINK_TYPE PUBLIC)
+endif()
 
 add_llvm_library(LLVMExecutionEngine
   ExecutionEngine.cpp
@@ -12,6 +17,7 @@ add_llvm_library(LLVMExecutionEngine
 
   DEPENDS
   intrinsics_gen
+  ${dependency_hack}
   )
 
 add_subdirectory(Interpreter)

[mgorny]

Well, I can confirm that the hack works for me. Not that I like it but if it's meant to be only temporary, I'm fine with any solution that works.

[mgorny]

Well, @chapuni disagrees on D36211. Which way should we go then?

[chapuni]

@mgorny was doing the right thing.

The user of llvm/ExecutionEngine/SectionMemoryManager.h (from lldb/Expression/IRExecutionUnit.h) may depend on RuntimeDyld.
If a derived class from RTDyldMemoryManager is implemented, it should depend on RuntimeDyld.

[beanz]

@chapuni, I disagree. LLDBExpression doesn't actually use RuntimeDyld directly, rather ExecutionEngine's headers do. I don't want downstream users of ExecutionEngine to know about the things it neccesitates pulling in.

[chapuni]

Regardless of D36211, I hope this to be landed. It's not late, if we would revert this again, after the discussion would be concluded.

This is an issue around hidden header-header dependencies. If clang's -fmodules could provide dependent modules, it would suggest RuntimeDyld. (It's not strict, though)

[beanz]

@mgorny I want to apologize here but I need to ask you not to commit this until @chapuni and I can come to an agreement between this solution and D36211. He ping'd me on IRC yesterday after I left for the day, I'll try and get in touch with him today to see if we can agree on the path forward.

source/Expression/CMakeLists.txt

LINK_LIBS
     lldbSymbol
     lldbTarget
     lldbUtility
     lldbPluginExpressionParserClang
 
   LINK_COMPONENTS
     Core
     ExecutionEngine
+    RuntimeDyld
     Support
   )