[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 ↗

(On Diff #51978)

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 ↗

(On Diff #51978)

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 ↗

(On Diff #55604)

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 ↗

(On Diff #55604)

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

lib/Support/Windows/Signals.inc

 typedef BOOL (WINAPI *fpEnumerateLoadedModules)(HANDLE,PENUMLOADED_MODULES_CALLBACK64,PVOID);
 static fpEnumerateLoadedModules fEnumerateLoadedModules;
 
+typedef BOOL (WINAPI * fpMiniDumpWriteDump)(HANDLE,DWORD,HANDLE,MINIDUMP_TYPE,
+  CONST PMINIDUMP_EXCEPTION_INFORMATION,
+  CONST PMINIDUMP_USER_STREAM_INFORMATION,
+  CONST PMINIDUMP_CALLBACK_INFORMATION);
+static fpMiniDumpWriteDump fMiniDumpWriteDump;
+
 static bool load64BitDebugHelp(void) {
   HMODULE hLib = ::LoadLibraryW(L"Dbghelp.dll");
   if (hLib) {

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

   }
 }
 
+static void WINAPI CreateMiniDump(llvm::raw_ostream &OS,
+                                  _EXCEPTION_POINTERS *pExceptionInfo) {
+  char buffer[_MAX_PATH];
+
+  DWORD length =
+      ::GetModuleFileName(::GetModuleHandle(0), buffer, sizeof(buffer));
+
+  // If GetModuleHandle fails, we would get the current process executable file
+  // name, which is fine.
+
+  if (length >= sizeof(buffer))
+    strcpy(buffer, "clang.exe");
+
+  char appName[_MAX_PATH];
+
+  _splitpath(buffer, NULL, NULL, appName, NULL);
+
+  DWORD tempPathLength = ::GetTempPath(sizeof(buffer), buffer);
+
+  // If GetTempPath fails, we would write minidump to the current directory.
+
+  if (tempPathLength >= sizeof(buffer))
+    tempPathLength = 0;
+
+  size_t maxPrefixLength = sizeof(buffer) - tempPathLength;
+
+  int prefixLength =
+      _snprintf(buffer + tempPathLength, maxPrefixLength, "%s", appName);
+
+  // If _snprintf fails, we would use 'minidump' as the prefix
+  // and write file to the current directory.
+
+  if (prefixLength <= 0 || prefixLength >= int(maxPrefixLength)) {
+    strcpy(buffer, "minidump");
+    prefixLength = strlen(buffer);
+  } else {
+    prefixLength += tempPathLength;
+  }
+
+  SYSTEMTIME systemTime;
+
+  ::GetSystemTime(&systemTime);
+
+  int maxSuffixLength = sizeof(buffer) - prefixLength;
+
+  int suffixLength = _snprintf(
+      buffer + prefixLength, maxSuffixLength,
+      ".%04d%02d%02d%02d%02d%02d%03d.dmp", systemTime.wYear, systemTime.wMonth,
+      systemTime.wDay, systemTime.wHour, systemTime.wMinute, systemTime.wSecond,
+      systemTime.wMilliseconds);
+
+  if (suffixLength <= 0 || suffixLength >= maxSuffixLength) {
+    _snprintf(buffer, sizeof(buffer),
+              "minidump.%04d%02d%02d%02d%02d%02d%03d.dmp", systemTime.wYear,
+              systemTime.wMonth, systemTime.wDay, systemTime.wHour,
+              systemTime.wMinute, systemTime.wSecond, systemTime.wMilliseconds);
+  }
+
+  HANDLE hFile = ::CreateFile(buffer, GENERIC_WRITE, FILE_SHARE_WRITE, 0,
+                              CREATE_NEW, FILE_ATTRIBUTE_NORMAL, 0);
+
+  bool writeMiniDumpSucceeded = false;
+  int errorCode = GetLastError();
+
+  if (hFile != INVALID_HANDLE_VALUE) {
+    _MINIDUMP_EXCEPTION_INFORMATION ExInfo;
+
+    ExInfo.ThreadId = ::GetCurrentThreadId();
+    ExInfo.ExceptionPointers = pExceptionInfo;
+    ExInfo.ClientPointers = NULL;
+
+    writeMiniDumpSucceeded = fMiniDumpWriteDump(
+        ::GetCurrentProcess(), ::GetCurrentProcessId(), hFile,
+        (MINIDUMP_TYPE)(MiniDumpNormal | MiniDumpWithDataSegs), &ExInfo, 0, 0);
+
+    errorCode = GetLastError();
+
+    ::CloseHandle(hFile);
+  }
+
+  if (writeMiniDumpSucceeded)
+    OS << "Minidump successfully saved: " << buffer << '\n';
+  else
+    OS << "Cannot create minidump file: " << buffer << " (error code "
+       << errorCode << ")\n";
+}
+
 namespace llvm {
 
 //===----------------------------------------------------------------------===//

 
   HANDLE hProcess = GetCurrentProcess();
   HANDLE hThread = GetCurrentThread();
-  PrintStackTraceForThread(llvm::errs(), hProcess, hThread, StackFrame,
+  llvm::raw_ostream &OS = llvm::errs();
+
+  PrintStackTraceForThread(OS, hProcess, hThread, StackFrame,
                            ep->ContextRecord);
 
+  CreateMiniDump(OS, ep);
+
   _exit(ep->ExceptionRecord->ExceptionCode);
 }