[Support] Creation of minidump after compiler crash on Windows

[lenykholodov]

Summary

In the current implementation compiler only prints stack trace to console after crash. This patch adds saving of minidump files which contain a useful subset of the information for further debugging.

[aaron.ballman]

You may also want to take a look at D14022, which was implementing this same sort of idea (though it appears to have stalled a bit).

[lenykholodov]

In D18216#377216, @aaron.ballman wrote:

You may also want to take a look at D14022, which was implementing this same sort of idea (though it appears to have stalled a bit).

This patch is much simpler than D14022. It was prepared as small and reliable as possible.

[aaron.ballman]

In D18216#377410, @lenykholodov wrote:

In D18216#377216, @aaron.ballman wrote:

You may also want to take a look at D14022, which was implementing this same sort of idea (though it appears to have stalled a bit).

This patch is much simpler than D14022. It was prepared as small and reliable as possible.

The point I was getting at is that we came up with a design in D14022 that we liked and there has been a fair amount of review done on already. This patch would directly conflict with that one, and so we should determine if the original author is intending to continue their work or not, and if not, what ideas from the original patch we may want to incorporate here.

[gbedwell]

Really sorry for stalling on D14022. I've been sidetracked on some high-priority off-list work for a few months that took me off this although I've been acutely aware that having a stalled patch sitting in review for so long is bad form, so I've been intending to get back to this very very soon. Leny, I'm happy to work with you to make sure we have a solution that satisfies both our use-cases, as it's definitely an incredibly useful feature to have. I'll take a look at this patch in depth as soon as I'm back from EuroLLVM next week.

[lenykholodov]

In D18216#377672, @gbedwell wrote:

Really sorry for stalling on D14022. I've been sidetracked on some high-priority off-list work for a few months that took me off this although I've been acutely aware that having a stalled patch sitting in review for so long is bad form, so I've been intending to get back to this very very soon. Leny, I'm happy to work with you to make sure we have a solution that satisfies both our use-cases, as it's definitely an incredibly useful feature to have. I'll take a look at this patch in depth as soon as I'm back from EuroLLVM next week.

Ok. I'll prepare unit test for crash reports and will wait for update for your patch or feedback for mine.

[lenykholodov]

The updated patch is based on D14022 and includes fixes for comments from reviewers.

[gbedwell]

Thanks for working on this!

This passes all of my local testing for our use cases for the change proposed in D14022. From that point of view, I'm very happy to resolve that review in favour of this patch. I don't feel comfortable giving LGTMs in this area of the compiler just yet, but from my point of view I'm happy.

Thanks again.

[lenykholodov]

Thank you, Greg! @aaron.ballman and @rnk, could you please review updated patch?

[aaron.ballman]

include/llvm/Support/Process.h

73

Perhaps: AreCoreFilesPrevented() to be a bit more grammatically correct?

lib/Support/Windows/Signals.inc

124

Are we still supporting 32-bit MinGW so that we need to keep doing this dance? :-(

585

No need to initialize this.

611

This doesn't qualify as a "small" vector; might as well just use std::vector.

622

Same here.

652

No need to initialize this.

696

I don't think this is reasonable to assume since the crash dump functionality could be called from any executable in the LLVM project. I would say that if we can't get the executable filename, things are in worse shape than we realize and we should just bail out.

lib/Support/Windows/WindowsSupport.h

175

s/0/NULL

[lenykholodov]

Fixes according to the review. Tested llvm support building with MinGW-W64-builds-4.2.0

[lenykholodov]

Hi Aaron,

Thank you for review. I have updated patch according to your comments. Could you please take a look?

include/llvm/Support/Process.h

73

Fixed

lib/Support/Windows/Signals.inc

124

I have checked llvm support building with mingw and have not found any issues. Could you please clarify what is wrong with this code?

585

Fixed

611

This was done similar to using of SmallVector in Path.inc file. I can change type to std::vector. However, this will be inconsistent with other code in Support library.

652

Fixed

696

Fixed

lib/Support/Windows/WindowsSupport.h

175

Fixed

[aaron.ballman]

Thank you for working on this, the patch LGTM!

lib/Support/Windows/Signals.inc

124

The code is fine, I just wish we didn't have to dynamically load everything and redeclare things from public SDKs. I should have been more clear, sorry about that! There's nothing for you to change here, more of a note that we should see whether we still need to continue following this pattern for MinGW.

611

Consistency is reasonable enough, the SmallVector can handle being not small and this isn't on a hot code path, so I think it's fine as-is.

[lenykholodov]

lib/Support/Windows/Signals.inc

124

Thank you for clarification

include/llvm/Support/Process.h

   /// @brief Prevent core file generation.
   static void PreventCoreFiles();
 
+  /// \brief true if PreventCoreFiles has been called, false otherwise.
+  static bool AreCoreFilesPrevented();
+
   // This function returns the environment variable \arg name's value as a UTF-8
   // string. \arg Name is assumed to be in UTF-8 encoding too.
   static Optional<std::string> GetEnv(StringRef name);

lib/Support/Process.cpp

  { ALLCOLORS("4",""), ALLCOLORS("4","1;") }
 };
 
