[cmake] Expose the dependencies of ExecutionEngine as PUBLIC

[mgorny]

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.

[chapuni]

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]

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]

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

[chapuni]

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.

[chapuni]

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]

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

[mgorny]

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

[chapuni]

Sorry, I was away.

[mgorny]

No problem at all. Thanks a lot.

cmake/modules/AddLLVM.cmake

 #   SONAME
 #     Should set SONAME link flags and create symlinks
 #   PLUGIN_TOOL
 #     The tool (i.e. cmake target) that this plugin will link against
 #   )
 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})
   if(ARG_ADDITIONAL_HEADERS)
     # Pass through ADDITIONAL_HEADERS.
     set(ARG_ADDITIONAL_HEADERS ADDITIONAL_HEADERS ${ARG_ADDITIONAL_HEADERS})
   endif()
   if(ARG_OBJLIBS)
else()
     # dependency information for this library as defined by LLVMBuild.
     #
     # It would be nice to verify that we have the dependencies for this library
     # name, but using get_property(... SET) doesn't suffice to determine if a
     # property has been set to an empty value.
     get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIB_DEPS_${name})
   endif()
 
+  if(NOT ARG_DEPENDENCY_LINK_TYPE)
     if(ARG_STATIC)
-    set(libtype INTERFACE)
+      set(ARG_DEPENDENCY_LINK_TYPE INTERFACE)
     else()
       # We can use PRIVATE since SO knows its dependent libs.
-    set(libtype PRIVATE)
+      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}
       )
 
   if(LLVM_COMMON_DEPENDS)
     add_dependencies(${name} ${LLVM_COMMON_DEPENDS})
     # Add dependencies also to objlibs.

lib/ExecutionEngine/CMakeLists.txt

-
+# 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
   ExecutionEngineBindings.cpp
   GDBRegistrationListener.cpp
   SectionMemoryManager.cpp
   TargetSelect.cpp
 
   ADDITIONAL_HEADER_DIRS
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/ExecutionEngine
 
   DEPENDS
   intrinsics_gen
+  ${dependency_hack}
   )
 
 add_subdirectory(Interpreter)
 add_subdirectory(MCJIT)
 add_subdirectory(Orc)
 add_subdirectory(RuntimeDyld)
 
 if( LLVM_USE_OPROFILE )
   add_subdirectory(OProfileJIT)
 endif( LLVM_USE_OPROFILE )
 
 if( LLVM_USE_INTEL_JITEVENTS )
   add_subdirectory(IntelJITEvents)
 endif( LLVM_USE_INTEL_JITEVENTS )