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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Please remember to upload your diffs with full context, as per this article: https://llvm.org/docs/Phabricator.html.

You'll need to document the new output style in the llvm-symbolizer and llvm-addr2line documentation located in llvm\docs\CommandGuide, preferably with one or more examples.

How does this new output style interact with existing options (I'm thinking -p, -a, -i and any other option that affects the output)? What about DATA etc output? You need testing which shows this interaction.

You also need testing for addresses not found.

If we're going to add support for this, I don't think in YAML output mode that it's a good idea to print lines of output that aren't part of the YAML (in this case the "some text" lines which correspond to invalid input addresses).



================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:42
+  static void mapping(IO &IO, DILineInfo &Value) {
+    Optional<StringRef> EmptySource;
+    IO.mapOptional("Source", Value.Source);
----------------
What's this for? clang-tidy is complaining it is unused.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:166
+    DIInliningInfoYaml Val;
+    for (uint32_t i = 0; i < FramesNum; i++)
+      Val.Frames.push_back(Info.getFrame(i));
----------------
`i` -> `I` to match LLVM style.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-yaml.test:1-25
+This test checks YAML output.
+
+RUN: llvm-symbolizer --output-style=YAML -e %p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck %s
+
+CHECK: some text
+CHECK-NEXT: ---
+CHECK-NEXT: Frames:
----------------
It's somewhat traditional for tests I've seen to have comment markers for all lit and FileCheck lines, plus genuine comments, even if they aren't really needed. This makes it easier to maintain them going forwards too.

In this case, I'd add `## ` for the real comment line, and `# ` before each RUN and CHECK line (with the space between `#` and the start of the rest of the line in all cases).


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-yaml.test:3
+
+RUN: llvm-symbolizer --output-style=YAML -e %p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck %s
+
----------------
Add a matching line for `llvm-addr2line --output-style=YAML` too, to show the different default can still be overridden.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96289