+// This is set to true when Process::PreventCoreFiles() is called.
+static bool coreFilesPrevented = false;
+
+bool Process::AreCoreFilesPrevented() {
+  return coreFilesPrevented;
+}
+
 // Include the platform-specific parts of this class.
 #ifdef LLVM_ON_UNIX
 #include "Unix/Process.inc"

lib/Support/Unix/Process.inc

   signal(SIGSEGV, _exit);
   signal(SIGBUS,  _exit);
 #endif
+
+  coreFilesPrevented = true;
 }
 
 Optional<std::string> Process::GetEnv(StringRef Name) {

lib/Support/Windows/Process.inc

   SetErrorMode(SEM_FAILCRITICALERRORS |
                SEM_NOGPFAULTERRORBOX |
                SEM_NOOPENFILEERRORBOX);
+
+  coreFilesPrevented = true;
 }
 
 /// Returns the environment variable \arg Name's value as a string encoded in

lib/Support/Windows/Signals.inc

 //
 //===----------------------------------------------------------------------===//
 #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>
 

 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,

 static bool load64BitDebugHelp(void) {
   HMODULE hLib = ::LoadLibraryW(L"Dbghelp.dll");
   if (hLib) {
+    fMiniDumpWriteDump = (fpMiniDumpWriteDump)
+                      ::GetProcAddress(hLib, "MiniDumpWriteDump");
     fStackWalk64 = (fpStackWalk64)
                       ::GetProcAddress(hLib, "StackWalk64");
     fSymGetModuleBase64 = (fpSymGetModuleBase64)

     fEnumerateLoadedModules = (fpEnumerateLoadedModules)
       ::GetProcAddress(hLib, "EnumerateLoadedModules64");
   }
-  return fStackWalk64 && fSymInitialize && fSymSetOptions;
+  return fStackWalk64 && fSymInitialize && fSymSetOptions && fMiniDumpWriteDump;
 }
 
 using namespace llvm;

 /// 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);

   Cleanup();
 }
 
