[Support] Fix Windows dump file hang with multi-threaded crashes

[andrewng]

Summary

Prevents deadlock between MiniDumpWriteDump and
CryptAcquireContextW (called via fs::createTemporaryFile) in
WriteWindowsDumpFile.

However, there's no guarantee that deadlock can't still occur between
MiniDumpWriteDump and some other Win32 API call. But that would appear
to be the "accepted" risk of using MiniDumpWriteDump in this manner.

[rnk]

The change seems reasonable, but I hesitate to add the test as is.

llvm/unittests/Support/WindowsDumpFileTest.cpp

41 ↗

(On Diff #441722)

I appreciate the effort and thoroughness that went into writing this test, but I don't think we should commit it as is.

If the critical section is removed, this test will nondeterministically fail with a timeout due to the deadlock. That's not something we can use to bisect problems or debug why things went wrong. I don't think it's worth spending resources to run this test if it can't produce actionable results from a buildbot.

[andrewng]

llvm/unittests/Support/WindowsDumpFileTest.cpp

41 ↗

(On Diff #441722)

I'm not sure that there's a "better" way to test this issue. Do you have any suggestions? Or are you suggesting to just discard the testing completely? Which I think could be acceptable in this case. I added testing because that's usually a requirement for any change to be accepted.

[rnk]

llvm/unittests/Support/WindowsDumpFileTest.cpp

41 ↗

(On Diff #441722)

Yes, I think it's fine to land this code change without a test. It doesn't seem practical in this case.

[andrewng]

Address review comment and removed the unit test.

[rnk]

lgtm, thanks!

llvm/lib/Support/Windows/Signals.inc

 
 /// Write a Windows dump file containing process information that can be
 /// used for post-mortem debugging.
 ///
 /// \returns zero error code if a mini dump created, actual error code
 /// otherwise.
 static std::error_code WINAPI
 WriteWindowsDumpFile(PMINIDUMP_EXCEPTION_INFORMATION ExceptionInfo) {
+  struct ScopedCriticalSection {
+    ScopedCriticalSection() { EnterCriticalSection(&CriticalSection); }
+    ~ScopedCriticalSection() { LeaveCriticalSection(&CriticalSection); }
+  } SCS;
+
   using namespace llvm;
   using namespace llvm::sys;
 
   std::string MainExecutableName = fs::getMainExecutable(nullptr, nullptr);
   StringRef ProgramName;
 
   if (MainExecutableName.empty()) {
     // If we can't get the executable filename,