Page MenuHomePhabricator

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

Authored by sterni on Jan 3 2022, 6:31 AM.

Details

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.

Diff Detail

Unit TestsFailed

TimeTest
60 msx64 debian > LLVM.Bindings/Go::go.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llvm-go go=/usr/bin/go test llvm.org/llvm/bindings/go/llvm

Event Timeline

sterni created this revision.Jan 3 2022, 6:31 AM
sterni requested review of this revision.Jan 3 2022, 6:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2022, 6:31 AM

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 added inline comments.Jan 3 2022, 9:49 AM
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.

Ericson2314 edited the summary of this revision. (Show Details)
beanz accepted this revision.Jan 3 2022, 9:56 AM
beanz added a subscriber: tstellar.

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?

This revision is now accepted and ready to land.Jan 3 2022, 9:56 AM
Ericson2314 added inline comments.Jan 3 2022, 9:59 AM
llvm/tools/llvm-config/CMakeLists.txt
9
beanz added inline comments.Jan 3 2022, 10:05 AM
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.

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.

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 added a comment.Jan 3 2022, 5:31 PM

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 updated this revision to Diff 397167.Jan 3 2022, 5:41 PM

Add suggest comment above DISABLE_LLVM_LINK_LLVM_DYLIB

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

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 added a comment.Jan 4 2022, 2:25 AM

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, 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 added a comment.Jan 4 2022, 6:11 AM

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.

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!

This revision was landed with ongoing or failed builds.Jan 4 2022, 9:34 AM
This revision was automatically updated to reflect the committed changes.