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
