[Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
beanz added a comment. @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 https://reviews.llvm.org/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. Repository: rL LLVM https://reviews.llvm.org/D31367 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
chapuni accepted this revision. chapuni added a comment. Regardless of https://reviews.llvm.org/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) Repository: rL LLVM https://reviews.llvm.org/D31367 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
chapuni added a comment. @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. Repository: rL LLVM https://reviews.llvm.org/D31367 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
mgorny reopened this revision. mgorny added a reviewer: chapuni. mgorny added a subscriber: chapuni. mgorny added a comment. This revision is now accepted and ready to land. Well, @chapuni disagrees on https://reviews.llvm.org/D36211. Which way should we go then? Repository: rL LLVM https://reviews.llvm.org/D31367 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
mgorny added a comment. 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. Repository: rL LLVM https://reviews.llvm.org/D31367 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
beanz added a subscriber: lhames. beanz added a comment. @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) Repository: rL LLVM https://reviews.llvm.org/D31367 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
On czw, 2017-03-30 at 13:55 -0700, Chris Bieneman wrote: > I had a talk with Lang about the ExecutionEngine library structuring, and it > sounds like there are some problems there that need to be worked out. > > Luckily for this specific case, I think the solution is actually quite simple: > > ``` > diff --git a/include/llvm/ExecutionEngine/ExecutionEngine.h > b/include/llvm/ExecutionEngine/ExecutionEngine.h > index f68337c..cc99f94 100644 > --- a/include/llvm/ExecutionEngine/ExecutionEngine.h > +++ b/include/llvm/ExecutionEngine/ExecutionEngine.h > @@ -15,7 +15,6 @@ > #ifndef LLVM_EXECUTIONENGINE_EXECUTIONENGINE_H > #define LLVM_EXECUTIONENGINE_EXECUTIONENGINE_H > > -#include "RuntimeDyld.h" > #include "llvm-c/ExecutionEngine.h" > #include "llvm/ADT/SmallVector.h" > #include "llvm/ADT/StringRef.h" > @@ -49,6 +48,7 @@ class ObjectCache; > class RTDyldMemoryManager; > class Triple; > class Type; > +class JITSymbolResolver; > > namespace object { >class Archive; > ``` > > It seems to me that there is no reason why ExecutionEngine.h needs to include > RuntimeDyld.h. a forward declaration of the JITSymbolResolver class will > suffice. > This does not solve the problem: 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)' lib64/liblldbExpression.a(IRExecutionUnit.cpp.o):IRExecutionUnit.cpp:vtable for lldb_private::IRExecutionUnit::MemoryManager: error: undefined reference to 'llvm::RuntimeDyld::MemoryManager::anchor()' lib64/liblldbExpression.a(IRExecutionUnit.cpp.o):IRExecutionUnit.cpp:vtable for lldb_private::IRExecutionUnit::MemoryManager: error: undefined reference to 'llvm::JITSymbolResolver::anchor()' collect2: error: ld returned 1 exit status -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
On Thu, Mar 30, 2017 at 10:47:39PM +0200, Michał Górny wrote: > On czw, 2017-03-30 at 21:37 +0200, Joerg Sonnenberger wrote: > > On Thu, Mar 30, 2017 at 06:00:15PM +, Chris Bieneman via Phabricator > > wrote: > > > 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`. > > > > Welcome to the brave new world of newer ELF linkers. Just because a > > library pulls in a symbol into the namespace doesn't mean you can just > > use that symbol. They want you to explicitly specify the dependency... > > This does happen for static archives though. > > This is expected and desired. Just because some random library you're > using is using zlib does not mean you should skip linking zlib when your > program is using it too. That justification never made that much sense. The linker simply does not know whether the dependency is considered part of the ABI contract of the library or not. Hint: the symbol lookup rules for ELF is recursive. Having different rules for link-time vs run-time is a very good sign that something is very fishy. But let's not get distracted. The behavior change of GNU linkers (and anything following them) is exactly why the original patch was correct here. Symbols from RuntimeDyld are directly used -- as such the library should be an explicit declared dependency. If no such reference is created, i.e. due to not using any of the header functions references such symbols, no such dependency is necessary. ExecutionEngine can't tell in advance. Joerg ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
On czw, 2017-03-30 at 13:55 -0700, Chris Bieneman wrote: > I had a talk with Lang about the ExecutionEngine library structuring, and it > sounds like there are some problems there that need to be worked out. > > Luckily for this specific case, I think the solution is actually quite simple: > > ``` > diff --git a/include/llvm/ExecutionEngine/ExecutionEngine.h > b/include/llvm/ExecutionEngine/ExecutionEngine.h > index f68337c..cc99f94 100644 > --- a/include/llvm/ExecutionEngine/ExecutionEngine.h > +++ b/include/llvm/ExecutionEngine/ExecutionEngine.h > @@ -15,7 +15,6 @@ > #ifndef LLVM_EXECUTIONENGINE_EXECUTIONENGINE_H > #define LLVM_EXECUTIONENGINE_EXECUTIONENGINE_H > > -#include "RuntimeDyld.h" > #include "llvm-c/ExecutionEngine.h" > #include "llvm/ADT/SmallVector.h" > #include "llvm/ADT/StringRef.h" > @@ -49,6 +48,7 @@ class ObjectCache; > class RTDyldMemoryManager; > class Triple; > class Type; > +class JITSymbolResolver; > > namespace object { >class Archive; > ``` > > It seems to me that there is no reason why ExecutionEngine.h needs to include > RuntimeDyld.h. a forward declaration of the JITSymbolResolver class will > suffice. > Thanks, I'll test this patch and get back to you. -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
I had a talk with Lang about the ExecutionEngine library structuring, and it sounds like there are some problems there that need to be worked out. Luckily for this specific case, I think the solution is actually quite simple: ``` diff --git a/include/llvm/ExecutionEngine/ExecutionEngine.h b/include/llvm/ExecutionEngine/ExecutionEngine.h index f68337c..cc99f94 100644 --- a/include/llvm/ExecutionEngine/ExecutionEngine.h +++ b/include/llvm/ExecutionEngine/ExecutionEngine.h @@ -15,7 +15,6 @@ #ifndef LLVM_EXECUTIONENGINE_EXECUTIONENGINE_H #define LLVM_EXECUTIONENGINE_EXECUTIONENGINE_H -#include "RuntimeDyld.h" #include "llvm-c/ExecutionEngine.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" @@ -49,6 +48,7 @@ class ObjectCache; class RTDyldMemoryManager; class Triple; class Type; +class JITSymbolResolver; namespace object { class Archive; ``` It seems to me that there is no reason why ExecutionEngine.h needs to include RuntimeDyld.h. a forward declaration of the JITSymbolResolver class will suffice. -Chris > On Mar 30, 2017, at 11:41 AM, Michał Górny via Phabricator >wrote: > > mgorny added a comment. > > In https://reviews.llvm.org/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`)? > > > Repository: > rL LLVM > > https://reviews.llvm.org/D31367 > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
On czw, 2017-03-30 at 21:37 +0200, Joerg Sonnenberger wrote: > On Thu, Mar 30, 2017 at 06:00:15PM +, Chris Bieneman via Phabricator > wrote: > > 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`. > > Welcome to the brave new world of newer ELF linkers. Just because a > library pulls in a symbol into the namespace doesn't mean you can just > use that symbol. They want you to explicitly specify the dependency... > This does happen for static archives though. This is expected and desired. Just because some random library you're using is using zlib does not mean you should skip linking zlib when your program is using it too. -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
On Thu, Mar 30, 2017 at 06:00:15PM +, Chris Bieneman via Phabricator wrote: > 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`. Welcome to the brave new world of newer ELF linkers. Just because a library pulls in a symbol into the namespace doesn't mean you can just use that symbol. They want you to explicitly specify the dependency... This does happen for static archives though. Joerg ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
mgorny added a comment. In https://reviews.llvm.org/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`)? Repository: rL LLVM https://reviews.llvm.org/D31367 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
beanz added a comment. 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. Repository: rL LLVM https://reviews.llvm.org/D31367 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
beanz added a comment. 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. Repository: rL LLVM https://reviews.llvm.org/D31367 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
mgorny added a comment. In https://reviews.llvm.org/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. Repository: rL LLVM https://reviews.llvm.org/D31367 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
labath added a comment. 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? Repository: rL LLVM https://reviews.llvm.org/D31367 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
This revision was automatically updated to reflect the committed changes. Closed by commit rL298776: Expression: add missing linkage to RuntimeDyld component (authored by mgorny). Changed prior to commit: https://reviews.llvm.org/D31367?vs=93041=93051#toc Repository: rL LLVM https://reviews.llvm.org/D31367 Files: lldb/trunk/source/Expression/CMakeLists.txt Index: lldb/trunk/source/Expression/CMakeLists.txt === --- lldb/trunk/source/Expression/CMakeLists.txt +++ lldb/trunk/source/Expression/CMakeLists.txt @@ -34,5 +34,6 @@ LINK_COMPONENTS Core ExecutionEngine +RuntimeDyld Support ) Index: lldb/trunk/source/Expression/CMakeLists.txt === --- lldb/trunk/source/Expression/CMakeLists.txt +++ lldb/trunk/source/Expression/CMakeLists.txt @@ -34,5 +34,6 @@ LINK_COMPONENTS Core ExecutionEngine +RuntimeDyld Support ) ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component
mgorny created this revision. mgorny added a project: LLDB. 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)' Repository: rL LLVM https://reviews.llvm.org/D31367 Files: source/Expression/CMakeLists.txt Index: source/Expression/CMakeLists.txt === --- source/Expression/CMakeLists.txt +++ source/Expression/CMakeLists.txt @@ -34,5 +34,6 @@ LINK_COMPONENTS Core ExecutionEngine +RuntimeDyld Support ) Index: source/Expression/CMakeLists.txt === --- source/Expression/CMakeLists.txt +++ source/Expression/CMakeLists.txt @@ -34,5 +34,6 @@ LINK_COMPONENTS Core ExecutionEngine +RuntimeDyld Support ) ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits