[PATCH] D139750: Optionally print symbolizer markup backtraces.

[Daniel Thornburgh via Phabricator via llvm-commits]

mysterymath added inline comments.


================
Comment at: llvm/lib/Support/Signals.cpp:271
+  for (int I = 0; I < Depth; I++)
+    OS << format("{{{bt:%d:%#016x}}}\n", I, StackTrace[I]);
+  return true;
----------------
mcgrathr wrote:
> Best format is to append `:ra` or `:pc` depending on whether it's a return-address or exact PC frame.
> When it's from a crash, the first PC is an exact PC and usually others are return-address frames (though when using full CFI unwinding, individual frames might be exact PC frames).
> 
Unfortunately, the first PC will be well within the signal handler, so the source of the signal will be in some earlier frame. It seems difficult to find it, and separately difficult to tell whether it is a raise, direct function call, or a e.g. SIGSEGV. Accordingly, this seems like it would be a useful enhancement, but I'd expect it to be of a similar size to this patch.


================
Comment at: llvm/lib/Support/Unix/Signals.inc:526
+    ArrayRef<uint8_t> BuildID = findBuildID(Info);
+    if (BuildID.empty())
+      return;
----------------
mcgrathr wrote:
> It's less useful but you can still print the context if there's no build ID. The name might be helpful, though automated lookup likely won't work.
At least at present, llvm-symbolizer doesn't accept module lines without a supported module type. The definition for "elf" also requires a build ID. It's possible to change this of course, but we'd probably want to do some surgery to the language of the spec too to make it more explicit that the typed portion of module and mmap are optional.


================
Comment at: llvm/lib/Support/Unix/Signals.inc:616
+                               const char *MainExecutableName) {
+  OS << "{{{reset}}}\n";
+  DSOMarkupPrinter MP(OS, MainExecutableName);
----------------
mcgrathr wrote:
> You may want to use the new `reset:begin` / `reset:end` pair around the context + backtrace, in which case IMHO neither really belongs in the "context" part of the printing per se.
> 
Ack; this seems a better fit for a change set where we actually do something with them (plus define them in the reference).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139750/new/

https://reviews.llvm.org/D139750