[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

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

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

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,

llvm/unittests/Support/CMakeLists.txt

add_llvm_unittest(SupportTests
   ToolOutputFileTest.cpp
   TypeNameTest.cpp
   TypeTraitsTest.cpp
   TrailingObjectsTest.cpp
   TrigramIndexTest.cpp
   UnicodeTest.cpp
   VersionTupleTest.cpp
   VirtualFileSystemTest.cpp
+  WindowsDumpFileTest.cpp
   WithColorTest.cpp
   YAMLIOTest.cpp
   YAMLParserTest.cpp
   buffer_ostream_test.cpp
   formatted_raw_ostream_test.cpp
   raw_fd_stream_test.cpp
   raw_ostream_test.cpp
   raw_pwrite_stream_test.cpp

llvm/unittests/Support/WindowsDumpFileTest.cpp

+//===- llvm/unittests/Support/WindowsDumpFileTest.cpp ---------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/Signals.h"
+
+#include "llvm/Config/config.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Process.h"
+#include "llvm/Support/Program.h"
+#include "llvm/Support/Threading.h"
+
+#include "gtest/gtest.h"
+
+#include <atomic>
+#include <future>
+
+#if defined(_WIN32) && LLVM_ENABLE_THREADS && LLVM_ENABLE_CRASH_DUMPS
+
+using namespace llvm;
+
+// See llvm/utils/unittest/UnitTestMain/TestMain.cpp.
+extern const char *TestMainArgv0;
+
+// Just a reachable symbol to ease resolving of the executable's path.
+static cl::opt<std::string>
+    WindowsDumpFileTestStringArg1("windows-dump-file-test-string-arg1");
+
+TEST(WindowsDumpFileTest, MultiThread) {
+  unsigned NumThreads = hardware_concurrency().compute_thread_count();
+  if (NumThreads == 1)
+    GTEST_SKIP();
+
+  // Use at least 4 threads.
+  NumThreads = std::max(NumThreads, 4U);
+
+  if (getenv("LLVM_WINDOWS_DUMP_FILE_TEST_MULTI_THREAD")) {
+    ASSERT_FALSE(sys::Process::AreCoreFilesPrevented());
+    sys::PrintStackTraceOnErrorSignal(StringRef(), false);
+    ASSERT_FALSE(sys::Process::AreCoreFilesPrevented());
+
+    std::vector<std::thread> Threads;
+    std::atomic<unsigned> ThreadsStarted(0);
+    std::promise<void> Started;
+    std::mutex Mutex;
+    std::condition_variable Cond;
+    std::atomic<bool> Ready(false);
+    for (unsigned I = 0; I < NumThreads; ++I)
+      Threads.emplace_back([&, NumThreads] {
+        if (++ThreadsStarted == NumThreads)
+          Started.set_value();
+
+        std::unique_lock<std::mutex> Lock(Mutex);
+        Cond.wait(Lock, [&] { return Ready.load(); });
+        Lock.unlock();
+
+        LLVM_BUILTIN_TRAP;
+      });
+
+    Started.get_future().wait();
+
+    {
+      std::unique_lock<std::mutex> Lock(Mutex);
+      Ready = true;
+    }
+    Cond.notify_all();
+
+    for (auto &T : Threads)
+      T.join();
+
+    LLVM_BUILTIN_TRAP;
+  }
+
+  // Create a temporary directory to be used as the temporary directory for the
+  // test run.
+  SmallString<128> TmpDir;
+  std::error_code EC =
+      sys::fs::createUniqueDirectory("WindowsDumpFileTest", TmpDir);
+  ASSERT_FALSE(EC);
+
+  std::string Executable =
+      sys::fs::getMainExecutable(TestMainArgv0, &WindowsDumpFileTestStringArg1);
+  StringRef Argv[] = {Executable,
+                      "--gtest_filter=WindowsDumpFileTest.MultiThread"};
+  std::vector<StringRef> Env;
+  Env.emplace_back("LLVM_WINDOWS_DUMP_FILE_TEST_MULTI_THREAD=1");
+
+  // Set temporary directory environment variables.
+  std::string TmpEnv = ("TMP=" + TmpDir).str();
+  Env.emplace_back(TmpEnv);
+  std::string TempEnv = ("TEMP=" + TmpDir).str();
+  Env.emplace_back(TempEnv);
+
+  // The following is required to prevent disabling of dumps.
+  Env.emplace_back("LLVM_PROGRAM_TEST_NO_STACKTRACE_HANDLER=1");
+
+  unsigned TimeoutInSecs = NumThreads;
+  std::string Error;
+  int Ret = sys::ExecuteAndWait(Executable, Argv, ArrayRef<StringRef>(Env), {},
+                                TimeoutInSecs, 0, &Error);
+  ASSERT_NE(Ret, 0);
+  ASSERT_TRUE(Error.empty());
+
+  // Remove temporary directory.
+  sys::fs::remove_directories(TmpDir);
+}
+
+#endif