Workaround -Wglobal-constructor warning.

[schweitzpgi]

This line was tripping the -Wglobal-constructor warning which was causing a build failure when -Werror was turned on.

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Eric Schweitz (schweitzpgi)

Changes

This line was tripping the -Wglobal-constructor warning which was causing a build failure when -Werror was turned on.

---

Full diff: https://github.com/llvm/llvm-project/pull/94699.diff

1 Files Affected:

• (modified) llvm/lib/Support/CodeGenCoverage.cpp (+5-2)

diff --git a/llvm/lib/Support/CodeGenCoverage.cpp b/llvm/lib/Support/CodeGenCoverage.cpp
index 4d41c42e527e2..f87cbf18ef1a5 100644
--- a/llvm/lib/Support/CodeGenCoverage.cpp
+++ b/llvm/lib/Support/CodeGenCoverage.cpp
@@ -21,7 +21,10 @@
 
 using namespace llvm;
 
-static sys::SmartMutex<true> OutputMutex;
+static sys::SmartMutex<true> &OutputMutex() {
+  static sys::SmartMutex<true> mutex;
+  return mutex;
+}
 
 CodeGenCoverage::CodeGenCoverage() = default;
 
@@ -79,7 +82,7 @@ bool CodeGenCoverage::parse(MemoryBuffer &Buffer, StringRef BackendName) {
 bool CodeGenCoverage::emit(StringRef CoveragePrefix,
                            StringRef BackendName) const {
   if (!CoveragePrefix.empty() && !RuleCoverage.empty()) {
-    sys::SmartScopedLock<true> Lock(OutputMutex);
+    sys::SmartScopedLock<true> Lock(OutputMutex());
 
     // We can handle locking within a process easily enough but we don't want to
     // manage it between multiple processes. Use the process ID to ensure no

[dwblaikie]

Huh, I'm surprised we're remotely -Wglobal-constructors clean... but guess so, maybe.

Was this a recent regression? Wouldn't hurt to reference the commit hash of the blame and cc the author on this patch?

[schweitzpgi]

This regards the global added by @danielsanders in f76f315.

Just hiding the mutex in a function body to avoid the warning.

[schweitzpgi]

Was this a recent regression?

It was a "recent" regression in that our project was changing how it was built and packaged. But this global variable has been around since 2017.

[schweitzpgi]

Here's the evidence/snippage that the warning/error actually can be triggered in our environ.

$ make
[  0%] Built target LLVMDemangle
[  0%] Built target LLVMSupportBlake3
Consolidate compiler generated dependencies of target LLVMSupport
[  0%] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/CodeGenCoverage.cpp.o
/home/eric/tpls/llvm/llvm/lib/Support/CodeGenCoverage.cpp:24:30: error: declaration requires a global destructor [-Werror,-Wglobal-constructors]
static sys::SmartMutex<true> OutputMutex;
                             ^
1 error generated.
make[2]: *** [lib/Support/CMakeFiles/LLVMSupport.dir/build.make:426: lib/Support/CMakeFiles/LLVMSupport.dir/CodeGenCoverage.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:20373: lib/Support/CMakeFiles/LLVMSupport.dir/all] Error 2
make: *** [Makefile:156: all] Error 2

[dwblaikie]

If it's been around for 7 years, it seems like a bit of a change to start enforcing a build invariant now - and maybe a hassle for you if it's not an invariant that the rest of the developers on LLVM are actively enforcing... (& so may be better for you to disable this warning when building LLVM, rather than trying to clean it up)

And is this really the only instance/problem? As much as LLVM's tried to avoid global ctors, my understanding was that it was far from clean. I'd imagine once this is fixed, you'd hit more/other instances of this?

[efriedma-quic]

libSupport is supposed to support this... but there's a specific carveout for targets that don't support constant initialization of mutexes. See https://reviews.llvm.org/D105959 / 402461b / etc.

CC @joker-eph

[dwblaikie]

libSupport is supposed to support this... but there's a specific carveout for targets that don't support constant initialization of mutexes. See https://reviews.llvm.org/D105959 / 402461b / etc.

CC @joker-eph

oooh right... I remember that vaguely.

Though in this case it's a global /dtor/ hmm, nope, the carveout seems to do the right thing for that+mutexes.

SmartMutex has a recursive_mutex and unsigned member, so it should have the global ctor/dtor impact that recursive_mutex has, which is tested by the carveout @efriedma-quic linked...

@schweitzpgi could you look into why that carveout isn't working for you?

[joker-eph]

If this is the only place it is used, you can do simpler I believe:

Suggested change

	sys::SmartScopedLock<true> Lock(OutputMutex());
	static sys::SmartMutex<true> OutputMutex;
	sys::SmartScopedLock<true> Lock(OutputMutex);

The global is only useful if used in multiple places.

[schweitzpgi]

Since the variable is static, this is the only use.

[schweitzpgi]

If it's been around for 7 years, it seems like a bit of a change to start enforcing a build invariant now - and maybe a hassle for you if it's not an invariant that the rest of the developers on LLVM are actively enforcing... (& so may be better for you to disable this warning when building LLVM, rather than trying to clean it up)

And is this really the only instance/problem? As much as LLVM's tried to avoid global ctors, my understanding was that it was far from clean. I'd imagine once this is fixed, you'd hit more/other instances of this?

It is the only instance. It would be nice to fix this upstream instead of have to patch around it in the build environment since it is trivial.

[schweitzpgi]

@schweitzpgi could you look into why that carveout isn't working for you?

No clue.

[efriedma-quic]

LGTM

As a followup, if we really don't need the carveout for mutexes anymore, could you look at removing it?