[PATCH] D134637: clang-tblgen build: avoid duplicate inclusion of libLLVMSupport

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

+1 from me also, but someone else should check that this is a reasonable way to implement it cmake wise (not that this is a horrible hack but I never can tell with cmake).

One more question, does this same issue potentially apply to llvm-tblgen and has that got any special cmake changes to account for it? (if it doesn't, leave it as is, but as a comparison point)



================
Comment at: clang/lib/Support/CMakeLists.txt:21
+  # libLLVM-*.so, to be used by clang-tblgen. This is so clang-tblgen doesn't
+  # accidentally link against libLLVMSupport twice (once statically and once via
+  # libLLVM-*.so).
----------------
nhaehnle wrote:
> DavidSpickett wrote:
> > Without this change, is it the case that it will always link against libLLVMSupport twice with the DYLIB conifg?
> > 
> > "accidentally" sounds like you could stumble into it but from what I see, it's always been doing this and once your other change lands, it would always result in an error.
> > ```
> > This is so clang-tblgen doesn't link against libLLVMSupport twice (once statically and once via libLLVM-*.so).
> > ```
> I meant "accidentally" in the sense that *-tblgen isn't supposed to link against libLLVM-*.so, but ended up doing so after clangSupport was added earlier this year. Perhaps I should just remover the adverb?
That would work, otherwise it seems like a thing that sometimes happens under conditions that aren't explained.


================
Comment at: clang/lib/Support/CMakeLists.txt:26
+    DISABLE_LLVM_LINK_LLVM_DYLIB
+    ${clangSupport_sources})
+endif()
----------------
nhaehnle wrote:
> DavidSpickett wrote:
> > Can you detail which targets link to/include what and how the problem happens? I'm trying to understand why we can't just use `DISABLE_LLVM_LINK_LLVM_DYLIB` on its own here.
> clangSupport is included by clang-tblgen but also by libclangcpp. The underlying idea is that of all the users of clangSupport, clang-tblgen is special because it uses the DISABLE_LLVM_LINK_LLVM_DYLIB override.
> 
> clangSupport links against Support, which becomes a link against libLLVM-*.so with LLVM_LINK_LLVM_DYLIB=ON. So, in an LLVM_LINK_LLVM_DYLIB=ON build, we get with this change:
> 
> - clangSupport links against Support, which becomes dynamically linking against libLLVM-*.so (this is unchanged)
> - clangSupport_tablegen links against Support statically
> - clang-tblgen links against clangSupport_tablegen (and also directly against Support) statically
> - other users of clangSupport link against clangSupport somehow, and then transitively dynamically against libLLVM-*.so
> 
> Does that answer your questions?
> 
> Specifically, if we were to just add DISABLE_LLVM_LINK_LLVM_DYLIB to clangSupport, then we risk a situation where some other user of clangSupport links against Support twice; once via the copy of Support that is statically linked to from clangSupport; and once via libLLVM-*.so that gets pulled in via other dependencies. To be honest, I don't know for certain whether that is a problem that would happen, but it seemed likely enough to me that I wouldn't want to risk it.
> Does that answer your questions?

Yes but I don't think I have the expertise to say this is a good way to implement this change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134637/new/

https://reviews.llvm.org/D134637