Correct default TimerGroup singleton to use magic-statics so that it gets cleaned up

[erichkeane]

Summary

When running Valgrind, I discovered that the string in the default TimerGroup (created in getDefaultTimerGroup) gets 'possibly lost', it is never deleted in the current implementation. The TimerGroup object allocates a string, but it is stored as a pointer in getDefaultTimerGroup(), so it is never cleaned up.

This patch uses a magic-static to maintain the thread-safe behavior, however stores the value on the stack so that it gets cleaned up afterwards.

A few things:
1- The mutex that was previously created will be taken inside the DefaultTimerGroup's constructor, so there shouldn't be any concurrency problems that didn't previously exist.
2- I realize that this fix doesn't terribly matter too much, since it is cleaning stuff up right before program termination, but I thought it is a pretty minor cost for code correctness and removing the error from valgrind (reducing noise!)

[erichkeane]

Is there a better person to review this? I believe that this is a vast improvement over the existing implementation, and should be a pretty easy to review change. Is there someone better to review this?

[aaron.ballman]

Are you sure that magic statics are thread-safe on all compilers we support building with?

[erichkeane]

I don't know. Is there an official list somewhere? MSVC2015, GCC4.3.

[aaron.ballman]

In D28367#678937, @erichkeane wrote:

I don't know. Is there an official list somewhere? MSVC2015, GCC4.3.

The public docs look reasonable:

http://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library

[erichkeane]

Ah, I'm apparently bad at googling! According to that list, the answer is 'yes':

We use: Clang 3.1 supported since 2.9
We use: GCC 4.8 Supported since 4.3
We use: Visual Studio 2015 (Update 3), Supported since 2015

[aaron.ballman]

Then I think this code is okay. The downside is, if it turns out there's a subtle bug, it is going to be really hard to track down.

[MatzeB]

Last time I wanted to add more static constructors/destructors I was yelled at in the review...

llvm/trunk/lib/Support/Timer.cpp

std::unique_ptr<raw_fd_ostream> llvm::CreateInfoOutputFile() {
   if (!EC)
     return Result;
 
   errs() << "Error opening info-output-file '"
     << OutputFilename << " for appending!\n";
   return llvm::make_unique<raw_fd_ostream>(2, false); // stderr.
 }
 
-
-static TimerGroup *DefaultTimerGroup = nullptr;
 static TimerGroup *getDefaultTimerGroup() {
-  TimerGroup *tmp = DefaultTimerGroup;
-  sys::MemoryFence();
-  if (tmp) return tmp;
-
-  sys::SmartScopedLock<true> Lock(*TimerLock);
-  tmp = DefaultTimerGroup;
-  if (!tmp) {
-    tmp = new TimerGroup("misc", "Miscellaneous Ungrouped Timers");
-    sys::MemoryFence();
-    DefaultTimerGroup = tmp;
-  }
-
-  return tmp;
+  static TimerGroup DefaultTimerGroup("misc", "Miscellaneous Ungrouped Timers");
+  return &DefaultTimerGroup;
 }
 
 //===----------------------------------------------------------------------===//
 // Timer Implementation
 //===----------------------------------------------------------------------===//
 
 void Timer::init(StringRef Name, StringRef Description) {
   init(Name, Description, *getDefaultTimerGroup());
void TimerGroup::PrintQueuedTimers(raw_ostream &OS) {
   unsigned Padding = (80-Description.length())/2;
   if (Padding > 80) Padding = 0;         // Don't allow "negative" numbers
   OS.indent(Padding) << Description << '\n';
   OS << "===" << std::string(73, '-') << "===\n";
 
   // If this is not an collection of ungrouped times, print the total time.
   // Ungrouped timers don't really make sense to add up.  We still print the
   // TOTAL line to make the percentages make sense.
-  if (this != DefaultTimerGroup)
+  if (this != getDefaultTimerGroup())
     OS << format("  Total Execution Time: %5.4f seconds (%5.4f wall clock)\n",
                  Total.getProcessTime(), Total.getWallTime());
   OS << '\n';
 
   if (Total.getUserTime())
     OS << "   ---User Time---";
   if (Total.getSystemTime())
     OS << "   --System Time--";