ManagedStatic: remove from DebugCounter

[nhaehnle]

Summary

Follow the pattern used in MLIR for the cl::opt instances.

[nhaehnle]

Rebase

[nhaehnle]

• make DebugCounter::isCountingEnabled public so that the DebugCounterOwner doesn't have to be a nested class. This simplifies later changes

[efriedma]

LGTM

[dblaikie]

llvm/lib/Support/DebugCounter.cpp

72–75

This might be an unnecessary layer of indirection - the static member could be in DebugCounter::instance() directly?

[mehdi_amini]

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?)

[nhaehnle]

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

[mehdi_amini]

In D129116#3898503, @nhaehnle wrote:

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?

[nhaehnle]

In D129116#3899375, @mehdi_amini wrote:

In D129116#3898503, @nhaehnle wrote:

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?

[mehdi_amini]

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.

[nhaehnle]

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?

[mehdi_amini]

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.

[nhaehnle]

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.

llvm/include/llvm/Support/DebugCounter.h

 #include <string>
 
 namespace llvm {
 
 class raw_ostream;
 
 class DebugCounter {
 public:
-  ~DebugCounter();
-
   /// Returns a reference to the singleton instance.
   static DebugCounter &instance();
 
   // Used by the command line option parser to push a new value it parsed.
   void push_back(const std::string &);
 
   // Register a counter with the specified name.
   //
public:
 
   // Force-enables counting all DebugCounters.
   //
   // Since DebugCounters are incompatible with threading (not only do they not
   // make sense, but we'll also see data races), this should only be used in
   // contexts where we're certain we won't spawn threads.
   static void enableAllCounters() { instance().Enabled = true; }
 
-private:
   static bool isCountingEnabled() {
 // Compile to nothing when debugging is off
 #ifdef NDEBUG
     return false;
 #else
     return instance().Enabled;
 #endif
   }
 
+private:
   unsigned addCounter(const std::string &Name, const std::string &Desc) {
     unsigned Result = RegisteredCounters.insert(Name);
     Counters[Result] = {};
     Counters[Result].Desc = Desc;
     return Result;
   }
   // Struct to store counter info.
   struct CounterInfo {

llvm/lib/Support/DebugCounter.cpp

 #include "llvm/Support/DebugCounter.h"
 
 #include "DebugOptions.h"
 
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Format.h"
-#include "llvm/Support/ManagedStatic.h"
 
 using namespace llvm;
 
 namespace {
 // This class overrides the default list implementation of printing so we
 // can pretty print the list of debug counter options.  This type of
 // dynamic option is pretty rare (basically this and pass lists).
 class DebugCounterList : public cl::list<std::string, DebugCounter> {
for (const auto &Name : CounterInstance) {
           CounterInstance.getCounterInfo(CounterInstance.getCounterId(Name));
       size_t NumSpaces = GlobalWidth - Info.first.size() - 8;
       outs() << "    =" << Info.first;
       outs().indent(NumSpaces) << " -   " << Info.second << '\n';
     }
   }
 };
 
-struct CreateDebugCounterOption {
-  static void *call() {
-    return new DebugCounterList(
-        "debug-counter", cl::Hidden,
-        cl::desc("Comma separated list of debug counter skip and count"),
-        cl::CommaSeparated, cl::location(DebugCounter::instance()));
+// All global objects associated to the DebugCounter, including the DebugCounter
+// itself, are owned by a single global instance of the DebugCounterOwner
+// struct. This makes it easier to control the order in which constructors and
+// destructors are run.
+struct DebugCounterOwner {
+  DebugCounter DC;
+  DebugCounterList DebugCounterOption{
+      cl::CommaSeparated, cl::location(DC)};
+  cl::opt<bool> PrintDebugCounter{
+      "print-debug-counter", cl::Hidden, cl::init(false), cl::Optional,
+      cl::desc("Print out debug counter info after all counters accumulated")};
+
+  DebugCounterOwner() {
+    // Our destructor uses the debug stream. By referencing it here, we
+    // ensure that its destructor runs after our destructor.
+    (void)dbgs();
   }
-};
-} // namespace
 
-static ManagedStatic<DebugCounterList, CreateDebugCounterOption>
-    DebugCounterOption;
-static bool PrintDebugCounter;
+  // Print information when destroyed, iff command line option is specified.
+  ~DebugCounterOwner() {
+    if (DC.isCountingEnabled() && PrintDebugCounter)
+      DC.print(dbgs());
+  }
 
-void llvm::initDebugCounterOptions() {
-  *DebugCounterOption;
-  static cl::opt<bool, true> RegisterPrintDebugCounter(
-      "print-debug-counter", cl::Hidden, cl::location(PrintDebugCounter),
-      cl::init(false), cl::Optional,
-      cl::desc("Print out debug counter info after all counters accumulated"));
+  static DebugCounterOwner &instance() {
+    static DebugCounterOwner O;
+    return O;
   }
+};
 
-static ManagedStatic<DebugCounter> DC;
+} // anonymous namespace
 
-// Print information when destroyed, iff command line option is specified.
-DebugCounter::~DebugCounter() {
-  if (isCountingEnabled() && PrintDebugCounter)
-    print(dbgs());
-}
+void llvm::initDebugCounterOptions() { (void)DebugCounter::instance(); }
 
-DebugCounter &DebugCounter::instance() { return *DC; }
+DebugCounter &DebugCounter::instance() {
+  return DebugCounterOwner::instance().DC;
+}
 
 // This is called by the command line parser when it sees a value for the
 // debug-counter option defined above.
 void DebugCounter::push_back(const std::string &Val) {
   if (Val.empty())
     return;
   // The strings should come in as counter=value
   auto CounterPair = StringRef(Val).split('=');