[LLD] Avoid exiting with a locked mutex NFC

[andrewng]

Summary

In ErrorHandler::error(), rearrange code to avoid calling exitLld with
the mutex locked. Acquire mutex lock when flushing the output streams in
exitLld.

[MaskRay]

lld/Common/ErrorHandler.cpp

66

it does not seem that exitLld can be invoked concurrently.

[andrewng]

It is generally not good practice to hold a lock whilst exiting, as this has the potential to cause deadlocks. I don't believe that this situation can occur currently with LLD. However, after some recent observations, it appears that on Windows, in particular when using the static run-time, the only guaranteed way to not intermittently crash on exit is to wait for all threads to terminate (see related review D70447). Doing so would enable this deadlock situation to occur.

lld/Common/ErrorHandler.cpp

66

Yes, indeed that is another potential problem that exists and I believe that multiple threads calling _exit is also undefined behaviour.

However, this change is really to protect against flushing the streams whilst some other thread could be writing to them.

lld/Common/ErrorHandler.cpp

if (errorHandler().outputBuffer)
     errorHandler().outputBuffer->discard();
 
   // Dealloc/destroy ManagedStatic variables before calling _exit().
   // In an LTO build, allows us to get the output of -time-passes.
   // Ensures that the thread pool for the parallel algorithms is stopped to
   // avoid intermittent crashes on Windows when exiting.
   llvm_shutdown();
 
+  {
+    std::lock_guard<std::mutex> lock(mu);
     lld::outs().flush();
     lld::errs().flush();
+  }
   _exit(val);
 }
 
 void lld::diagnosticHandler(const DiagnosticInfo &di) {
   SmallString<128> s;
   raw_svector_ostream os(s);
   DiagnosticPrinterRawOStream dp(os);
   di.print(dp);
if (vsDiagnostics) {
 
     if (std::regex_match(str, m, re)) {
       error(m.str(1) + m.str(2));
       error(m.str(1) + m.str(3));
       return;
     }
   }
 
+  bool exit = false;
+  {
     std::lock_guard<std::mutex> lock(mu);
 
     if (errorLimit == 0 || errorCount < errorLimit) {
       lld::errs() << sep << getLocation(msg) << ": " << Colors::RED
                   << "error: " << Colors::RESET << msg << "\n";
     } else if (errorCount == errorLimit) {
       lld::errs() << sep << getLocation(msg) << ": " << Colors::RED
-                << "error: " << Colors::RESET << errorLimitExceededMsg << "\n";
-    if (exitEarly)
-      exitLld(1);
+                  << "error: " << Colors::RESET << errorLimitExceededMsg
+                  << "\n";
+      exit = exitEarly;
     }
 
     sep = getSeparator(msg);
     ++errorCount;
   }
 
+  if (exit)
+    exitLld(1);
+}
+
 void ErrorHandler::fatal(const Twine &msg) {
   error(msg);
   exitLld(1);
 }