Page MenuHomePhabricator

Expression: add missing linkage to RuntimeDyld component
AbandonedPublic

Authored by mgorny on Mar 25 2017, 6:53 AM.

Details

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)'

Event Timeline

mgorny created this revision.Mar 25 2017, 6:53 AM
joerg accepted this revision.Mar 25 2017, 11:53 AM
This revision is now accepted and ready to land.Mar 25 2017, 11:53 AM
This revision was automatically updated to reflect the committed changes.
labath added a subscriber: labath.Mar 29 2017, 1:11 PM

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?

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 edited edge metadata.Mar 30 2017, 10:06 AM

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.

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.

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 added a subscriber: lhames.Apr 3 2017, 3:27 PM

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

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 reopened this revision.Aug 2 2017, 1:26 AM
mgorny added a reviewer: chapuni.
mgorny added a subscriber: chapuni.

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

This revision is now accepted and ready to land.Aug 2 2017, 1:26 AM
chapuni edited edge metadata.Aug 2 2017, 1:33 AM

@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 added a comment.Aug 3 2017, 2:05 PM

@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 accepted this revision.Aug 3 2017, 5:00 PM

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 added a comment.Aug 4 2017, 9:56 AM

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

mgorny abandoned this revision.Aug 5 2017, 2:54 AM