[Lldb-commits] [PATCH] D31367: Expression: add missing linkage to RuntimeDyld component

[Chris Bieneman via lldb-commits]

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 <reviews at reviews.llvm.org> 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
> 
> 
>