Page MenuHomePhabricator

Replace MCTargetOptionsCommandFlags.inc and CommandFlags.inc by runtime-registration
ClosedPublic

Authored by serge-sans-paille on Mar 3 2020, 3:54 PM.

Details

Summary

MCTargetOptionsCommandFlags.inc and CommandFlags.inc are headers which contain cl::opt with static storage.
These headers are meant to be incuded by tools to make it easier to parametrize codegen/mc.

However, these headers are also included in at least two libraries: lldCommon and handle-llvm. As a result, when creating DYLIB, clang-cpp holds a reference to the options, and lldCommon holds another reference. Linking the two in a single executable, as zig does[0], results in a double registration.

This patch explores an other approach: the .inc files are moved to regular files, and the registration happens on-demand through static declaration of options in the constructor of a static object.

[0] https://bugzilla.redhat.com/show_bug.cgi?id=1756977#c5

Diff Detail

Event Timeline

Herald added a reviewer: andreadb. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript

On Arch Linux side, I reported a similar issue https://bugs.archlinux.org/task/64464#comment183541 for ldc (LLVM-based D compiler)

% cmake -H. -BArch -G Ninja -DCMAKE_BUILD_TYPE=Debug -DD_COMPILER=$HOME/dlang/dmd-2.089.0/linux/bin64/dmd -DCMAKE_CXX_STANDARD=14 -DLDC_WITH_LLD=On
% ninja -C Arch
...
: CommandLine Error: Option 'mc-relax-all' registered more than once!
% Arch/bin/ldc2
: CommandLine Error: Option 'mc-relax-all' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

I agree that moving global constructors from .inc files into lib/CodeGen/CommandFlags/CommandFlags.cpp is the correct direction.

Testing several configurations helps capture build problems early:

  • -DBUILD_SHARED_LIBS=off (default)
  • -DBUILD_SHARED_LIBS=off -DLLVM_LINK_LLVM_DYLIB=on -DCLANG_LINK_CLANG_DYLIB=on
  • -DBUILD_SHARED_LIBSS=on

Fix lto-test bugs + convert more files, still not ready for review.

This passes the following configurations:

-DLLVM_ENABLE_PROJECTS='clang;clang-tools-extra;lld' -DBUILD_SHARED_LIBS=on -DLLVM_LINK_LLVM_DYLIB=off -DCLANG_LINK_CLANG_DYLIB=off

-DLLVM_ENABLE_PROJECTS='clang;clang-tools-extra;lld' -DBUILD_SHARED_LIBS=off -DLLVM_LINK_LLVM_DYLIB=off -DCLANG_LINK_CLANG_DYLIB=off

-DLLVM_ENABLE_PROJECTS='clang;clang-tools-extra;lld' -DBUILD_SHARED_LIBS=off -DLLVM_LINK_LLVM_DYLIB=on -DCLANG_LINK_CLANG_DYLIB=on

MaskRay added inline comments.Mar 4 2020, 3:21 PM
llvm/tools/llvm-mc-assemble-fuzzer/CMakeLists.txt
14

Delete comment.

MaskRay accepted this revision.Mar 4 2020, 3:22 PM
This revision is now accepted and ready to land.Mar 4 2020, 3:22 PM

@MaskRay The library version prooved to be a bad path: we wanted clang-cpp to always link to a shared library, which interacts in ungraceful ways with BUILD_SHARED_LIBS.

This is another attempt that looks better to me: all options are available in LLVMCodeGen, but only registered if a given static constructor is called by client code. Some doc and asserts are missing but that's the rough idea. Any thoughts?

bcain added a subscriber: bcain.Mar 9 2020, 8:46 AM
MaskRay added a comment.EditedMar 9 2020, 6:32 PM

@MaskRay The library version prooved to be a bad path: we wanted clang-cpp to always link to a shared library, which interacts in ungraceful ways with BUILD_SHARED_LIBS.

This is another attempt that looks better to me: all options are available in LLVMCodeGen, but only registered if a given static constructor is called by client code. Some doc and asserts are missing but that's the rough idea. Any thoughts?

I think the patch is moving toward the correct direction and fixes some problems, so there is still value to land it. Placing static constructors in a header which may be included in two places in an application (application codegen; lld depended by the application) is bad. With this patch, at least the -DBUILD_SHARED_LIBS=off build (libLLVM*.a) will work now.

(A -DBUILD_SHARED_LIBS=on build will also work. In this case, the application should link against libLLVM*.so and libclang*.so, but no distribution has done this. The application will have to link against a number of libclang*.so, instead of a single libclang-cpp.so)

A copy of libLLVMCoreGen.a is linked into libclang-cpp.so. If the application depends on libclang-cpp.so, it cannot pull in another libLLVMCodeGen.a, because that will cause duplicate registration of llvm::cl::opt flags.
I agree that we have to let client code explicitly initialize (register) the flags.

Cleaned up code, using a few macros to avoid duplication.

No longer a WIP

@MaskRay

because that will cause duplicate registration of llvm::cl::opt flags.

Not with this setup, if I'm not mistaken: the static registration is triggered by the static variables inside the constructor call, and if I'm not mistaken, initialization of such variables only happens once. Basically, this relies on the unicity of the constructor symbol, and this is granted by the fact there's only one lib defining it (either libLLVM or libLLVMCodeGen).

@MaskRay are you ok with that version?

MaskRay accepted this revision.Mar 16 2020, 1:36 PM

The title still says this is a WIP..

serge-sans-paille retitled this revision from [WIP] Replace MCTargetOptionsCommandFlags.inc and CommandFlags.inc by libraries to Replace MCTargetOptionsCommandFlags.inc and CommandFlags.inc by libraries.Mar 16 2020, 2:05 PM
serge-sans-paille retitled this revision from Replace MCTargetOptionsCommandFlags.inc and CommandFlags.inc by libraries to Replace MCTargetOptionsCommandFlags.inc and CommandFlags.inc by runtime-registration.
serge-sans-paille edited the summary of this revision. (Show Details)

Renamed registration constructor to use a more llvm-ish name.

MaskRay accepted this revision.Mar 16 2020, 2:31 PM
This revision was automatically updated to reflect the committed changes.

I may be wrong, but I believe this breaks check-llvm when run with the following configuration: cmake -DLLVM_BINUTILS_INCDIR=/path/to/binutils/include:

Failing Tests (6):
    LLVM :: tools/gold/X86/common_thinlto.ll
    LLVM :: tools/gold/X86/emit-llvm.ll
    LLVM :: tools/gold/X86/pr25907.ll
    LLVM :: tools/gold/X86/relax-relocs.ll
    LLVM :: tools/gold/X86/relocation-model-pic.ll
    LLVM :: tools/gold/X86/strip_names.ll

When bisecting for those specific test failures, I found this commit. The issue, I believe, is with how those tests pass options to the LLVMgold plugin. For what it's worth, my binutils reports version 2.27.

@modocache : thanks for the heads up, I'm investigating the issue.

Awesome, thank you!