Fix lock order inversion between ManagedStatic and Statistic

[inglorion]

Summary

Statistic and ManagedStatic both use mutexes. There was a lock order
inversion where, during initialization, Statistic's mutex would be
held while taking ManagedStatic's, and in llvm_shutdown,
ManagedStatic's mutex would be held while taking Statistic's
mutex. This change causes Statistic's initialization code to avoid
holding its mutex while calling ManagedStatic's methods, avoiding the
inversion.

[dsanders]

llvm/lib/Support/Statistic.cpp

97 ↗

(On Diff #141465)

Are you sure llvm_shutdown() calls Statistic::RegisterStatistic()? It's surprising that it would be constructing and registering new Statistic objects in a function that's intended to destroy them.

[inglorion]

Thanks for looking, @dsanders. Does this comment make things clearer?

[inglorion]

slight reword of comment

[inglorion]

llvm/lib/Support/Statistic.cpp

97 ↗

(On Diff #141465)

Ah, not RegisterStatic, but PrintStatistics. I had intended "us" to mean "functions in this part of the code", but seeing that that wasn't clear, I've changed the comment.

[dsanders]

Sorry for the delay, I was out of the office for a few days.

I see how the inversion comes about now and this makes sense to me. LGTM.

Thanks

llvm/trunk/lib/Support/Statistic.cpp

 static ManagedStatic<StatisticInfo> StatInfo;
 static ManagedStatic<sys::SmartMutex<true> > StatLock;
 
 /// RegisterStatistic - The first time a statistic is bumped, this method is
 /// called.
 void Statistic::RegisterStatistic() {
   // If stats are enabled, inform StatInfo that this statistic should be
   // printed.
-  sys::SmartScopedLock<true> Writer(*StatLock);
+  // llvm_shutdown calls destructors while holding the ManagedStatic mutex.
+  // These destructors end up calling PrintStatistics, which takes StatLock.
+  // Since dereferencing StatInfo and StatLock can require taking the
+  // ManagedStatic mutex, doing so with StatLock held would lead to a lock
+  // order inversion. To avoid that, we dereference the ManagedStatics first,
+  // and only take StatLock afterwards.
   if (!Initialized.load(std::memory_order_relaxed)) {
+    sys::SmartMutex<true> &Lock = *StatLock;
+    StatisticInfo &SI = *StatInfo;
+    sys::SmartScopedLock<true> Writer(Lock);
+    // Check Initialized again after acquiring the lock.
+    if (Initialized.load(std::memory_order_relaxed))
+      return;
     if (Stats || Enabled)
-      StatInfo->addStatistic(this);
+      SI.addStatistic(this);
 
     // Remember we have been registered.
     Initialized.store(true, std::memory_order_release);
   }
 }
 
 StatisticInfo::StatisticInfo() {
   // Ensure timergroup lists are created first so they are destructed after us.