[Symbolizer] Ignore unknown additional symbolizer markup fields

[mysterymath]

Summary

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.

[mcgrathr]

llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp

136–137

I don't know of a reason to print anything at all when a reset element is encountered.

llvm/test/DebugInfo/symbolize-filter-markup-reset.test

24–26

A trailing colon is an odd case, though I see no reason not to accept it. But it's never what an expected future extension would look like. Probably the test should cover reset:this=1:that, etc.

[mysterymath]

Update test cases.

llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp

136–137

This is mostly just to provide context for error messages; it would be highly surprising to see a "no mmap covers address" error not too long after a mmap that appears to cover the address, but where a reset was present in-between.

llvm/test/DebugInfo/symbolize-filter-markup-reset.test

24–26

Good point; the tool is agnostic to what the field looks like, and these tests are fairly white-box, so I've changed these to add an "ext" field to make the purpose of the case clearer.

llvm/docs/SymbolizerMarkupFormat.rst

 short alphabetic string that must be in lower case. The rest of the element
 consists of one or more fields. Fields are separated by ``:`` and cannot contain
 any ``:`` or ``}`` characters. How many fields must be or may be present and
 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:
 
 ``%s``
   A string of printable characters, not including ``:`` or ``}``.
 
 ``%p``
   An address value represented by ``0x`` followed by an even number of

llvm/include/llvm/DebugInfo/Symbolize/MarkupFilter.h

private:
   object::BuildID parseBuildID(StringRef Str) const;
   std::optional<std::string> parseMode(StringRef Str) const;
   std::optional<PCType> parsePCType(StringRef Str) const;
   std::optional<uint64_t> parseFrameNumber(StringRef Str) const;
 
   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;
 
   const MMap *getOverlappingMMap(const MMap &Map) const;
   const MMap *getContainingMMap(uint64_t Addr) const;
 
   uint64_t adjustAddr(uint64_t Addr, PCType Type) const;

llvm/lib/DebugInfo/Symbolize/MarkupFilter.cpp

if (Node.Tag != "reset")
     return false;
   if (!checkNumFields(Node, 0))
     return true;
 
   if (!Modules.empty() || !MMaps.empty()) {
     endAnyModuleInfoLine();
     for (const MarkupNode &Node : DeferredNodes)
       filterNode(Node);
-    highlight();
-    OS << "[[[reset]]]" << lineEnding();
-    restoreColor();
+    printRawElement(Node);
+    OS << lineEnding();
 
     Modules.clear();
     MMaps.clear();
   }
   return true;
 }
 
 bool MarkupFilter::tryModule(const MarkupNode &Node,
bool MarkupFilter::trySymbol(const MarkupNode &Node) {
   return true;
 }
 
 bool MarkupFilter::tryPC(const MarkupNode &Node) {
   if (Node.Tag != "pc")
     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)
     return true;
 
   // PC addresses that aren't part of a backtrace are assumed to be precise code
   // locations.
   PCType Type = PCType::PreciseCode;
bool MarkupFilter::tryPC(const MarkupNode &Node) {
   return true;
 }
 
 bool MarkupFilter::tryBackTrace(const MarkupNode &Node) {
   if (Node.Tag != "bt")
     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)
     return true;
 
   std::optional<uint64_t> Addr = parseAddr(Node.Fields[1]);
   if (!Addr)
     return true;
if (any_of(Node.Tag, [](char C) { return C < 'a' || C > 'z'; })) {
     return false;
   }
   return true;
 }
 
 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;
 }
 
 bool MarkupFilter::checkNumFieldsAtLeast(const MarkupNode &Element,
                                          size_t Size) const {
   if (Element.Fields.size() < Size) {
     WithColor::error(errs())
         << "expected at least " << Size << " field(s); found "
         << Element.Fields.size() << "\n";
     reportLocation(Element.Tag.end());
     return false;
   }
   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,
+  if (Element.Fields.size() <= Size)
+    return;
+  WithColor::warning(errs())
 }
 
 void MarkupFilter::reportTypeError(StringRef Str, StringRef TypeName) const {
   WithColor::error(errs()) << "expected " << TypeName << "; found '" << Str
                            << "'\n";
   reportLocation(Str.begin());
 }
 

llvm/test/DebugInfo/symbolize-filter-markup-bt.test

 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:    #1.1  0x0000000000000018 second /tmp[[SEP]]tmp.c:8:3 (a.o+0x8)
 CHECK:    #1    0x0000000000000018 first /tmp[[SEP]]tmp.c:4:3 (a.o+0x8)
 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}}}
 {{{mmap:0x10:256:load:0:r:0}}}
 {{{bt:0:0x19}}}
 {{{bt:1:0x19}}}
 {{{bt:0:0x19:ra}}}
 {{{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:
 #
 # static void second(void);
 # void first(void) {
 #   second();
 # }

llvm/test/DebugInfo/symbolize-filter-markup-pc.test

 REQUIRES: x86-registered-target
 RUN: split-file %s %t
 RUN: mkdir -p %t/.build-id/ab
 RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %t/asm.s \
 RUN:   -o %t/.build-id/ab/cdef.debug
 RUN: llvm-symbolizer --debug-file-directory=%t --filter-markup < %t/input \
 RUN:   > %t.output 2> %t.err
 RUN: FileCheck %s --input-file=%t.output --match-full-lines \
 RUN:   --implicit-check-not {{.}}
 RUN: FileCheck %s --check-prefix=ERR --input-file=%t.err --match-full-lines
 
 CHECK: [[BEGIN:\[{3}]]ELF module #0x0 "a.o"; BuildID=abcdef [0x0-0xff](r)[[END:\]{3}]]
 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}}}
 {{{mmap:0:256:load:0:r:0}}}
 {{{pc:0}}}
 {{{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"
   .globl  first                           # -- Begin function first
   .p2align  4, 0x90
   .type first,@function
 first:                                  # @first
 .Lfunc_begin0:

llvm/test/DebugInfo/symbolize-filter-markup-reset.test

 RUN: split-file %s %t
 RUN: llvm-symbolizer --filter-markup < %t/log > %t.out 2> %t.err
 RUN: FileCheck %s --input-file=%t.out --match-full-lines \
 RUN:   --implicit-check-not {{.}}
 RUN: FileCheck %s --check-prefix=ERR -input-file=%t.err --match-full-lines
 
 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}}}
 {{{module:0:a.o:elf:ab}}}
 {{{mmap:0:1:load:0:r:0}}}
   {{{reset}}}
 {{{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}}}