Add automatic Windows Minidump support for tools crashes

[gbedwell]

Summary

Hi,

This patch adds the ability for LLVM-based tools on Windows to automatically generate MiniDump .dmp files in the case of tool crashes which can then be used for post-mortem debugging within Visual Studio. The default Windows behaviour in applications is typically to allow the creation of a .dmp file on a crash by popping up a modal dialog box. As this is hardly ideal for a command line tool that's likely to be running unattended or even on a headless remote machine we've had this disabled since r227470. With this patch a .dmp file is now generated automatically when LLVMUnhandledExceptionFilter is executed in the event of a crash, just prior to the existing code for printing a stacktrace.

We've found this very useful in the past to give us clues in the case of intermittent crashes that can't be replicated locally in a debugging environment (or at all) or cases where our users may be unable or unwilling to provide us with a repro example. Obviously for this to be most useful it needs to be used with a build configuration such as Debug or RelWithDebInfo, but the latter works quite nicely for shipping MSVC builds as the debugging information gets stored in PDB files which don't need to be shipped, but just archived for such a time that you may need to analyze a .dmp file.

The core of this patch is the call to MiniDumpWriteDump ( https://msdn.microsoft.com/en-us/library/windows/desktop/ms680360(v=vs.85).aspx ) inside new function WriteWindowsDumpFile. I've also implemented support for reading registry keys from "SOFTWARE\Microsoft\Windows\Windows Error Reporting\LocalDumps" ( https://msdn.microsoft.com/en-us/library/windows/desktop/bb787181(v=vs.85).aspx ) to allow custom MiniDump options and location to be specified (for example, in cases where a reporting tool may be automatically monitoring a specific location for any crash dumps).

Now, for example, if I force a clang crash using a debug pragma (without having set up the above registry keys) I will now see a message like the following prior to the normal stack trace output:

Wrote crash dump file "C:\Users\GREG~1\AppData\Local\Temp\clang.exe-91d481.dmp"

There are a few things that I'm not entirely happy with that I'd like to get some opinions on, but hopefully the attached patch can serve as a starting point for discussion:

• PreventCoreFiles(): Obviously there are plenty of cases where we want to disable automatic minidump creation. This is the purpose of the existing Process::PreventCoreFiles(). Unfortunately, the existing cases covered by that function deal with calling various OS APIs, whereas as for this approach it needs to control a later decision we make in the crash handler so we need to carry some information around about whether it's been called or not, hence the introduction of bool Process::CoreFilesPrevented. Is this reasonable, or is there a better way to convey that information to the point at which it is needed?

• "LLVM_DISABLE_CRASH_REPORT": I'm assuming that we don't want crashing lit tests to start filling the disk up with crash dump files every time someone introduces a crash bug during normal development. I noticed that lit is setting this environment variable which is currently only used in some APPLE guarded code in Unix/Signals.inc with the comment "Environment variable to disable any kind of crash dialog". I can see that it used to also be checked in Windows/Signals.inc but was removed as we unconditionally prevent any crash dialogs now. Right now in this patch I've got the Windows implementation calling PreventCoreFiles() in the presence of the environment variable. Is my assumption that we don't want crash dumps for lit tests correct? If so, is this the way to do it?

• Testing: Unfortunately I'm at a loss for an effective way to do any useful form of unit or regression testing for this type of change. Internally I think I can test this in our private tests by creating a mock environment with fake Windows registry keys, etc. and using clang's crash pragmas to ensure that files get written to the expected location (at least in the case of clang) but I can't think of an effective way that is going to be contributable to the community. What's the policy for tests in the deep OS-entangled code in the Support libs?

Many Thanks,

Greg Bedwell
SN Systems Ltd. - Sony Computer Entertainment Group

[rnk]

For testing, what do you think is most worth testing here? I think testing the registry reading logic is probably the hardest to do since it requires making local modifications to the registry. If we give up on testing that, we could add some environment variable (LLVM_CRASH_DUMP_DIR) that bypasses all that logic and use it to test the integration from the crash handler through to creating an actual file on disk.

You can look at unittests/Support/ProgramTest.cpp for examples of other subprocess tests in LLVM. I think our crash handler might have bad interactions with gtest, though, since it uses SEH __try to attempt to report crashes as test failures. That'll probably block our minidumping. Alternatively, you could add something similar to clang's crash recovery tests. We have pragmas that explicitly force clang to do stuff like dereference null, and then you'd verify that we get a dump file.

lib/Support/Windows/Signals.inc

488

Are there issues with enabling this under mingw?

579

This can just be a StringRef instead of a SmallString. No need to copy it.

584

You never bother calling RegCloseKey on this HKEY. That's fine, this is a crash handler, we don't need to worry, but I would like to see a comment to that effect.

585

This Twine construction should be unnecessary, it should happen implicitly.

591

I think if you want operator+ to do the right thing here, you want to write this as:

FindRegistryKey(Twine(LocalDumpsRegistryLocation) + '\\' + ProgramName);

599

LLVM doesn't normally format one-line ifs this way.

[aaron.ballman]

include/llvm/Support/Process.h

47

Do we have to worry about multithreaded access to this? I'm not certain how we handle other situations where we're adding what are effectively globals. Perhaps documenting that this is not thread-safe is sufficient.

I would feel a bit more comfortable if this wasn't exposed as a public member, since it's set to true by PreventCoreFiles, but can also be set to true outside of that API.

lib/Support/Windows/Signals.inc

496

Why do we want to access a 64-bit key from a 32-bit app by default? That seems like surprising behavior for a generic-sounding helper function.

Since you always use the defaults anyway, perhaps it makes more sense to leave them out and hard-code them. Then you can name the function something like FindWERKey or something.

509

I'm concerned that this API will fail on systems with non-ASCII characters in the resulting path.

510

Formatting

536

Formatting

584

I would prefer we clean up properly -- don't we have an RAII helper class that does this for Win32 API types already?

616

Should be able to ditch the explicit creation of Twine.

620

This will have the same problem that rnk commented on above.

631

This needs to check for INVALID_HANDLE_VALUE, not nullptr.

634

Failing to close the handle in this case. I would recommend the RAII helper class (ScopedHandle, IIRC).

[gbedwell]

Thanks both for your feedback! I'll start tackling it all once my jetlag from developer meeting travel subsides :).

[gbedwell]

Apologies for the (long) delay here. I'm looking at this again now and I'll post an update soon.

[gbedwell]

Hi,

Just a quick update. I'm continuing to work on this. So far I've addressed the majority of comments locally, but as has sadly been the continuing story of the last few months, other commitments cropping up are meaning that I've had much less time than I'd hoped to actually just sit down in front of an editor and work on LLVM. Currently I'm in the depths of path handling trying to reconcile the half of the existing code that seems to assume char, with the rest that deals with wchar_t paths. I'm going to be out of the office next week, so if there is a pressing need to move forward on this or D18216 I'm happy for that to happen in the meantime and I'll expand on that with a subsequent patch to fill in anything missing such as the Windows registry handling side here, otherwise I'll resume this as soon as I'm back at my desk!

Thanks
-Greg

[lenykholodov]

Hi Greg,

Thank you for the update. There is no time pressure. However, if you don't mind, I can update D18216 with ideas from D14022 and then post it for review.

[lenykholodov]

Hi guys,

I have updated D18216 patch with ideas from D14022. I would be highly appreciated if you have time to review it.

[gbedwell]

Superseded by D18216.

include/llvm/Support/Process.h

 
 namespace sys {
 
 
 /// \brief A collection of legacy interfaces for querying information about the
 /// current executing process.
 class Process {
 public:
+  /// \brief true if PreventCoreFiles has been called, false otherwise.
+  static bool CoreFilesPrevented;
+
   static unsigned getPageSize();
 
   /// \brief Return process memory usage.
   /// This static function will return the total amount of memory allocated
   /// by the process. This only counts the memory allocated via the malloc,
   /// calloc and realloc functions and includes any "free" holes in the
   /// allocated space.
   static size_t GetMallocUsage();

lib/Support/Process.cpp

#define ALLCOLORS(FGBG,BOLD) {\
     COLOR(FGBG, "7", BOLD)\
   }
 
 static const char colorcodes[2][2][8][10] = {
  { ALLCOLORS("3",""), ALLCOLORS("3","1;") },
  { ALLCOLORS("4",""), ALLCOLORS("4","1;") }
 };
 
+// This is set to true when Process::PreventCoreFiles() is called.
+bool Process::CoreFilesPrevented = false;
+
 // Include the platform-specific parts of this class.
 #ifdef LLVM_ON_UNIX
 #include "Unix/Process.inc"
 #endif
 #ifdef LLVM_ON_WIN32
 #include "Windows/Process.inc"
 #endif

lib/Support/Unix/Process.inc

#if defined(HAVE_MACH_MACH_H) && !defined(__GNU__)
 
   // Disable crash reporting on Mac OS X 10.5
   signal(SIGABRT, _exit);
   signal(SIGILL,  _exit);
   signal(SIGFPE,  _exit);
   signal(SIGSEGV, _exit);
   signal(SIGBUS,  _exit);
 #endif
+
+  Process::CoreFilesPrevented = true;
 }
 
 Optional<std::string> Process::GetEnv(StringRef Name) {
   std::string NameStr = Name.str();
   const char *Val = ::getenv(NameStr.c_str());
   if (!Val)
     return None;
   return std::string(Val);

lib/Support/Windows/Process.inc

void Process::PreventCoreFiles() {
   // WerAddExcludedApplication and WerRemoveExcludedApplication (Vista and
   // later).
   //
   // Windows also has modal pop-up message boxes.  As this method is used by
   // bugpoint, preventing these pop-ups is additionally important.
   SetErrorMode(SEM_FAILCRITICALERRORS |
                SEM_NOGPFAULTERRORBOX |
                SEM_NOOPENFILEERRORBOX);
+
+  Process::CoreFilesPrevented = true;
 }
 
 /// Returns the environment variable \arg Name's value as a string encoded in
 /// UTF-8. \arg Name is assumed to be in UTF-8 encoding.
 Optional<std::string> Process::GetEnv(StringRef Name) {
   // Convert the argument to UTF-16 to pass it to _wgetenv().
   SmallVector<wchar_t, 128> NameUTF16;
   if (windows::UTF8ToUTF16(Name, NameUTF16))

lib/Support/Windows/Signals.inc

 //===- Win32/Signals.cpp - Win32 Signals Implementation ---------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file provides the Win32 specific implementation of the Signals class.
 //
 //===----------------------------------------------------------------------===//
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Process.h"
+#include "llvm/Support/WindowsError.h"
 #include <algorithm>
+#include <io.h>
 #include <signal.h>
 #include <stdio.h>
 
 #include "llvm/Support/Format.h"
 #include "llvm/Support/raw_ostream.h"
 
 // The Windows.h header must be after LLVM and standard headers.
 #include "WindowsSupport.h"
typedef PVOID (__stdcall *PFUNCTION_TABLE_ACCESS_ROUTINE64)( HANDLE ahProcess,
                       DWORD64 AddrBase);
 
 typedef DWORD64 (__stdcall *PGET_MODULE_BASE_ROUTINE64)(HANDLE hProcess,
                       DWORD64 Address);
 
 typedef DWORD64 (__stdcall *PTRANSLATE_ADDRESS_ROUTINE64)(HANDLE hProcess,
                       HANDLE hThread, LPADDRESS64 lpaddr);
 
+typedef BOOL(WINAPI *fpMiniDumpWriteDump)(HANDLE, DWORD, HANDLE, MINIDUMP_TYPE,
+                                          PMINIDUMP_EXCEPTION_INFORMATION,
+                                          PMINIDUMP_USER_STREAM_INFORMATION,
+                                          PMINIDUMP_CALLBACK_INFORMATION);
+static fpMiniDumpWriteDump fMiniDumpWriteDump;
+
 typedef BOOL (WINAPI *fpStackWalk64)(DWORD, HANDLE, HANDLE, LPSTACKFRAME64,
                       PVOID, PREAD_PROCESS_MEMORY_ROUTINE64,
                       PFUNCTION_TABLE_ACCESS_ROUTINE64,
                       PGET_MODULE_BASE_ROUTINE64,
                       PTRANSLATE_ADDRESS_ROUTINE64);
 static fpStackWalk64 fStackWalk64;
 
 typedef DWORD64 (WINAPI *fpSymGetModuleBase64)(HANDLE, DWORD64);

 static fpSymSetOptions fSymSetOptions;
 
 typedef BOOL (WINAPI *fpSymInitialize)(HANDLE, PCSTR, BOOL);
 static fpSymInitialize fSymInitialize;
 
 static bool load64BitDebugHelp(void) {
   HMODULE hLib = ::LoadLibraryW(L"Dbghelp.dll");
   if (hLib) {
+    fMiniDumpWriteDump = (fpMiniDumpWriteDump)
+                      ::GetProcAddress(hLib, "MiniDumpWriteDump");
     fStackWalk64 = (fpStackWalk64)
                       ::GetProcAddress(hLib, "StackWalk64");
     fSymGetModuleBase64 = (fpSymGetModuleBase64)
                       ::GetProcAddress(hLib, "SymGetModuleBase64");
     fSymGetSymFromAddr64 = (fpSymGetSymFromAddr64)
                       ::GetProcAddress(hLib, "SymGetSymFromAddr64");
     fSymGetLineFromAddr64 = (fpSymGetLineFromAddr64)
                       ::GetProcAddress(hLib, "SymGetLineFromAddr64");
#endif
   SetErrorMode(SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX |
                SEM_NOOPENFILEERRORBOX);
   _set_error_mode(_OUT_TO_STDERR);
 }
 
 /// PrintStackTraceOnErrorSignal - When an error signal (such as SIBABRT or
 /// SIGSEGV) is delivered to the process, print a stack trace and then exit.
 void sys::PrintStackTraceOnErrorSignal(bool DisableCrashReporting) {
+  if (DisableCrashReporting || getenv("LLVM_DISABLE_CRASH_REPORT"))
+    Process::PreventCoreFiles();
+
   DisableSystemDialogsOnCrash();
   RegisterHandler();
   LeaveCriticalSection(&CriticalSection);
 }
 }
 
 #if defined(__MINGW32__) && !defined(__MINGW64_VERSION_MAJOR)
 // Provide a prototype for RtlCaptureContext, mingw32 from mingw.org is
void llvm::sys::RunInterruptHandlers() {
   // The interrupt handler may be called from an interrupt, but it may also be
   // called manually (such as the case of report_fatal_error with no registered
   // error handler). We must ensure that the critical section is properly
   // initialized.
   InitializeThreading();
   Cleanup();
 }
 
+#ifdef _MSC_VER
+
+/// \brief Find the Windows Registry Key for a given location.
+///
+/// \returns a valid HKEY if the location exists, else NULL.
+static HKEY FindRegistryKey(const llvm::Twine &RegistryLocation,
+                            HKEY RegistryRoot = HKEY_LOCAL_MACHINE,
+                            DWORD AccessRights = KEY_QUERY_VALUE | KEY_READ |
+                                                 KEY_WOW64_64KEY) {
+  HKEY Key = NULL;
+  if (RegOpenKeyEx(RegistryRoot, RegistryLocation.str().c_str(), 0,
+                   AccessRights, &Key) != ERROR_SUCCESS)
+    return NULL;
+  return Key;
+}
+
+/// \brief Populate ResultDirectory with the value for "DumpFolder" for a given
+/// Windows Registry key.
+///
+/// \returns true if a valid value for DumpFolder exists, false otherwise.
+static bool GetDumpFolder(HKEY Key,
+                          llvm::SmallVectorImpl<char> &ResultDirectory) {
+  if (!Key) return false;
+
+  const std::size_t BufferLength = MAX_PATH + 1;
+  char Buffer[BufferLength];
+  DWORD BufferLengthBytes = BufferLength;
+
+  LONG Error = RegGetValue(Key, 0, "DumpFolder", RRF_RT_REG_SZ, NULL, &Buffer,
+                           &BufferLengthBytes);
+  ResultDirectory = llvm::SmallString<128>(Buffer);
+  return (Error == ERROR_SUCCESS);
+}
+
+/// \brief Populate ResultType with a valid MINIDUMP_TYPE based on the value of
+/// "DumpType" for a given Windows Registry key.
+///
+/// According to
+/// https://msdn.microsoft.com/en-us/library/windows/desktop/bb787181(v=vs.85).aspx
+/// valid values for DumpType are:
+///   * 0: Custom dump
+///   * 1: Mini dump
+///   * 2: Full dump
+/// If "Custom dump" is specified then the "CustomDumpFlags" field is read
+/// containing a bitwise combination of MINIDUMP_TYPE values.
+///
+/// \returns true if a valid value for ResultType can be set, false otherwise.
+static bool GetDumpType(HKEY Key, MINIDUMP_TYPE &ResultType) {
+  if (!Key) return false;
+
+  DWORD DumpType = 1;
+  DWORD TypeSize = sizeof(DumpType);
+  if (RegGetValue(Key, NULL, "DumpType", RRF_RT_REG_DWORD, NULL, &DumpType,
+                  &TypeSize) != ERROR_SUCCESS)
+    return false;
+
+  switch (DumpType) {
+    case 0: {
+      DWORD Flags = 0;
+      if (RegGetValue(Key, NULL, "CustomDumpFlags", RRF_RT_REG_DWORD, NULL,
+                      &Flags, &TypeSize) != ERROR_SUCCESS)
+        return false;
+
+      ResultType = static_cast<MINIDUMP_TYPE>(MiniDumpValidTypeFlags & Flags);
+      break;
+    }
+    case 1:
+      ResultType = MiniDumpNormal;
+      break;
+    case 2:
+      ResultType = MiniDumpWithFullMemory;
+      break;
+    default:
+      return false;
+  }
+  return true;
+}
+
+/// \brief Write a Windows dump file containing process information that can be
+/// used for post-mortem debugging.
+static std::error_code WINAPI
+WriteWindowsDumpFile(PMINIDUMP_EXCEPTION_INFORMATION ExceptionInfo) {
+  using namespace llvm;
+  using namespace llvm::sys;
+
+  StringRef ProgramName = path::filename(__argv[0]);
+
+  // The Windows Registry location as specified at
+  // https://msdn.microsoft.com/en-us/library/windows/desktop/bb787181%28v=vs.85%29.aspx
+  // "Collecting User-Mode Dumps" that may optionally be set to collect crash
+  // dumps in a specified location.
+  const SmallString<128> LocalDumpsRegistryLocation(
+      "SOFTWARE\\Microsoft\\Windows\\Windows Error Reporting\\LocalDumps");
+
+  // The key pointing to the Registry location that may contain global crash
+  // dump settings.  This will be NULL if the location can not be found.
+  HKEY DefaultLocalDumpsKey =
+      FindRegistryKey(Twine(LocalDumpsRegistryLocation));
+
+  // The key pointing to the Registry location that may contain
+  // application-specific crash dump settings.  This will be NULL if the
+  // location can not be found.
+  HKEY AppSpecificKey =
+      FindRegistryKey(Twine(LocalDumpsRegistryLocation + "\\" + ProgramName));
+
+  // Look to see if a dump type is specified in the registry; first with the
+  // app-specific key and failing that with the global key.  If none are found
+  // default to a normal dump (GetDumpType will return false either if the key
+  // is NULL or if there is no valid DumpType value at its location).
+  MINIDUMP_TYPE DumpType;
+  if (!GetDumpType(AppSpecificKey, DumpType))
+    if (!GetDumpType(DefaultLocalDumpsKey, DumpType)) DumpType = MiniDumpNormal;
+
+  // Look to see if a dump location is specified in the registry; first with the
+  // app-specific key and failing that with the global key.  If none are found
+  // we'll just create the dump file in the default temporary file location
+  // (GetDumpFolder will return false either if the key is NULL or if there is
+  // no valid DumpFolder value at its location).
+  bool ExplicitDumpDirectorySet = true;
+  SmallString<128> DumpDirectory;
+  if (!GetDumpFolder(AppSpecificKey, DumpDirectory))
+    if (!GetDumpFolder(DefaultLocalDumpsKey, DumpDirectory))
+      ExplicitDumpDirectorySet = false;
+
+  int FD;
+  SmallString<128> DumpPath;
+
+  if (ExplicitDumpDirectorySet) {
+    if (std::error_code EC = fs::create_directories(Twine(DumpDirectory)))
+      return EC;
+
+    if (std::error_code EC = fs::createUniqueFile(
+            Twine(DumpDirectory + "\\" + ProgramName + ".%%%%%%.dmp"), FD,
+            DumpPath))
+      return EC;
+  } else if (std::error_code EC =
+                 fs::createTemporaryFile(ProgramName, "dmp", FD, DumpPath))
+    return EC;
+
+  // Our support functions return a file descriptor but Windows wants a handle.
+  HANDLE FileHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+
+  // Actually write the contents of the dump file using our open handle.
+  if (!FileHandle ||
+      !fMiniDumpWriteDump(GetCurrentProcess(), GetCurrentProcessId(),
+                          FileHandle, DumpType, ExceptionInfo, NULL, NULL))
+    return std::error_code(mapWindowsError(GetLastError()));
+
+  if (!CloseHandle(FileHandle))
+    return std::error_code(mapWindowsError(GetLastError()));
+
+  llvm::errs() << "Wrote crash dump file \"" << DumpPath << "\"\n";
+  return std::error_code();
+}
+#endif
+
 static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
   Cleanup();
 
+#ifdef _MSC_VER
+  // With MSVC we'll automatically write a Minidump file here to help diagnose
+  // the nasty sorts of crashes that aren't 100% reproducible from a set of
+  // inputs (or in the event that the user is unable or unwilling to provide a
+  // reproducible case).
+  if (!llvm::Process::CoreFilesPrevented) {
+    MINIDUMP_EXCEPTION_INFORMATION ExceptionInfo;
+    ExceptionInfo.ThreadId = GetCurrentThreadId();
+    ExceptionInfo.ExceptionPointers = ep;
+    ExceptionInfo.ClientPointers = FALSE;
+
+    if (std::error_code EC = WriteWindowsDumpFile(&ExceptionInfo))
+      llvm::errs() << "Could not write crash dump file: " << EC.message()
+                   << "\n";
+  }
+#endif
+
   // Initialize the STACKFRAME structure.
   STACKFRAME64 StackFrame = {};
 
 #if defined(_M_X64)
   StackFrame.AddrPC.Offset = ep->ContextRecord->Rip;
   StackFrame.AddrPC.Mode = AddrModeFlat;
   StackFrame.AddrStack.Offset = ep->ContextRecord->Rsp;
   StackFrame.AddrStack.Mode = AddrModeFlat;