Follow the pattern used in MLIR for the cl::opt instances.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
- make DebugCounter::isCountingEnabled public so that the DebugCounterOwner doesn't have to be a nested class. This simplifies later changes
llvm/lib/Support/DebugCounter.cpp | ||
---|---|---|
72–75 | This might be an unnecessary layer of indirection - the static member could be in DebugCounter::instance() directly? |
Seems like this leads to an issue with one of our build config:
: CommandLine Error: Option 'debug-counter' registered more than once! LLVM ERROR: inconsistency in registered CommandLine options /tmp/ci-m2DdQ5vcSh/tools/mlir/test/mlir-pdll-lsp-server/Output/exit-with-shutdown.test.script: line 1: 27186 Aborted (core dumped) mlir-pdll-lsp-server -lit-test < /var/lib/buildkite-agent/builds/buildkite-588bd64db9-w8hgc-1/mlir/mlir-core/mlir/test/mlir-pdll-lsp-server/exit-with-shutdown.test
See log and cmake invocation here: https://buildkite.com/mlir/mlir-core/builds/26578#01842a7b-cfac-4cce-aecd-70d10fb3e088
(it might be something to fix in how mlir-pdll-lsp-server is defined?)
Sounds like the equivalent root cause to what was fixed in D134637, and also https://github.com/llvm/llvm-project/issues/58015 sounds related.
No, I don't. I wasn't aware of that bot (didn't receive/see an email) and don't know its configuration. Actually, perhaps it requires exactly the change that Tobias Grosser posted on that GitHub issue?
That why I sent a link to the log with the cmake invocation so that you can reproduce and fix it...
Simplifying the cmake from the log in the link I sent on Monday, this should be enough to reproduce:
cmake -G Ninja ${src_dir}/llvm -DLLVM_TARGETS_TO_BUILD="X86" -DLLVM_ENABLE_PROJECTS=mlir -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=Off -DLLVM_LINK_LLVM_DYLIB=ON
It's very likely that ninja mlir-pdll-lsp-server and ./bin/mlir-pdll-lsp-server would be enough to repro.
It doesn't repro for me. That may just be a toolchain difference. It seems my linker drops the libLLVM-*.so dependency once it sees that no symbol is needed. Are you able to test with Tobias Grosser's patch?
I didn't try before, it is reproducing on my machine. The link command (that I get by doing after a first build: rm bin/mlir-pdll-lsp-server && ninja bin/mlir-pdll-lsp-server -v):
/usr/bin/c++ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -Wl,-rpath-link,/usr/local/google/home/aminim/projects/llvm-project/build-dylib/./lib -Wl,--gc-sections tools/mlir/tools/mlir-pdll-lsp-server/CMakeFiles/mlir-pdll-lsp-server.dir/mlir-pdll-lsp-server.cpp.o -o bin/mlir-pdll-lsp-server -Wl,-rpath,"\$ORIGIN/../lib" lib/libMLIRPdllLspServerLib.a lib/libMLIRPDLLCodeGen.a lib/libMLIRParser.a lib/libMLIRBytecodeReader.a lib/libMLIRAsmParser.a lib/libMLIRPDLDialect.a lib/libMLIRInferTypeOpInterface.a lib/libMLIRSideEffectInterfaces.a lib/libMLIRIR.a lib/libMLIRPDLLParser.a lib/libMLIRPDLLAST.a lib/libMLIRPDLLODS.a lib/libMLIRTableGen.a lib/libLLVMTableGen.a lib/libLLVMSupport.a -lrt -ldl -lm /usr/lib/x86_64-linux-gnu/libz.so /usr/lib/x86_64-linux-gnu/libtinfo.so lib/libLLVMDemangle.a lib/libMLIRLspServerSupportLib.a lib/libMLIRSupport.a lib/libLLVM-16git.so
If I remove the lib/libLLVM-16git.so from the command line I get:
/usr/bin/ld: lib/libMLIRLspServerSupportLib.a(CompilationDatabase.cpp.o): in function `std::enable_if<llvm::yaml::has_ScalarTraits<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::value, void>::type llvm::yaml::yamlize<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >(llvm::yaml::IO&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, bool, llvm::yaml::EmptyContext&) [clone .isra.0]': CompilationDatabase.cpp:(.text._ZN4llvm4yaml7yamlizeINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEENSt9enable_ifIXsrNS0_16has_ScalarTraitsIT_EE5valueEvE4typeERNS0_2IOERSA_bRNS0_12EmptyContextE.isra.0+0x99): undefined reference to `llvm::yaml::IO::getContext() const' /usr/bin/ld: CompilationDatabase.cpp:(.text._ZN4llvm4yaml7yamlizeINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEENSt9enable_ifIXsrNS0_16has_ScalarTraitsIT_EE5valueEvE4typeERNS0_2IOERSA_bRNS0_12EmptyContextE.isra.0+0xa7): undefined reference to `llvm::yaml::ScalarTraits<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, void>::output(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, void*, llvm::raw_ostream&)' /usr/bin/ld: CompilationDatabase.cpp:(.text._ZN4llvm4yaml7yamlizeINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEENSt9enable_ifIXsrNS0_16has_ScalarTraitsIT_EE5valueEvE4typeERNS0_2IOERSA_bRNS0_12EmptyContextE.isra.0+0x14e): undefined reference to `llvm::yaml::IO::getContext() const' /usr/bin/ld: CompilationDatabase.cpp:(.text._ZN4llvm4yaml7yamlizeINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEEENSt9enable_ifIXsrNS0_16has_ScalarTraitsIT_EE5valueEvE4typeERNS0_2IOERSA_bRNS0_12EmptyContextE.isra.0+0x163): undefined reference to `llvm::yaml::ScalarTraits<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, void>::input(llvm::StringRef, void*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)' /usr/bin/ld: lib/libMLIRLspServerSupportLib.a(CompilationDatabase.cpp.o): in function `mlir::lsp::CompilationDatabase::loadDatabase(llvm::StringRef) [clone .localalias]': CompilationDatabase.cpp:(.text._ZN4mlir3lsp19CompilationDatabase12loadDatabaseEN4llvm9StringRefE+0x95): undefined reference to `llvm::yaml::Input::Input(llvm::StringRef, void*, void (*)(llvm::SMDiagnostic const&, void*), void*)' /usr/bin/ld: CompilationDatabase.cpp:(.text._ZN4mlir3lsp19CompilationDatabase12loadDatabaseEN4llvm9StringRefE+0xe0): undefined reference to `llvm::yaml::Input::setCurrentDocument()' /usr/bin/ld: CompilationDatabase.cpp:(.text._ZN4mlir3lsp19CompilationDatabase12loadDatabaseEN4llvm9StringRefE+0x146): undefined reference to `llvm::yaml::Input::beginMapping()' /usr/bin/ld: CompilationDatabase.cpp:(.text._ZN4mlir3lsp19CompilationDatabase12loadDatabaseEN4llvm9StringRefE+0x162): undefined reference to `llvm::yaml::Input::preflightKey(char const*, bool, bool, bool&, void*&)' /usr/bin/ld: CompilationDatabase.cpp:(.text._ZN4mlir3lsp19CompilationDatabase12loadDatabaseEN4llvm9StringRefE+0x329): undefined reference to `llvm::yaml::Input::preflightKey(char const*, bool, bool, bool&, void*&)' /usr/bin/ld: CompilationDatabase.cpp:(.text._ZN4mlir3lsp19CompilationDatabase12loadDatabaseEN4llvm9StringRefE+0x80c): undefined reference to `llvm::yaml::Input::endMapping()' /usr/bin/ld: CompilationDatabase.cpp:(.text._ZN4mlir3lsp19CompilationDatabase12loadDatabaseEN4llvm9StringRefE+0x814): undefined reference to `llvm::yaml::Input::error()' /usr/bin/ld: CompilationDatabase.cpp:(.text._ZN4mlir3lsp19CompilationDatabase12loadDatabaseEN4llvm9StringRefE+0x82c): undefined reference to `llvm::yaml::Input::nextDocument()' /usr/bin/ld: CompilationDatabase.cpp:(.text._ZN4mlir3lsp19CompilationDatabase12loadDatabaseEN4llvm9StringRefE+0x834): undefined reference to `llvm::yaml::Input::setCurrentDocument()' /usr/bin/ld: CompilationDatabase.cpp:(.text._ZN4mlir3lsp19CompilationDatabase12loadDatabaseEN4llvm9StringRefE+0xcd7): undefined reference to `llvm::yaml::Input::postflightKey(void*)' /usr/bin/ld: CompilationDatabase.cpp:(.text._ZN4mlir3lsp19CompilationDatabase12loadDatabaseEN4llvm9StringRefE+0xcf7): undefined reference to `llvm::yaml::Input::postflightKey(void*)' /usr/bin/ld: CompilationDatabase.cpp:(.text._ZN4mlir3lsp19CompilationDatabase12loadDatabaseEN4llvm9StringRefE+0x1129): undefined reference to `llvm::yaml::Input::~Input()' /usr/bin/ld: lib/libMLIRLspServerSupportLib.a(Logging.cpp.o): in function `llvm::detail::provider_format_adapter<std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> > >&>::format(llvm::raw_ostream&, llvm::StringRef)': Logging.cpp:(.text._ZN4llvm6detail23provider_format_adapterIRNSt6chrono10time_pointINS2_3_V212system_clockENS2_8durationIlSt5ratioILl1ELl1000000000EEEEEEE6formatERNS_11raw_ostreamENS_9StringRefE[_ZN4llvm6detail23provider_format_adapterIRNSt6chrono10time_pointINS2_3_V212system_clockENS2_8durationIlSt5ratioILl1ELl1000000000EEEEEEE6formatERNS_11raw_ostreamENS_9StringRefE]+0x5): undefined reference to `llvm::format_provider<std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> > >, void>::format(std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> > > const&, llvm::raw_ostream&, llvm::StringRef)' collect2: error: ld returned 1 exit status
I'm not sure how you got these symbols or how you got a binary that don't link to the .so? Are you having a specific default toolchain on your machine (I am using plain gcc/bfd here).
Assuming we're talking about this patch: https://github.com/llvm/llvm-project/issues/58015#issuecomment-1279343628 ; then I just tried and it does not help. But looking at the thread, I'm not sure why it should actually. Seems like a different problem to me really.
Thanks, I can reproduce this now. I was using LLD, and that behaved differently from ld.bfd.
I dug into it now. It's a hairy tangle. There are, among other things, the following relevant dependencies that I found:
mlir-pdll-lsp-server -> MLIRPDLLParser --(static)--> LLVMSupport mlir-pdll-lsp-server -> MLIRPDLLParser --> MLIRSupport --(dynamic)--> LLVMSupport mlir-pdll-lsp-server -> MLIRLspServerSupportLib --> MLIRSupport --(dynamic) --> LLVMSupport ; last dependency implicit from add_mlir_library mlir-lsp-server -> MLIRLspServerLib --> (lots of MLIR libraries) --> MLIRSupport --(dynamic)--> LLVMSupport mlir-lsp-server -> MLIRLspServerLib --> MLIRLspServerSupportLib --> (see above) mlir-pdll -> MLIRPDLLParser --> (see above) mlir-tblgen --(static)--> LLVMSupport
So to try and sum it up:
- mlir-lsp-server really "wants to" link dynamically to LLVMSupport
- mlir-pdll-lsp-server shares MLIRLspServerSupportLib with mlir-lsp-server, and so really also "wants to" link dynamically to LLVMSupport, but is broken because MLIRPDLLParser is confused about static vs. dynamic linking
- mlir-pdll shares MLIRPDLLParser with mlir-pdll-lsp-server, and so maybe also "wants to" link dynamically to LLVMSupport? But it, too, is confused, plus it's a tablegen-style tools which are traditionally statically linked
How important is it actually to try to avoid having these tools link against libLLVM-*.so? Upon reflection, I don't actually understand why. Initially, I thought this is for native llvm-tblgen and clang-tblgen builds when cross-compiling: we don't want these to depend on libLLVM-*.so. But then, during cross-compilation the native build works by executing cmake recursively, so can't the recursive cmake invocation just force LLVM_LINK_LLVM_DYLIB=OFF?
So my feeling is that the least painful solution in the long run is to just get rid of all the DISABLE_LLVM_LINK_LLVM_DYLIB for tablegen-y tool, starting in MLIR but eventually propagating that change to other parts of llvm-project. Would that be okay for MLIR? I can take a stab at it, there are some more dependencies that would have to be taken care of first.
This might be an unnecessary layer of indirection - the static member could be in DebugCounter::instance() directly?