clang-tblgen build: avoid duplicate inclusion of libLLVMSupport

[nhaehnle]

Summary

TableGen executables are supposed to never be linked against libLLVM-*.so,
even when LLVM_LINK_LLVM_DYLIB=ON, presumably for cross-compilation.

It turns out that clang-tblgen *did* link against libLLVM-*.so,
indirectly so via the clangSupport.

This lead to a regression in what should have been unrelated work of
cleaning up ManagedStatics in LLVMSupport. A running clang-tblgen
process ended up with two copies of a cl::opt static global:

• one from libLLVMSupport linked statically into clang-tblgen as a direct dependency
• one from libLLVMSupport linked into libLLVM-*.so, which clang-tblgen linked against due to the clangSupport dependency

For a bit more context, see the discussion at
https://discourse.llvm.org/t/flang-aarch64-dylib-buildbot-need-help-understanding-a-regression-in-clang-tblgen/64871/

None of the potential solutions I could find are perfect. Presumably one
possible solution would be to remove "Support" from the explicit
dependencies of clang-tblgen. However, relying on the transitive
inclusion via clangSupport seems risky, and in any case this wouldn't
address the issue of clang-tblgen surprisingly linking against libLLVM-*.so.

This change instead creates a second version of the clangSupport library
that is explicitly linked statically, to be used by clang-tblgen.

[mstorsjo]

Nice, +1 this looks reasonable to me. I've run into similar issues occasionally with mingw builds with dylib enabled too, where (depending on how the linker ends up pulling in objects) you can end up with duplicate definitions etc.

I would expect that this also fixes the somewhat surprising issue, where building clang-tblgen alone ends up building way way more object files than expected, if dylibs are enabled.

[DavidSpickett]

clang/lib/Support/CMakeLists.txt

21

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

26

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.

[nhaehnle]

clang/lib/Support/CMakeLists.txt

21

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?

26

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.

[DavidSpickett]

+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)

clang/lib/Support/CMakeLists.txt

21

That would work, otherwise it seems like a thing that sometimes happens under conditions that aren't explained.

26

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.

[mstorsjo]

Adding @beanz who might have some more cmake knowledge on whether this is the best/least bad way of doing things.

clang/lib/Support/CMakeLists.txt

26

FWIW I did try to implement something similar for some of the MLIR tablegen tools too, and I ended up doing something very similar (splitting support libraries into a regular and static-only variant, but for a deeper hierarcy of libraries) - but I haven't taken time to try to upstream that yet. If we get this as generic pattern for how to do it, I could try to upstream those patches later too.

[aaron.ballman]

Adding the CMake code owners as reviewers for input on this.

[beanz]

I think this approach mostly looks sane to me. @phosek, and @Ericson2314 may have different feedback.

clang/lib/Support/CMakeLists.txt

23

Unless there is a reason not to you should probably use add_llvm_library here probably with the BUILDTREE_ONLY option.

27

We could add an else here that creates the clangSupport_tablegen target as an alias of clangSupport

add_library(clangSupport_tablegen ALIAS clangSupport)

See: https://cmake.org/cmake/help/v3.13/command/add_library.html#alias-libraries

This would allow clang-tablegen to always depend on clangSupport_tablegen simplifying the code on that end.

[phosek]

A possible alternative solution would be to build clangSupport_sources as an object library, and then link that library into clangSupport and clang-tblgen which could be done unconditionally; the advantage is that you don't need to compile clangSupport_sources twice.

[nhaehnle]

• define an alias so that clang-tblgen can always link against "clangSupport_tablegen"
• use add_llvm_library(... BUILDTREE_ONLY ...)

[nhaehnle]

Remove the confusing "accidentally"

[nhaehnle]

Thank you all for the reviews. I've integrated the suggestions except for:

A possible alternative solution would be to build clangSupport_sources as an object library, and then link that library into clangSupport and clang-tblgen which could be done unconditionally; the advantage is that you don't need to compile clangSupport_sources twice.

I'm not sure how this would work. It doesn't seem to be something with precedent in the LLVM tree, and seems to require using raw CMake add_library, though it's quite likely that I missed something?

