[PATCH] D50150: Require users to opt into DebugCounting

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv created this revision.
george.burgess.iv added reviewers: zhizhouy, davide.

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 (https://reviews.llvm.org/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 (https://reviews.llvm.org/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.


Repository:
  rL LLVM

https://reviews.llvm.org/D50150

Files:
  include/llvm/Support/DebugCounter.h
  lib/Support/DebugCounter.cpp


Index: lib/Support/DebugCounter.cpp
===================================================================
--- lib/Support/DebugCounter.cpp
+++ lib/Support/DebugCounter.cpp
@@ -82,6 +82,7 @@
              << " is not a registered counter\n";
       return;
     }
+    enableAllCounters();
     Counters[CounterID].Skip = CounterVal;
     Counters[CounterID].IsSet = true;
   } else if (CounterPair.first.endswith("-count")) {
@@ -92,6 +93,7 @@
              << " is not a registered counter\n";
       return;
     }
+    enableAllCounters();
     Counters[CounterID].StopAfter = CounterVal;
     Counters[CounterID].IsSet = true;
   } else {
Index: include/llvm/Support/DebugCounter.h
===================================================================
--- include/llvm/Support/DebugCounter.h
+++ include/llvm/Support/DebugCounter.h
@@ -70,10 +70,9 @@
     return instance().addCounter(Name, Desc);
   }
   inline static bool shouldExecute(unsigned CounterName) {
-// Compile to nothing when debugging is off
-#ifdef NDEBUG
-    return true;
-#else
+    if (!isCountingEnabled())
+      return true;
+
     auto &Us = instance();
     auto Result = Us.Counters.find(CounterName);
     if (Result != Us.Counters.end()) {
@@ -93,7 +92,6 @@
     }
     // 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
@@ -142,7 +140,23 @@
   }
   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] = {};
@@ -159,6 +173,10 @@
   };
   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)                              \


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D50150.158566.patch
Type: text/x-patch
Size: 2578 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180801/f43508dc/attachment.bin>