+/// \brief Find the Windows Registry Key for a given location.
+///
+/// \returns a valid HKEY if the location exists, else NULL.
+static HKEY FindWERKey(const llvm::Twine &RegistryLocation) {
+  HKEY Key;
+  if (ERROR_SUCCESS != ::RegOpenKeyEx(HKEY_LOCAL_MACHINE,
+                                      RegistryLocation.str().c_str(), 0,
+                                      KEY_QUERY_VALUE | KEY_READ, &Key))
+    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) {
+  using llvm::sys::windows::UTF16ToUTF8;
+
+  if (!Key)
+    return false;
+
+  DWORD BufferLengthBytes = 0;
+
+  if (ERROR_SUCCESS != ::RegGetValueW(Key, 0, L"DumpFolder", REG_EXPAND_SZ,
+                                      NULL, NULL, &BufferLengthBytes))
+    return false;
+
+  SmallVector<wchar_t, MAX_PATH> Buffer(BufferLengthBytes);
+
+  if (ERROR_SUCCESS != ::RegGetValueW(Key, 0, L"DumpFolder", REG_EXPAND_SZ,
+                                      NULL, Buffer.data(), &BufferLengthBytes))
+    return false;
+
+  DWORD ExpandBufferSize = ::ExpandEnvironmentStringsW(Buffer.data(), NULL, 0);
+
+  if (!ExpandBufferSize)
+    return false;
+
+  SmallVector<wchar_t, MAX_PATH> ExpandBuffer(ExpandBufferSize);
+
+  if (ExpandBufferSize != ::ExpandEnvironmentStringsW(Buffer.data(),
+                                                      ExpandBuffer.data(),
+                                                      ExpandBufferSize))
+    return false;
+
+  if (UTF16ToUTF8(ExpandBuffer.data(), ExpandBufferSize - 1, ResultDirectory))
+    return false;
+
+  return true;
+}
+
+/// \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;
+  DWORD TypeSize = sizeof(DumpType);
+  if (ERROR_SUCCESS != ::RegGetValueW(Key, NULL, L"DumpType", RRF_RT_REG_DWORD,
+                                      NULL, &DumpType,
+                                      &TypeSize))
+    return false;
+
+  switch (DumpType) {
+  case 0: {
+    DWORD Flags = 0;
+    if (ERROR_SUCCESS != ::RegGetValueW(Key, NULL, L"CustomDumpFlags",
+                                        RRF_RT_REG_DWORD, NULL, &Flags,
+                                        &TypeSize))
+      return false;
+
+    ResultType = static_cast<MINIDUMP_TYPE>(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.
+///
+/// \returns zero error code if a mini dump created, actual error code
+/// otherwise.
+static std::error_code WINAPI
+WriteWindowsDumpFile(PMINIDUMP_EXCEPTION_INFORMATION ExceptionInfo) {
+  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,
+    // things are in worse shape than we realize
+    // and we should just bail out.
+    return mapWindowsError(::GetLastError());
+  }
+
+  ProgramName = path::filename(MainExecutableName.c_str());
+
+  // 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.
+  StringRef 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.
+  ScopedRegHandle DefaultLocalDumpsKey(FindWERKey(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.
+  ScopedRegHandle AppSpecificKey(
+      FindWERKey(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<MAX_PATH> DumpDirectory;
+  if (!GetDumpFolder(AppSpecificKey, DumpDirectory))
+    if (!GetDumpFolder(DefaultLocalDumpsKey, DumpDirectory))
+      ExplicitDumpDirectorySet = false;
+
+  int FD;
+  SmallString<MAX_PATH> DumpPath;
+
+  if (ExplicitDumpDirectorySet) {
+    if (std::error_code EC = fs::create_directories(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.
+  ScopedCommonHandle FileHandle(reinterpret_cast<HANDLE>(_get_osfhandle(FD)));
+
+  if (!fMiniDumpWriteDump(::GetCurrentProcess(), ::GetCurrentProcessId(),
+                          FileHandle, DumpType, ExceptionInfo, NULL, NULL))
+    return mapWindowsError(::GetLastError());
+
+  llvm::errs() << "Wrote crash dump file \"" << DumpPath << "\"\n";
+  return std::error_code();
+}
+
 static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
   Cleanup();
 
+  // 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::AreCoreFilesPrevented()) {
+    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";
+  }
+
   // Initialize the STACKFRAME structure.
   STACKFRAME64 StackFrame = {};
 

lib/Support/Windows/WindowsSupport.h

   }
 };
 
+struct RegTraits : CommonHandleTraits {
+  typedef HKEY handle_type;
+
+  static handle_type GetInvalid() {
+    return NULL;
+  }
+
+  static void Close(handle_type h) {
+    ::RegCloseKey(h);
+  }
+
+  static bool IsValid(handle_type h) {
+    return h != GetInvalid();
+  }
+};
+
 struct FindHandleTraits : CommonHandleTraits {
   static void Close(handle_type h) {
     ::FindClose(h);

 typedef ScopedHandle<CommonHandleTraits> ScopedCommonHandle;
 typedef ScopedHandle<FileHandleTraits>   ScopedFileHandle;
 typedef ScopedHandle<CryptContextTraits> ScopedCryptContext;
+typedef ScopedHandle<RegTraits>          ScopedRegHandle;
 typedef ScopedHandle<FindHandleTraits>   ScopedFindHandle;
 typedef ScopedHandle<JobHandleTraits>    ScopedJobHandle;