[PATCH] D129519: [Symbolizer] Implement contextual symbolizer markup elements.

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Apologies for the delay in responding. Some small suggestions but otherwise looking good to me.



================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/MarkupFilter.h:64
+  // elements as possible are folded into one ModuleInfo line.
+  struct ModuleInfo {
+    const Module *Module;
----------------
Would it be worth calling this `ModuleLineInfo`? or `ModuleInfoLine`? This would make it explicit from the name that this is a textual element and not information about Module that is permanently retained?


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/MarkupFilter.h:69
+    // it's from a later mmap.
+    bool IsOriginal;
+
----------------
Would `InlineMMaps` be more descriptive? I think this is true when there is a module info line that has a module element immediately followed by mmaps for that module id. False if a new module info line is started for a previously seen module id?  


================
Comment at: llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp:78
+    if (tryContextualElement(*Node, Deferred))
+      return true;
+
----------------
Was thinking if there was a neat way of making sure that if true then all Deferred nodes had been output. Only thing I could think of was clearing Deferred in the `FlushDeferred` lambda and asserting that it was empty here. It would sadly mean making DeferredNodes non const in `tryContextualElement` so debateable whether it is worth doing.  


================
Comment at: llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp:315
+// below, since the usual case upon returning a None Optional is to return None.
+#define ASSIGN_OR_RETURN(TYPE, NAME, EXPR)                                     \
+  auto NAME##Opt = (EXPR);                                                     \
----------------
Perhaps `ASSIGN_OR_RETURN_NONE`? Makes it a bit clearer at the call site. Not a strong opinion though.


================
Comment at: llvm/test/DebugInfo/symbolize-filter-markup-module.test:12
+CHECK: [[BEGIN]]ELF module #0x1 "b.o"; adds 0x0(r)[[END]]
+ERR: error: expected at least 3 fields; found 0
+ERR: error: unknown module type
----------------
Worth adding a newline to space the errors from the check as I think that would match the log below, which does have a newline in it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129519