[PATCH] D96289: Add support for YAML output style to llvm-symbolizer

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've still got to review the testing further, but here are some comments for now.

As a tip, once you have addressed an inline comment, mark it as done, by clicking the checkbox on the comment, before uploading your latest patch version, as it will make it easier to see what you've attempted to address already.



================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:43
+    IO.mapOptional("Source", Value.Source);
+    IO.mapOptional("FunctionName", Value.FunctionName, llvm::DILineInfo::BadString);
+    IO.mapOptional("StartFileName", Value.StartFileName, llvm::DILineInfo::BadString);
----------------
Please reformat using clang-format, as per the linter comment.

Why add the `llvm::` namespace qualifier? You're already in the llvm namespace and use `DILineInfo` in the function signature without it.

Same goes elsewhere below.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:116
+    llvm::yaml::Output YOut(OS);
+    DILineInfo Val = Info; // copy: YOut<< requires mutability.
+    YOut << Val;
----------------
Re. the comment - I wonder if that's a bug in the `<<` implementation? Does it need a `const` adding to its signature, or does that break other things?


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-yaml-data.test:1
+## This test checks YAML output.
+
----------------



================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-yaml-data.test:7
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: echo DATA %t.o 0 | llvm-symbolizer --output-style=YAML | FileCheck %s
+
----------------
This could be simplified slightly, I believe, as shown in the inline edit.

It would also be a good idea to add `--strict-whitespace`, `--match-full-lines` and `--implicit-check-not={{.}}` to ensure that the output tested exactly matches what you expect.

`--strict-whitespace` ensures strings of multiple whitespace characters are not converted to a single space (in both input and check patterns), `--match-full-lines` ensures each check line matches the entirety of an input line, and `--implicit-check-not` in this case ensures there's no output apart from what is being explicitly checked for.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-yaml-data.test:9-12
+# CHECK: ---
+# CHECK-NEXT: Name:            foo
+# CHECK-NEXT: Size:            4
+# CHECK-NEXT: ...
----------------
A slight snag with the above suggestion is that the space between the ':' and the start of the check needs to go. You can make it look a little nicer by indenting the first `CHECK` line to line up the colons as shown in the inline edit.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-yaml-frame.test:1
+## This test checks YAML output.
+
----------------



================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-yaml-frame.test:7
+# RUN: llvm-mc -filetype=obj -triple=i386-linux-gnu -o %t.o %s
+# RUN: echo 'FRAME %t.o 0' | llvm-symbolizer --output-style=YAML | FileCheck %s
+
----------------
Same comments as the DATA test apply here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96289