[Symbolizer] Ignore unknown additional symbolizer markup fields

mysterymath · mysterymath · commit 7b31a73ffe87 · 2023-06-28T10:39:28.000-07:00
The symbolizer markup syntax is structured such that fields require only previous fields for their interpretation; this was originally intended to make adding new fields a natural extension mechanism for existing elements. This codifies this into the spec and makes the behavior of the llvm-symbolizer match. Extra fields are now warned about, but ignored, rather than ignoring the whole element. Reviewed By: mcgrathr Differential Revision: https://reviews.llvm.org/D153821
diff --git a/llvm/docs/SymbolizerMarkupFormat.rst b/llvm/docs/SymbolizerMarkupFormat.rst
@@ -143,6 +143,11 @@ what they contain is specified for each element type.
 No markup elements or ANSI SGR control sequences are interpreted inside the
 contents of a field.
 
+Implementations must ignore markup fields after those expected; this allows
+adding new fields to backwards-compatibly extend elements. Implementations need
+not ignore them silently, but the element should behave otherwise as if the
+fields were removed.
+
 In the descriptions of each element type, ``printf``-style placeholders indicate
 field contents:
 
diff --git a/llvm/include/llvm/DebugInfo/Symbolize/MarkupFilter.h b/llvm/include/llvm/DebugInfo/Symbolize/MarkupFilter.h
@@ -123,7 +123,7 @@ class MarkupFilter {
   bool checkTag(const MarkupNode &Node) const;
   bool checkNumFields(const MarkupNode &Element, size_t Size) const;
   bool checkNumFieldsAtLeast(const MarkupNode &Element, size_t Size) const;
-  bool checkNumFieldsAtMost(const MarkupNode &Element, size_t Size) const;
+  void warnNumFieldsAtMost(const MarkupNode &Element, size_t Size) const;
 
   void reportTypeError(StringRef Str, StringRef TypeName) const;
   void reportLocation(StringRef::iterator Loc) const;
diff --git a/llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp b/llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp
@@ -133,9 +133,8 @@ bool MarkupFilter::tryReset(const MarkupNode &Node,
     endAnyModuleInfoLine();
     for (const MarkupNode &Node : DeferredNodes)
       filterNode(Node);
-    highlight();
-    OS << "[[[reset]]]" << lineEnding();
-    restoreColor();
+    printRawElement(Node);
+    OS << lineEnding();
 
     Modules.clear();
     MMaps.clear();
@@ -239,8 +238,7 @@ bool MarkupFilter::tryPC(const MarkupNode &Node) {
     return false;
   if (!checkNumFieldsAtLeast(Node, 1))
     return true;
-  if (!checkNumFieldsAtMost(Node, 2))
-    return true;
+  warnNumFieldsAtMost(Node, 2);
 
   std::optional<uint64_t> Addr = parseAddr(Node.Fields[0]);
   if (!Addr)
@@ -293,8 +291,7 @@ bool MarkupFilter::tryBackTrace(const MarkupNode &Node) {
     return false;
   if (!checkNumFieldsAtLeast(Node, 2))
     return true;
-  if (!checkNumFieldsAtMost(Node, 3))
-    return true;
+  warnNumFieldsAtMost(Node, 3);
 
   std::optional<uint64_t> FrameNumber = parseFrameNumber(Node.Fields[0]);
   if (!FrameNumber)
@@ -655,10 +652,12 @@ bool MarkupFilter::checkTag(const MarkupNode &Node) const {
 bool MarkupFilter::checkNumFields(const MarkupNode &Element,
                                   size_t Size) const {
   if (Element.Fields.size() != Size) {
-    WithColor::error(errs()) << "expected " << Size << " field(s); found "
-                             << Element.Fields.size() << "\n";
+    bool Warn = Element.Fields.size() > Size;
+    WithColor(errs(), Warn ? HighlightColor::Warning : HighlightColor::Error)
+        << (Warn ? "warning: " : "error: ") << "expected " << Size
+        << " field(s); found " << Element.Fields.size() << "\n";
     reportLocation(Element.Tag.end());
-    return false;
+    return Warn;
   }
   return true;
 }
@@ -675,16 +674,14 @@ bool MarkupFilter::checkNumFieldsAtLeast(const MarkupNode &Element,
   return true;
 }
 
-bool MarkupFilter::checkNumFieldsAtMost(const MarkupNode &Element,
-                                        size_t Size) const {
-  if (Element.Fields.size() > Size) {
-    WithColor::error(errs())
-        << "expected at most " << Size << " field(s); found "
-        << Element.Fields.size() << "\n";
-    reportLocation(Element.Tag.end());
-    return false;
-  }
-  return true;
+void MarkupFilter::warnNumFieldsAtMost(const MarkupNode &Element,
+                                       size_t Size) const {
+  if (Element.Fields.size() <= Size)
+    return;
+  WithColor::warning(errs())
+      << "expected at most " << Size << " field(s); found "
+      << Element.Fields.size() << "\n";
+  reportLocation(Element.Tag.end());
 }
 
 void MarkupFilter::reportTypeError(StringRef Str, StringRef TypeName) const {
diff --git a/llvm/test/DebugInfo/symbolize-filter-markup-bt.test b/llvm/test/DebugInfo/symbolize-filter-markup-bt.test
@@ -18,12 +18,14 @@ CHECK:    #0.1  0x0000000000000018 second /tmp[[SEP]]tmp.c:8:3 (a.o+0x8)
 CHECK:    #0    0x0000000000000018 first /tmp[[SEP]]tmp.c:4:3 (a.o+0x8)
 CHECK:    #0    0x0000000000000019 first /tmp[[SEP]]tmp.c:5:1 (a.o+0x9)
 CHECK:    #0    0x00000000000000fe (a.o+0xee)
+
+CHECK:    #0    0x00000000000000fe (a.o+0xee)
+ERR: warning: expected at most 3 field(s); found 4
 CHECK: [[BEGIN]]bt:0:0x111[[END]]
+ERR: error: no mmap covers address
 
 ERR: error: expected at least 2 field(s); found 0
-ERR: error: no mmap covers address
 ERR: error: expected PC type; found ''
-ERR: error: expected at most 3 field(s); found 4
 
 ;--- input
 {{{module:0:a.o:elf:abcdef}}}
@@ -34,10 +36,11 @@ ERR: error: expected at most 3 field(s); found 4
 {{{bt:0:0x19:pc}}}
 {{{bt:0:0xff}}}
 
-{{{bt}}}
+{{{bt:0:0xff:pc:ext}}}
 {{{bt:0:0x111}}}
+
+{{{bt}}}
 {{{bt:0:0:}}}
-{{{bt:0:0:pc:}}}
 ;--- asm.s
 # Generated by running "clang -finline -g -S tmp.c" in the following tmp.c on
 # Linux x86_64:
diff --git a/llvm/test/DebugInfo/symbolize-filter-markup-pc.test b/llvm/test/DebugInfo/symbolize-filter-markup-pc.test
@@ -14,13 +14,15 @@ CHECK: first[/dir[[SEP:[/\\]]]tmp.c:3]
 CHECK: first[/dir[[SEP]]tmp.c:5]
 CHECK: first[/dir[[SEP]]tmp.c:4]
 CHECK: first[/dir[[SEP]]tmp.c:5]
+
 CHECK: [[BEGIN]]pc:0xff[[END]]
 CHECK: [[BEGIN]]pc:0x100[[END]]
+CHECK: first[/dir[[SEP]]tmp.c:5]
+ERR: error: no mmap covers address
+ERR: warning: expected at most 2 field(s); found 3
 
 ERR: error: expected at least 1 field(s); found 0
-ERR: error: no mmap covers address
 ERR: error: expected PC type; found ''
-ERR: error: expected at most 2 field(s); found 3
 
 ;--- input
 {{{module:0:a.o:elf:abcdef}}}
@@ -29,12 +31,13 @@ ERR: error: expected at most 2 field(s); found 3
 {{{pc:0x9}}}
 {{{pc:0x9:ra}}}
 {{{pc:0x9:pc}}}
+
 {{{pc:0xff}}}
+{{{pc:0x100}}}
+{{{pc:0x9:pc:ext}}}
 
 {{{pc}}}
-{{{pc:0x100}}}
 {{{pc:0x9:}}}
-{{{pc:0x9:pc:}}}
 ;--- asm.s
 	.text
 	.file	"tmp.c"
diff --git a/llvm/test/DebugInfo/symbolize-filter-markup-reset.test b/llvm/test/DebugInfo/symbolize-filter-markup-reset.test
@@ -8,7 +8,10 @@ CHECK: [[BEGIN:\[{3}]]ELF module #0x0 "a.o"; BuildID=ab [0x0-0x0](r)[[END:\]{3}]
 CHECK: {{  }}[[BEGIN]]reset[[END]]
 CHECK: [[BEGIN:\[{3}]]ELF module #0x0 "b.o"; BuildID=cd [0x1-0x1](r)[[END:\]{3}]]
 
-ERR: error: expected 0 field(s); found 1
+CHECK: [[BEGIN]]reset:ext[[END]]
+ERR: warning: expected 0 field(s); found 1
+
+CHECK: [[BEGIN:\[{3}]]ELF module #0x0 "a.o"; BuildID=ab [0x0-0x0](r)[[END:\]{3}]]
 
 ;--- log
   {{{reset}}}
@@ -18,4 +21,6 @@ ERR: error: expected 0 field(s); found 1
 {{{module:0:b.o:elf:cd}}}
 {{{mmap:0x1:1:load:0:r:0}}}
 
-{{{reset:}}}
+{{{reset:ext}}}
+{{{module:0:a.o:elf:ab}}}
+{{{mmap:0:1:load:0:r:0}}}
