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

[Aaron Ballman via llvm-commits]

aaron.ballman added inline comments.

================
Comment at: include/llvm/Support/Process.h:47
@@ +46,3 @@
+  /// \brief true if PreventCoreFiles has been called, false otherwise.
+  static bool CoreFilesPrevented;
+
----------------
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.

================
Comment at: lib/Support/Windows/Signals.inc:496
@@ +495,3 @@
+                            DWORD AccessRights = KEY_QUERY_VALUE | KEY_READ |
+                                                 KEY_WOW64_64KEY) {
+  HKEY Key = NULL;
----------------
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.

================
Comment at: lib/Support/Windows/Signals.inc:509
@@ +508,3 @@
+static bool GetDumpFolder(HKEY Key,
+                          llvm::SmallVectorImpl<char> &ResultDirectory) {
+  if (!Key) return false;
----------------
I'm concerned that this API will fail on systems with non-ASCII characters in the resulting path.

================
Comment at: lib/Support/Windows/Signals.inc:510
@@ +509,3 @@
+                          llvm::SmallVectorImpl<char> &ResultDirectory) {
+  if (!Key) return false;
+
----------------
Formatting

================
Comment at: lib/Support/Windows/Signals.inc:536
@@ +535,3 @@
+static bool GetDumpType(HKEY Key, MINIDUMP_TYPE &ResultType) {
+  if (!Key) return false;
+
----------------
Formatting

================
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));
----------------
rnk wrote:
> 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.
I would prefer we clean up properly -- don't we have an RAII helper class that does this for Win32 API types already?

================
Comment at: lib/Support/Windows/Signals.inc:616
@@ +615,3 @@
+  if (ExplicitDumpDirectorySet) {
+    if (std::error_code EC = fs::create_directories(Twine(DumpDirectory)))
+      return EC;
----------------
Should be able to ditch the explicit creation of Twine.

================
Comment at: lib/Support/Windows/Signals.inc:620
@@ +619,3 @@
+    if (std::error_code EC = fs::createUniqueFile(
+            Twine(DumpDirectory + "\\" + ProgramName + ".%%%%%%.dmp"), FD,
+            DumpPath))
----------------
This will have the same problem that rnk commented on above.

================
Comment at: lib/Support/Windows/Signals.inc:631
@@ +630,3 @@
+  // Actually write the contents of the dump file using our open handle.
+  if (!FileHandle ||
+      !fMiniDumpWriteDump(GetCurrentProcess(), GetCurrentProcessId(),
----------------
This needs to check for INVALID_HANDLE_VALUE, not nullptr.

================
Comment at: lib/Support/Windows/Signals.inc:634
@@ +633,3 @@
+                          FileHandle, DumpType, ExceptionInfo, NULL, NULL))
+    return std::error_code(mapWindowsError(GetLastError()));
+
----------------
Failing to close the handle in this case. I would recommend the RAII helper class (ScopedHandle, IIRC).


http://reviews.llvm.org/D14022