clang/lib/Support/CMakeLists.txt

23

That seems to work, thank you.

27

Good idea, done.

[phosek]

In D134637#3835935, @nhaehnle wrote:

Thank you all for the reviews. I've integrated the suggestions except for:

A possible alternative solution would be to build clangSupport_sources as an object library, and then link that library into clangSupport and clang-tblgen which could be done unconditionally; the advantage is that you don't need to compile clangSupport_sources twice.

I'm not sure how this would work. It doesn't seem to be something with precedent in the LLVM tree, and seems to require using raw CMake add_library, though it's quite likely that I missed something?

We support building object libraries, see https://github.com/llvm/llvm-project/blob/702d937f1e1d42892ab43d1b591f5041ce2f4e78/llvm/cmake/modules/AddLLVM.cmake#L443, which should be already set for clangSupport, see https://github.com/llvm/llvm-project/blob/702d937f1e1d42892ab43d1b591f5041ce2f4e78/clang/cmake/modules/AddClang.cmake#L102. In this case, it may be sufficient, to just link obj.clangSupport (that is, you'd use target_link_libraries(clang-tblgen PRIVATE obj.clangSupport)).

[nhaehnle]

• use the object library version of clangSupport if available

Thank you for the pointers. Using this object library makes a lot of
sense to me and it does appear to work. Judging by AddClang.cmake though,
the object library is only available if (NOT XCODE), so I kept the
previous way of doing things as an alternative. In all cases, I'm using an
alias so that the complexity is contained within the clangSupport CMakeLists.txt

[nhaehnle]

Gentle ping :)

If there are no objections, I'd like to commit this in a few days.

[nhaehnle]

Ping^2 after ~a week.

The previous discussion seemed quite favorable, so is this just a case of everybody being too afraid to say "Yes" because this is a dark corner that few people ever look at? ;)

[mstorsjo]

LGTM, this looks reasonable to me. (Although wait a little bit more if someone else responds to the ping this time...)

clang/lib/Support/CMakeLists.txt

 set(LLVM_COMMON_DEPENDS_OLD ${LLVM_COMMON_DEPENDS})
 
 # Drop clang-tablegen-targets from LLVM_COMMON_DEPENDS.
 # so that we could use clangSupport within clang-tblgen and other clang
 # component.
 list(REMOVE_ITEM LLVM_COMMON_DEPENDS clang-tablegen-targets)
 
 set(LLVM_LINK_COMPONENTS
   Support
   )
 
-add_clang_library(clangSupport
+set(clangSupport_sources
   RISCVVIntrinsicUtils.cpp
   )
 
+add_clang_library(clangSupport ${clangSupport_sources})
+
+if (NOT XCODE)
+  add_library(clangSupport_tablegen ALIAS obj.clangSupport)
+elseif (NOT LLVM_LINK_LLVM_DYLIB)
+  add_library(clangSupport_tablegen ALIAS clangSupport)
+else()
+  # Build a version of the support library that does not link against
+  # libLLVM-*.so, to be used by clang-tblgen. This is so clang-tblgen doesn't
+  # link against libLLVMSupport twice (once statically and once via
+  # libLLVM-*.so).
+  add_llvm_library(clangSupport_tablegen
+    BUILDTREE_ONLY STATIC DISABLE_LLVM_LINK_LLVM_DYLIB
+    ${clangSupport_sources})
+endif()
+
 set(LLVM_COMMON_DEPENDS ${LLVM_COMMON_DEPENDS_OLD})

clang/utils/TableGen/CMakeLists.txt

add_tablegen(clang-tblgen CLANG
   ClangTypeNodesEmitter.cpp
   MveEmitter.cpp
   NeonEmitter.cpp
   RISCVVEmitter.cpp
   SveEmitter.cpp
   TableGen.cpp
   )
 
-target_link_libraries(clang-tblgen PRIVATE clangSupport)
+target_link_libraries(clang-tblgen PRIVATE clangSupport_tablegen)
 
 set_target_properties(clang-tblgen PROPERTIES FOLDER "Clang tablegenning")