[PATCH] D130187: [Symbolizer] Implement data symbolizer markup element.

[Daniel Thornburgh via Phabricator via llvm-commits]

mysterymath added inline comments.


================
Comment at: llvm/test/DebugInfo/symbolize-filter-markup-data.test:17
+
+ERR: error: expected 1 field(s); found 0
+ERR: error: no mmap covers address
----------------
peter.smith wrote:
> Possibly worth a test with too many fields?
Eh, for that test to fail and this one to pass, there would have to be a discontinuity in how the implementation handles inputs with too many fields vs too few fields. The implementation currently uses "==", and that's rather unlikely to accidentally change. A possible class of errors would be to swap checkNumFields with checkNumFieldsAtLeast or similar, but these each come with different messages.


================
Comment at: llvm/test/DebugInfo/symbolize-filter-markup-data.test:32
+  .long 0x11223344
+  .size l, 4
+byte:
----------------
peter.smith wrote:
> I can't see a label `l` in the test. Was the intention to set the size of long to 4? If so I think you'll need `.size long, 4`. The same applies to `.size b, 1` below.
> 
> Checking the object file the sizes of `long` and `byte` are 0.
Yep, these didn't get updated when I renamed the variables. Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130187