This is an archive of the discontinued LLVM Phabricator instance.

ManagedStatic: remove from DebugCounter
ClosedPublic

Authored by nhaehnle on Jul 5 2022, 2:14 AM.

Diff Detail

Event Timeline

nhaehnle created this revision.Jul 5 2022, 2:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 2:14 AM
nhaehnle requested review of this revision.Jul 5 2022, 2:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 2:14 AM
lattner resigned from this revision.Jul 26 2022, 10:44 AM
nhaehnle updated this revision to Diff 449604.Aug 3 2022, 2:31 AM
  • make DebugCounter::isCountingEnabled public so that the DebugCounterOwner doesn't have to be a nested class. This simplifies later changes
This revision is now accepted and ready to land.Aug 3 2022, 11:17 AM
This revision was landed with ongoing or failed builds.Aug 25 2022, 10:10 AM
This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.
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.

Sounds like the equivalent root cause to what was fixed in D134637, and also https://github.com/llvm/llvm-project/issues/58015 sounds related.

Ah great! Do you have some patch to fix the target failing this bot as well?

Sounds like the equivalent root cause to what was fixed in D134637, and also https://github.com/llvm/llvm-project/issues/58015 sounds related.

Ah great! Do you have some patch to fix the target failing this bot as well?

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.