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

[Reid Kleckner via llvm-commits]

rnk added a comment.

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.


================
Comment at: lib/Support/Windows/Signals.inc:488
@@ -472,1 +487,3 @@
 
+#ifdef _MSC_VER
+
----------------
Are there issues with enabling this under mingw?

================
Comment at: lib/Support/Windows/Signals.inc:579
@@ +578,3 @@
+  // dumps in a specified location.
+  const SmallString<128> LocalDumpsRegistryLocation(
+      "SOFTWARE\\Microsoft\\Windows\\Windows Error Reporting\\LocalDumps");
----------------
This can just be a StringRef instead of a SmallString. No need to copy it.

================
Comment at: lib/Support/Windows/Signals.inc:584
@@ +583,3 @@
+  // dump settings.  This will be NULL if the location can not be found.
+  HKEY DefaultLocalDumpsKey =
+      FindRegistryKey(Twine(LocalDumpsRegistryLocation));
----------------
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.

================
Comment at: lib/Support/Windows/Signals.inc:585
@@ +584,3 @@
+  HKEY DefaultLocalDumpsKey =
+      FindRegistryKey(Twine(LocalDumpsRegistryLocation));
+
----------------
This Twine construction should be unnecessary, it should happen implicitly.

================
Comment at: lib/Support/Windows/Signals.inc:591
@@ +590,3 @@
+  HKEY AppSpecificKey =
+      FindRegistryKey(Twine(LocalDumpsRegistryLocation + "\\" + ProgramName));
+
----------------
I think if you want operator+ to do the right thing here, you want to write this as:
  FindRegistryKey(Twine(LocalDumpsRegistryLocation) + '\\' + ProgramName);

================
Comment at: lib/Support/Windows/Signals.inc:599
@@ +598,3 @@
+  if (!GetDumpType(AppSpecificKey, DumpType))
+    if (!GetDumpType(DefaultLocalDumpsKey, DumpType)) DumpType = MiniDumpNormal;
+
----------------
LLVM doesn't normally format one-line ifs this way.


http://reviews.llvm.org/D14022