[PATCH] D14022: Add automatic Windows Minidump support for tools crashes

[Greg Bedwell via llvm-commits]

gbedwell created this revision.
gbedwell added reviewers: rnk, aaron.ballman, Bigcheese.
gbedwell added a subscriber: llvm-commits.

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

http://reviews.llvm.org/D14022

Files:
  include/llvm/Support/Process.h
  lib/Support/Process.cpp
  lib/Support/Unix/Process.inc
  lib/Support/Windows/Process.inc
  lib/Support/Windows/Signals.inc

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D14022.38242.patch
Type: text/x-patch
Size: 10806 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151023/1c00d4f4/attachment.bin>