Require users to opt into DebugCounting

[george.burgess.iv]

Summary

r337748 made it so that DebugCounters are always incremented in asserting builds, so that we can better track how many times certain passes are run. This change broke some multithreaded tsan builds (D50080), since we increment these global counters in an unsynchronized manner.

We only need these counter values if we're either using a DebugCounter to limit a pass, or if we plan to print them out (D50031). Having an enable bit to track that seems like the best way forward.

I don't have a test, since the global state at play here makes unittesting pretty awkward, and we have some existing test coverage here. Happy to try to make something if people want that, though.

Whatever the fix here ends up being, I intend to try to get it into the 7.0 branch.

[davide]

LGTM, thanks George. I'm not particularly worried if it's too hard to write a test for this.

[george.burgess.iv]

Thanks!

include/llvm/Support/DebugCounter.h

public:
   //
   // FIXME: Currently, counter registration is required to happen before command
   // line option parsing. The main reason to register counters is to produce a
   // nice list of them on the command line, but i'm not sure this is worth it.
   static unsigned registerCounter(StringRef Name, StringRef Desc) {
     return instance().addCounter(Name, Desc);
   }
   inline static bool shouldExecute(unsigned CounterName) {
-// Compile to nothing when debugging is off
-#ifdef NDEBUG
+    if (!isCountingEnabled())
       return true;
-#else
+
     auto &Us = instance();
     auto Result = Us.Counters.find(CounterName);
     if (Result != Us.Counters.end()) {
       auto &CounterInfo = Result->second;
       ++CounterInfo.Count;
 
       // We only execute while the Skip is not smaller than Count,
       // and the StopAfter + Skip is larger than Count.
       // Negative counters always execute.
       if (CounterInfo.Skip < 0)
         return true;
       if (CounterInfo.Skip >= CounterInfo.Count)
         return false;
       if (CounterInfo.StopAfter < 0)
         return true;
       return CounterInfo.StopAfter + CounterInfo.Skip >= CounterInfo.Count;
     }
     // Didn't find the counter, should we warn?
     return true;
-#endif // NDEBUG
   }
 
   // Return true if a given counter had values set (either programatically or on
   // the command line).  This will return true even if those values are
   // currently in a state where the counter will always execute.
   static bool isCounterSet(unsigned ID) {
     return instance().Counters[ID].IsSet;
   }
public:
 
   // Iterate through the registered counters
   typedef UniqueVector<std::string> CounterVector;
   CounterVector::const_iterator begin() const {
     return RegisteredCounters.begin();
   }
   CounterVector::const_iterator end() const { return RegisteredCounters.end(); }
 
+  // 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
+  }
+
   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 {
     int64_t Count = 0;
     int64_t Skip = 0;
     int64_t StopAfter = -1;
     bool IsSet = false;
     std::string Desc;
   };
   DenseMap<unsigned, CounterInfo> Counters;
   CounterVector RegisteredCounters;
+
+  // Whether we should do DebugCounting at all. DebugCounters aren't
+  // thread-safe, so this should always be false in multithreaded scenarios.
+  bool Enabled = false;
 };
 
 #define DEBUG_COUNTER(VARNAME, COUNTERNAME, DESC)                              \
   static const unsigned VARNAME =                                              \
       DebugCounter::registerCounter(COUNTERNAME, DESC)
 
 } // namespace llvm
 #endif

lib/Support/DebugCounter.cpp

void DebugCounter::push_back(const std::string &Val) {
   if (CounterPair.first.endswith("-skip")) {
     auto CounterName = CounterPair.first.drop_back(5);
     unsigned CounterID = getCounterId(CounterName);
     if (!CounterID) {
       errs() << "DebugCounter Error: " << CounterName
              << " is not a registered counter\n";
       return;
     }
+    enableAllCounters();
     Counters[CounterID].Skip = CounterVal;
     Counters[CounterID].IsSet = true;
   } else if (CounterPair.first.endswith("-count")) {
     auto CounterName = CounterPair.first.drop_back(6);
     unsigned CounterID = getCounterId(CounterName);
     if (!CounterID) {
       errs() << "DebugCounter Error: " << CounterName
              << " is not a registered counter\n";
       return;
     }
+    enableAllCounters();
     Counters[CounterID].StopAfter = CounterVal;
     Counters[CounterID].IsSet = true;
   } else {
     errs() << "DebugCounter Error: " << CounterPair.first
            << " does not end with -skip or -count\n";
   }
 }