[PATCH] D96883: Add support for JSON output style to llvm-symbolizer

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:99-100
+
+  void listBegin() override{};
+  void listEnd() override{};
 };
----------------
dblaikie wrote:
> aorlov wrote:
> > jhenderson wrote:
> > > Is this clang-formatted properly? Slightly surprised there's no space after `override`.
> > It is generated by clang-format and validated by clang-tidy.
> The extraneous trailing semicolon is probably confusing clang-format, so maybe remove those and see how it gets formatted?
Ah, good spot!


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:218
+static json::Object toJSON(const Request &Request, StringRef ErrorMsg = "") {
+  json::Object J({{"ModuleName", Request.ModuleName.str()}});
+  if (Request.Address)
----------------
There's still an unfortunate single-letter variable name here. I'd suggest `Json` or `Object` for the variable name.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:227
+void JSONPrinter::print(const Request &Request, const DILineInfo &Info) {
+  DIInliningInfo I;
+  I.addFrame(Info);
----------------
Same comment as above. How about `InliningInfo`?


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:235
+  for (uint32_t I = 0, N = Info.getNumberOfFrames(); I < N; ++I) {
+    const DILineInfo &LI = Info.getFrame(I);
+    Array.push_back(json::Object(
----------------
`LI` is a similar abbreviation which doesn't clearly indicate its name. What's wrong with `LineInfo`?


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:247
+  }
+  json::Object J = toJSON(Request);
+  J["Symbol"] = std::move(Array);
----------------
`J` -> `Json` or `Object`


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:260
+       {"Size", toHex(Global.Size)}});
+  json::Object J = toJSON(Request);
+  J["Data"] = std::move(Data);
----------------
`Object` or `Json`.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:271
+  json::Array Frame;
+  for (const DILocal &L : Locals) {
+    json::Object Object(
----------------
`Local`


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:272
+  for (const DILocal &L : Locals) {
+    json::Object Object(
+        {{"FunctionName", L.FunctionName},
----------------
Maybe use `FrameObject` here to disambiguate from the `toJson` retiurn below.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:283
+  }
+  json::Object J = toJSON(Request);
+  J["Frame"] = std::move(Frame);
----------------
`Json` or `RequestJson` or similar.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:302
+                             StringRef ErrorBanner) {
+  json::Object J = toJSON(Request, ErrorInfo.message());
+  if (ObjectList)
----------------
`Json` or `Object`


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-code.test:46
+## The expected result is the same with -f -i.
+# RUN: llvm-addr2line --output-style=JSON -f -i -e %p/Inputs/addr.exe < %p/Inputs/addr.inp | \
+# RUN:   FileCheck %s --check-prefix=INLINE-A2L --strict-whitespace --match-full-lines --implicit-check-not={{.}}
----------------
If I follow this correctly, this test case is showing that llvm-addr2line with -f results in the function name being printed? Assuming that's correct, I think you need to show that llvm-addr2line without -f results in the function name not being included in the JSON output too.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-frame.test:38
+##
+## clang --target=aarch64-linux-android -fsanitize=hwaddress -fsanitize-hwaddress-abi=platform frame.c -O -g -S -o frame.s
+
----------------
Please include the version of clang explicitly here, not just in the ident string below. The reason is that someone might come along and trim off the superfluous parts of the below assembly to minimise the test case, but it would still be helpful to know how to generate the unmodified part. I would also use a final release version of clang, so that a future user can easily get the exact version of clang well into the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96883