[llvm][cmake] never link llvm-config against llvm dylib

[sterni]

Summary

When cross-compiling, in order to make the output of the native and
cross-compiled llvm-config match, one needs to re-pass all cmake flags
relevant to BuildVariables.inc via CROSS_TOOLCHAIN_FLAGS_NATIVE. If
LLVM_LINK_LLVM_DYLIB=ON is among those, building a full libLLVM shared
object is required for the native llvm-config, otherwise --shared-mode
will be incorrect and --link-shared broken.

To avoid this, we can make llvm-config link statically against the
needed components for simplicity's sake in both the native and cross
case.

[Ericson2314]

I don't want to formally approve this as @sterni and I are both relative outsiders and both coming from downstream, namely NixOS, but the idea makes sense to me.

[Ericson2314]

llvm/tools/llvm-config/CMakeLists.txt

9

I might move the bulk of the commit message to just being a comment here? Otherwise one needs to git blame to find the rationale.

[beanz]

I think this is a good change all around. Since llvm-config should only depend on Support, making it link the dylib is not terribly useful.

@tstellar, do you have any thoughts?

[Ericson2314]

llvm/tools/llvm-config/CMakeLists.txt

9

[beanz]

llvm/tools/llvm-config/CMakeLists.txt

9

That looks good. Please also leave it in the commit message. Having it in two places doesn't hurt, and allows people to see it wherever they are browsing.

[mstorsjo]

What do you think about not passing in LLVM_LINK_LLVM_DYLIB to the internal, chained cmake build for the native tools (or forcing it to false)? Either as complement to or replacement for this patch.

[mstorsjo]

In D116537#3217849, @mstorsjo wrote:

What do you think about not passing in LLVM_LINK_LLVM_DYLIB to the internal, chained cmake build for the native tools (or forcing it to false)? Either as complement to or replacement for this patch.

… because that should fix the same issue when building the native *-tblgen tools too.

[sterni]

In D116537#3217849, @mstorsjo wrote:

What do you think about not passing in LLVM_LINK_LLVM_DYLIB to the internal, chained cmake build for the native tools (or forcing it to false)? Either as complement to or replacement for this patch.

LLVM_LINK_LLVM_DYLIB isn't actually forwarded from the cross build to the native (re)configuration (only very few variables are), but it is actually desirable to pass this explicitly: If I want to cross compile something that depends on LLVM, I'd want native llvm-config to behave exactly as llvm-config would when executed on the target machine. llvm-config's behavior depends on the configuration variables via BuildVariables.inc which is generated independently for native and cross.

So in the case of cross compiling with LLVM_LINK_LLVM_DYLIB=ON, I'd need to pass this to the native build as well — not because I want that to be linked dynamically against libLLVM.so, but rather to make sure that native llvm-config returns the correct results: If I didn't pass it, LLVM_ENABLE_DYLIB and LLVM_LINK_DYLIB would be 0, causing the native llvm-config to believe that the shared libs don't exist at all (despite actually having been built).

So for this case we save ourselves a lot of trouble since building a native llvm-config that links statically against the relevant components is much simpler, while we are still able to pass in a configuration representative of the actual build. The proper way to resolve this situation would probably involve making native and cross-compiled llvm-config use the same BuildVariables.inc, but I'd guess this is hard to achieve with the current scheme.

Does that make sense? Anything I should clarify in the commit message?

[sterni]

Add suggest comment above DISABLE_LLVM_LINK_LLVM_DYLIB

Also reworded commit message slightly to (hopefully) make it a bit clearer.

[mstorsjo]

In D116537#3218590, @sterni wrote:

In D116537#3217849, @mstorsjo wrote:

What do you think about not passing in LLVM_LINK_LLVM_DYLIB to the internal, chained cmake build for the native tools (or forcing it to false)? Either as complement to or replacement for this patch.

