[llvm] r338762 - [Support] Add an enable bit to our DebugCounters

[George Burgess IV via llvm-commits]

Hi!

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.

I've included Chandler as owner of Support/.

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.

Thank you!
George


On Thu, Aug 2, 2018 at 12:50 PM George Burgess IV via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Author: gbiv
> Date: Thu Aug  2 12:50:27 2018
> New Revision: 338762
>
> URL: http://llvm.org/viewvc/llvm-project?rev=338762&view=rev
> Log:
> [Support] Add an enable bit to our DebugCounters
>
> r337748 made us start incrementing DebugCounters all of the time. This
> makes tsan unhappy in multithreaded environments.
>
> Since it doesn't make much sense to use DebugCounters with multiple
> threads, this patch makes us only count anything if the user passed a
> -debug-counter option or if some other piece of code explicitly asks
> for it (e.g. the pass in D50031).
>
> The amount of global state here makes writing a unittest for this
> behavior somewhat awkward. So, no test is provided.
>
> Differential Revision: https://reviews.llvm.org/D50150
>
> Modified:
>     llvm/trunk/include/llvm/Support/DebugCounter.h
>     llvm/trunk/lib/Support/DebugCounter.cpp
>
> Modified: llvm/trunk/include/llvm/Support/DebugCounter.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/DebugCounter.h?rev=338762&r1=338761&r2=338762&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/DebugCounter.h (original)
> +++ llvm/trunk/include/llvm/Support/DebugCounter.h Thu Aug  2 12:50:27 2018
> @@ -70,10 +70,9 @@ public:
>      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 @@ public:
>      }
>      // 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 @@ public:
>    }
>    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 @@ private:
>    };
>    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)
>       \
>
> Modified: llvm/trunk/lib/Support/DebugCounter.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/DebugCounter.cpp?rev=338762&r1=338761&r2=338762&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Support/DebugCounter.cpp (original)
> +++ llvm/trunk/lib/Support/DebugCounter.cpp Thu Aug  2 12:50:27 2018
> @@ -82,6 +82,7 @@ void DebugCounter::push_back(const std::
>               << " 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 @@ void DebugCounter::push_back(const std::
>               << " is not a registered counter\n";
>        return;
>      }
> +    enableAllCounters();
>      Counters[CounterID].StopAfter = CounterVal;
>      Counters[CounterID].IsSet = true;
>    } else {
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180802/40916cae/attachment.html>