[Support] Use atomics in DebugCounter to silence TSAN

[ilya-biryukov]

Summary

Note that the change does not aim to define semantics of DebugCounter
for multi-threaded code.

[ilya-biryukov]

This change is a bit ugly since it has to account for the value of LLVM_ENABLE_THREADS...
Happy to accept suggestions/alternative ways to fix this :-)

[george.burgess.iv]

Thanks for this!

I totally agree that DebugCounters, in their current state, make no sense to really use with threads. So, I'm not the biggest fan of silencing the tools that're correctly telling us "hey, you're holding these DebugCounters wrong."

Might it be reasonable to just have an enabled bit inside of the DebugCounters instance()? If true, we'll do all of this counting. If false, we'll just skip it all? (Logically, it would be set to "were we passed any -debug-counter args, or do we plan to run the pass from D50031?")

Also, if this is still breaking you (or anyone else), please let me know. I'm happy to revert the problematic patch while we work this out if it makes anyone's life easier. :)

[ilya-biryukov]

Abandoning in favor of George's fix.

In D50080#1182820, @george.burgess.iv wrote:

Might it be reasonable to just have an enabled bit inside of the DebugCounters instance()? If true, we'll do all of this counting. If false, we'll just skip it all? (Logically, it would be set to "were we passed any -debug-counter args, or do we plan to run the pass from D50031?")

LG. Anything that keeps TSAN happy is good with our use-case, there's does not seem to be a way to make debug counters useful in the multi-threaded environment anyway.

Also, if this is still breaking you (or anyone else), please let me know. I'm happy to revert the problematic patch while we work this out if it makes anyone's life easier. :)

It's tensorflow under TSAN that's broken, they do run LLVM code that uses debug counters in parallel and get those errors from TSAN.

include/llvm/Support/DebugCounter.h

 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/UniqueVector.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include <string>
 
+#if LLVM_ENABLE_THREADS
+#include <atomic>
+#endif
+
 namespace llvm {
 
 class DebugCounter {
 public:
   /// Returns a reference to the singleton instance.
   static DebugCounter &instance();
 
   // Used by the command line option parser to push a new value it parsed.
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 {
+    #if LLVM_ENABLE_THREADS
+    CounterInfo() = default;
+    CounterInfo(const CounterInfo &Other) { *this = Other; }
+    CounterInfo &operator =(const CounterInfo &Other) {
+      Count = Other.Count.load();
+      Skip = Other.Skip.load();
+      StopAfter = Other.StopAfter.load();
+      IsSet = Other.IsSet.load();
+      Desc = Other.Desc;
+      return *this;
+    }
+
+    // We use atomics merely to silence TSAN warnings. DebugCounter does not
+    // well-defined semantics for multi-threaded programs.
+    std::atomic<int64_t> Count{0};
+    std::atomic<int64_t> Skip{0};
+    std::atomic<int64_t> StopAfter{-1};
+    std::atomic<bool> IsSet{false};
+    #else
     int64_t Count = 0;
     int64_t Skip = 0;
     int64_t StopAfter = -1;
     bool IsSet = false;
+    #endif // LLVM_ENABLE_THREADS
     std::string Desc;
   };
   DenseMap<unsigned, CounterInfo> Counters;
   CounterVector RegisteredCounters;
 };
 
 #define DEBUG_COUNTER(VARNAME, COUNTERNAME, DESC)                              \
   static const unsigned VARNAME =                                              \
       DebugCounter::registerCounter(COUNTERNAME, DESC)
 
 } // namespace llvm
 #endif

unittests/Support/DebugCounterTest.cpp

 //===- llvm/unittest/Support/DebugCounterTest.cpp -------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/DebugCounter.h"
 #include "gtest/gtest.h"
 
 #include <string>
+
+#if LLVM_ENABLE_THREADS
+#include <thread>
+#include <mutex>
+#include <condition_variable>
+#endif
+
 using namespace llvm;
 
 #ifndef NDEBUG
 DEBUG_COUNTER(TestCounter, "test-counter",
               "Counter used for unit test");
 
 TEST(DebugCounterTest, CounterCheck) {
   EXPECT_FALSE(DebugCounter::isCounterSet(TestCounter));
TEST(DebugCounterTest, CounterCheck) {
 
   DebugCounter::setCounterValue(TestCounter, 3);
   EXPECT_TRUE(DebugCounter::shouldExecute(TestCounter));
   EXPECT_FALSE(DebugCounter::shouldExecute(TestCounter));
 
   DebugCounter::setCounterValue(TestCounter, 100);
   EXPECT_FALSE(DebugCounter::shouldExecute(TestCounter));
 }
-#endif
+
+#if LLVM_ENABLE_THREADS
+TEST(DebugCounterTest, MultipleThreads) {
+  constexpr int ThreadCount = 3;
+  constexpr int Iterations = 1000;
+
+  std::mutex Mut;
+  std::condition_variable Cond;
+  int ThreadsWaiting = 0;
+  bool StartRunning = false;
+
+  auto Run = [&]() {
+    {
+      std::unique_lock<std::mutex> Lock(Mut);
+      ++ThreadsWaiting;
+      Cond.notify_all();
+      Cond.wait(Lock, [&]() { return StartRunning; });
+    }
+    for (int I = 0; I < Iterations; ++I)
+      DebugCounter::shouldExecute(TestCounter);
+  };
+
+  // This test is here to make sure TSAN does not show warnings about data races.
+  // DebugCounter should not be used in multi-threaded environments.
+  std::vector<std::thread> Threads;
+  for (int I = 0; I < ThreadCount; ++I) {
+    Threads.emplace_back(Run);
+  }
+
+  {
+    std::unique_lock<std::mutex> Lock(Mut);
+    // Wait for the threads to start.
+    Cond.wait(Lock, [&]() { return ThreadsWaiting == ThreadCount; });
+    StartRunning = true;
+    // Indicate they can start running.
+    Cond.notify_all();
+  }
+
+  for (auto & T : Threads)
+    T.join();
+}
+#endif // LLVM_ENABLE_THREADS
+#endif // NDEBUG