LLVM_LINK_LLVM_DYLIB isn't actually forwarded from the cross build to the native (re)configuration (only very few variables are), but it is actually desirable to pass this explicitly: If I want to cross compile something that depends on LLVM, I'd want native llvm-config to behave exactly as llvm-config would when executed on the target machine. llvm-config's behavior depends on the configuration variables via BuildVariables.inc which is generated independently for native and cross.

Ok, so the missing context is that you're manually forwarding this by setting e.g. -DLLVM_LINK_LLVM_DYLIB=ON -DCROSS_TOOLCHAIN_FLAGS_NATIVE="... -DLLVM_LINK_LLVM_DYLIB=ON ..."?

[sterni]

In D116537#3219126, @mstorsjo wrote:

Ok, so the missing context is that you're manually forwarding this by setting e.g. -DLLVM_LINK_LLVM_DYLIB=ON -DCROSS_TOOLCHAIN_FLAGS_NATIVE="... -DLLVM_LINK_LLVM_DYLIB=ON ..."?

Yes.

[mstorsjo]

In D116537#3219158, @sterni wrote:

In D116537#3219126, @mstorsjo wrote:

Ok, so the missing context is that you're manually forwarding this by setting e.g. -DLLVM_LINK_LLVM_DYLIB=ON -DCROSS_TOOLCHAIN_FLAGS_NATIVE="... -DLLVM_LINK_LLVM_DYLIB=ON ..."?

Yes.

Ok, thanks. Don’t you have the same issue with the tblgen tools then too?

[sterni]

In D116537#3219487, @mstorsjo wrote:

Ok, thanks. Don’t you have the same issue with the tblgen tools then too?

If I read llvm/cmake/modules/TableGen.cmake correctly, add_tablegen builds the tablegen tools with DISABLE_LLVM_LINK_LLVM_DYLIB.

Not sure if there are any issues in practice, since we don't use the NATIVE llvm-tblgen for NixOS, since we can just reuse the tool from a different package (built for the builder's platform) which is not an option for llvm-config since the tool needs to produced the same build as the library it is supposed to describe.

[mstorsjo]

In D116537#3219577, @sterni wrote:

In D116537#3219487, @mstorsjo wrote:

Ok, thanks. Don’t you have the same issue with the tblgen tools then too?

If I read llvm/cmake/modules/TableGen.cmake correctly, add_tablegen builds the tablegen tools with DISABLE_LLVM_LINK_LLVM_DYLIB.

Oh, ok then - that even more settles that this patch is the right thing to do, and just applying the existing pattern consistently. Thanks for clarifying!

llvm/tools/llvm-config/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 set(BUILDVARIABLES_SRCPATH ${CMAKE_CURRENT_SOURCE_DIR}/BuildVariables.inc.in)
 set(BUILDVARIABLES_OBJPATH ${CMAKE_CURRENT_BINARY_DIR}/BuildVariables.inc)
 
 # Add the llvm-config tool.
 add_llvm_tool(llvm-config
   llvm-config.cpp
+  # This utility doesn't use much of LLVM, so linking a shared library for the
+  # entire thing is overkill. Avoiding that especially saves on build time when cross
+  # compiling LLVM and building both cross and native `llvm-config`s. We don't
+  # want to build an entire native libLLVM.so in addition to the cross one just
+  # for the native `llvm-config`!
+  DISABLE_LLVM_LINK_LLVM_DYLIB
   )
 
 # Compute the substitution values for various items.
 get_property(SUPPORT_SYSTEM_LIBS TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS)
 get_property(WINDOWSMANIFEST_SYSTEM_LIBS TARGET LLVMWindowsManifest PROPERTY LLVM_SYSTEM_LIBS)
 foreach(l ${SUPPORT_SYSTEM_LIBS} ${WINDOWSMANIFEST_SYSTEM_LIBS})
   if(MSVC)
     set(SYSTEM_LIBS ${SYSTEM_LIBS} "${l}.lib")