[PATCH] D12884: [Windows] Symbolize with llvm-symbolizer instead of dbghelp in a self-host

[Reid Kleckner via llvm-commits]

rnk added inline comments.

================
Comment at: lib/Support/Signals.cpp:99-100
@@ +98,4 @@
+  sys::fs::createTemporaryFile("symbolizer-output", "", OutputFile);
+  FileRemover InputRemover(InputFile.c_str());
+  FileRemover OutputRemover(OutputFile.c_str());
+
----------------
zturner wrote:
> Does `createTemporaryFile` not open the file in such a way that it has `FILE_FLAG_DELETE_ON_CLOSE` or the equivalent on unix platforms?
Apparently not. =/

================
Comment at: lib/Support/Signals.cpp:138
@@ +137,3 @@
+    if (!Modules[i]) {
+      OS << format("#%d " FMT_PTR "\n", frame_no++, StackTrace[i]);
+      continue;
----------------
zturner wrote:
> `os << format_hex()` already behaves consistently.  It's not a big deal, but it seems better to use type-safe formatting whenever possible, and I think `format_hex` would work just fine in that regard here.
Cool, I switched to that.

================
Comment at: lib/Support/Windows/Signals.inc:282-283
@@ -196,4 +281,4 @@
   // Initialize the symbol handler.
   fSymSetOptions(SYMOPT_DEFERRED_LOADS | SYMOPT_LOAD_LINES);
   fSymInitialize(hProcess, NULL, TRUE);
 
----------------
zturner wrote:
> Seems like this code could move after the `#ifdef`, no need to initialize anything if we end up not using Dbghelp anyway
StackWalk64 is part of dbghelp, and we need to initialize it first.

================
Comment at: lib/Support/Windows/Signals.inc:289-292
@@ +288,6 @@
+#ifdef __clang__
+  if (printStackTraceWithLLVMSymbolizer(OS, hProcess, hThread, StackFrame,
+                                        Context)) {
+    return;
+  }
+#else
----------------
zturner wrote:
> Why only when self-hosting?  Wouldn't it be worth it to try llvm-symbolizer no matter what?
Yeah, I'll get rid of that. My idea was to avoid changing behavior for a standard Windows build, but we might as well do it.


http://reviews.llvm.org/D12884