[PATCH] [RFC] Use llvm-symbolizer to symbolize LLVM/Clang crash dumps

[David Blaikie]

Some random thoughts. Nothing terribly important.

================
Comment at: lib/Support/Unix/Signals.inc:284
@@ +283,3 @@
+  DlIteratePhdrData *data = (DlIteratePhdrData*)arg;
+  const char *name = (data->first) ? data->main_exec_name : info->dlpi_name;
+  data->first = false;
----------------
parens around data->first seem unnecessary

================
Comment at: lib/Support/Unix/Signals.inc:287
@@ +286,3 @@
+  for (int i = 0; i < info->dlpi_phnum; i++) {
+    const ElfW(Phdr) *phdr = &info->dlpi_phdr[i];
+    if (phdr->p_type == PT_LOAD) {
----------------
Well that's some syntax I haven't seen before... I assume it's valid/compiles?

I would've expected to see that written as:

  const EelfW (*phdr)(Phdr) = ...

but if what you've got there works... great... just surprising.

================
Comment at: lib/Support/Unix/Signals.inc:288
@@ +287,3 @@
+    const ElfW(Phdr) *phdr = &info->dlpi_phdr[i];
+    if (phdr->p_type == PT_LOAD) {
+      intptr_t beg = info->dlpi_addr + phdr->p_vaddr;
----------------
could be an early continue instead

================
Comment at: lib/Support/Unix/Signals.inc:294
@@ +293,3 @@
+        intptr_t addr = (intptr_t)data->StackTrace[j];
+        if (beg <= addr && addr < end) {
+          data->modules[j] = name;
----------------
could be an early continue, but is of questionable benefit (same number of lines of code, only two lines to unindent)

================
Comment at: lib/Support/Unix/Signals.inc:301
@@ +300,3 @@
+  }
+  return 0;
+}
----------------
The return value seems to be always zero and unused. Remove it

================
Comment at: lib/Support/Unix/Signals.inc:326
@@ +325,3 @@
+  SmallString<32> InputFile, OutputFile;
+  sys::fs::createTemporaryFile("symbolizer-input", "", InputFD, InputFile);
+  sys::fs::createTemporaryFile("symbolizer-output", "", OutputFile);
----------------
hmm - should we not be just reading the output stream straight from the process via a pipe, rather than writing it to disk and reading it back from there?

Maybe we don't have portable primitives for that. (maybe there's no such common/portable primitive)

================
Comment at: lib/Support/Unix/Signals.inc:331
@@ +330,3 @@
+  std::vector<const StringRef *> Redirects(3, nullptr);
+  auto InputFileStr = llvm::make_unique<const StringRef>(InputFile);
+  auto OutputFileStr = llvm::make_unique<const StringRef>(OutputFile);
----------------
Could these just be straight StringRefs, rather than unique_ptr<StringRef>?

================
Comment at: lib/Support/Unix/Signals.inc:348
@@ +347,3 @@
+  int RunResult =
+      sys::ExecuteAndWait(LLVMSymbolizerPath, args, nullptr, Redirects.data());
+  if (RunResult != 0)
----------------
just an aside: but why is symbolication out-of-process in this way? (I mean I can think of the obvious "Because the process is already in a bad state, we don't want to do lots of work in that process for fear the bad state would somehow corrupt all that work" - but I'm not sure if that's the case & how much it applies when we're still dynamically allocating several structures here so we can call llvm-symbolizer and print the result, etc)

================
Comment at: lib/Support/Unix/Signals.inc:364
@@ +363,3 @@
+      // encounter empty line.
+      for (;;) {
+        StringRef FunctionName = *CurLine++;
----------------
So this loop is where inlining is handled? (multiple results from llvm-symbolizer for a single query, terminated by a blank line?)

================
Comment at: lib/Support/Unix/Signals.inc:369
@@ +368,3 @@
+        fprintf(FD, "#%d %p ", frame_no++, StackTrace[i]);
+        if (!FunctionName.startswith("??")) {
+          fprintf(FD, "%s ", FunctionName.str().c_str());
----------------
usually skip the braces on single line blocks

================
Comment at: lib/Support/Unix/Signals.inc:375
@@ +374,3 @@
+          fprintf(FD, "%s", FileLineInfo.str().c_str());
+        } else {
+          fprintf(FD, "(%s+%p)", modules[i], (void *)offsets[i]);
----------------
More braces to skip (if and else)

================
Comment at: lib/Support/Unix/Signals.inc:381
@@ +380,3 @@
+    } else {
+      fprintf(FD, "#%d %p\n", frame_no++, StackTrace[i]);
+    }
----------------
This could be an early continue and reduce indentation

http://reviews.llvm.org/D5610