[PATCH] D139750: Optionally print symbolizer markup backtraces.

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr accepted this revision.
mcgrathr added a comment.
This revision is now accepted and ready to land.

Some minor improvements could be made, but this generally lgtm.



================
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;
----------------
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).



================
Comment at: llvm/lib/Support/Unix/Signals.inc:526
+    ArrayRef<uint8_t> BuildID = findBuildID(Info);
+    if (BuildID.empty())
+      return;
----------------
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.


================
Comment at: llvm/lib/Support/Unix/Signals.inc:603
+  // least 4 bytes.
+  void fillModeStr(uint32_t Flags, char *Str) {
+    if (Flags & PF_R)
----------------
This can use `char Str[4]` in the decl to document the requirement on the size (and some compilers will even do warnings based on that).  But IMHO it would be better for this function to simply return `std::array<char, 4>` itself.


================
Comment at: llvm/lib/Support/Unix/Signals.inc:616
+                               const char *MainExecutableName) {
+  OS << "{{{reset}}}\n";
+  DSOMarkupPrinter MP(OS, MainExecutableName);
----------------
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.



================
Comment at: llvm/lib/Support/Unix/Signals.inc:538
+      OS << format("{{{mmap:%#016x:%#x:load:%d:%s:%#016x}}}\n", StartAddress,
+                   Phdr->p_memsz, ModuleCount, (char *)ModeStr,
+                   ModuleRelativeAddress);
----------------
mysterymath wrote:
> phosek wrote:
> > Can you use `reinterpret_cast` instead of C-style cast?
> This is just an array to pointer decay, so `static_cast` works. format just doesn't like being handed an array type.
IMHO `&ModeStr[0]` is more natural.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139750