<div dir="ltr">Hi!<div><br></div><div>I'd like to get this merged into the 7.0 branch. It fixes data races, which were introduced by r337748, in asserting builds of LLVM.</div><div><br></div><div>I've included Chandler as owner of Support/.</div><div><br></div><div>Alternatively, we can revert r337748 on 7.0, since it's not all that useful on its own. :) I'm equally happy with either approach.</div><div><br></div><div>Thank you!</div><div>George</div></div><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Aug 2, 2018 at 12:50 PM George Burgess IV via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: gbiv<br>
Date: Thu Aug 2 12:50:27 2018<br>
New Revision: 338762<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=338762&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=338762&view=rev</a><br>
Log:<br>
[Support] Add an enable bit to our DebugCounters<br>
<br>
r337748 made us start incrementing DebugCounters all of the time. This<br>
makes tsan unhappy in multithreaded environments.<br>
<br>
Since it doesn't make much sense to use DebugCounters with multiple<br>
threads, this patch makes us only count anything if the user passed a<br>
-debug-counter option or if some other piece of code explicitly asks<br>
for it (e.g. the pass in D50031).<br>
<br>
The amount of global state here makes writing a unittest for this<br>
behavior somewhat awkward. So, no test is provided.<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D50150" rel="noreferrer" target="_blank">https://reviews.llvm.org/D50150</a><br>
<br>
Modified:<br>
llvm/trunk/include/llvm/Support/DebugCounter.h<br>
llvm/trunk/lib/Support/DebugCounter.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/Support/DebugCounter.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/DebugCounter.h?rev=338762&r1=338761&r2=338762&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/DebugCounter.h?rev=338762&r1=338761&r2=338762&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Support/DebugCounter.h (original)<br>
+++ llvm/trunk/include/llvm/Support/DebugCounter.h Thu Aug 2 12:50:27 2018<br>
@@ -70,10 +70,9 @@ public:<br>
return instance().addCounter(Name, Desc);<br>
}<br>
inline static bool shouldExecute(unsigned CounterName) {<br>
-// Compile to nothing when debugging is off<br>
-#ifdef NDEBUG<br>
- return true;<br>
-#else<br>
+ if (!isCountingEnabled())<br>
+ return true;<br>
+<br>
auto &Us = instance();<br>
auto Result = Us.Counters.find(CounterName);<br>
if (Result != Us.Counters.end()) {<br>
@@ -93,7 +92,6 @@ public:<br>
}<br>
// Didn't find the counter, should we warn?<br>
return true;<br>
-#endif // NDEBUG<br>
}<br>
<br>
// Return true if a given counter had values set (either programatically or on<br>
@@ -142,7 +140,23 @@ public:<br>
}<br>
CounterVector::const_iterator end() const { return RegisteredCounters.end(); }<br>
<br>
+ // Force-enables counting all DebugCounters.<br>
+ //<br>
+ // Since DebugCounters are incompatible with threading (not only do they not<br>
+ // make sense, but we'll also see data races), this should only be used in<br>
+ // contexts where we're certain we won't spawn threads.<br>
+ static void enableAllCounters() { instance().Enabled = true; }<br>
+<br>
private:<br>
+ static bool isCountingEnabled() {<br>
+// Compile to nothing when debugging is off<br>
+#ifdef NDEBUG<br>
+ return false;<br>
+#else<br>
+ return instance().Enabled;<br>
+#endif<br>
+ }<br>
+<br>
unsigned addCounter(const std::string &Name, const std::string &Desc) {<br>
unsigned Result = RegisteredCounters.insert(Name);<br>
Counters[Result] = {};<br>
@@ -159,6 +173,10 @@ private:<br>
};<br>
DenseMap<unsigned, CounterInfo> Counters;<br>
CounterVector RegisteredCounters;<br>
+<br>
+ // Whether we should do DebugCounting at all. DebugCounters aren't<br>
+ // thread-safe, so this should always be false in multithreaded scenarios.<br>
+ bool Enabled = false;<br>
};<br>
<br>
#define DEBUG_COUNTER(VARNAME, COUNTERNAME, DESC) \<br>
<br>
Modified: llvm/trunk/lib/Support/DebugCounter.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/DebugCounter.cpp?rev=338762&r1=338761&r2=338762&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/DebugCounter.cpp?rev=338762&r1=338761&r2=338762&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Support/DebugCounter.cpp (original)<br>
+++ llvm/trunk/lib/Support/DebugCounter.cpp Thu Aug 2 12:50:27 2018<br>
@@ -82,6 +82,7 @@ void DebugCounter::push_back(const std::<br>
<< " is not a registered counter\n";<br>
return;<br>
}<br>
+ enableAllCounters();<br>
Counters[CounterID].Skip = CounterVal;<br>
Counters[CounterID].IsSet = true;<br>
} else if (CounterPair.first.endswith("-count")) {<br>
@@ -92,6 +93,7 @@ void DebugCounter::push_back(const std::<br>
<< " is not a registered counter\n";<br>
return;<br>
}<br>
+ enableAllCounters();<br>
Counters[CounterID].StopAfter = CounterVal;<br>
Counters[CounterID].IsSet = true;<br>
} else {<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>