[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{};
 };
----------------
Is this clang-formatted properly? Slightly surprised there's no space after `override`.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:217-218
+
+static json::Object toJSON(const Request &Request, int ErrorCode = 0,
+                           const StringRef ErrorMsg = "") {
+  json::Object J({{"ModuleName", std::string(Request.ModuleName)}});
----------------
`ErrorCode` is unused now. Also, no need for the `const` on `ErrorMsg`.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:219
+                           const StringRef ErrorMsg = "") {
+  json::Object J({{"ModuleName", std::string(Request.ModuleName)}});
+  if (Request.Address)
----------------
Usually we use `StringRef.str()` to convert to a `std::string`. Same applies below in the error message line.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:234-235
+void JSONPrinter::print(const Request &Request, const DIInliningInfo &Info) {
+  json::Array A;
+  uint32_t N = Info.getNumberOfFrames();
+  for (uint32_t I = 0; I < N; ++I) {
----------------
Don't abbeviate these names unnecessarily like this. `Array` and `FrameCount` would both be acceptable, for example. In general, avoid single letter variable names except for loop counters. Same goes throughout this patch. See https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly for details:

> We cannot stress enough how important it is to use descriptive names. Pick names that match the semantics and role of the underlying entities, within reason. Avoid abbreviations unless they are well known.

I think you can inline `N` into the initialising part of the `for` loop? I don't see it being used outside the loop.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:238
+    const DILineInfo &LI = Info.getFrame(I);
+    json::Object O(
+        {{"FunctionName",
----------------
`Object` maybe?


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:248
+         {"Discriminator", LI.Discriminator}});
+    if (LI.Source && Config.SourceContextLines > 0) {
+      int64_t FirstLine =
----------------
I think adding source context can be a later patch. Let's try to avoid adding more than we have to in this one patch.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-code-source.c:3
+
+// RUN: clang --target=x86_64-pc-linux -gdwarf-5 -gembed-source -g -c %s -o %t.o
+// RUN: llvm-symbolizer --output-style=JSON --print-source-context-lines=2 --obj=%t.o 0 | \
----------------
This won't work. clang isn't available in llvm-symbolizer tests, as the latter are part of the LLVM layer. Clang depends on the LLVM layer, but not vice-versa. You might be able to use `llvm-mc -g` to compile some assembly to be able to test this though.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-code.test:3
+
+## If the addresses are specified in the command line, the output JSON will
+## contain an array of the results for all of the given addresses.
----------------



================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-code.test:6
+
+## Handle symbolize library error - file does not exist.
+# RUN: llvm-symbolizer --output-style=JSON -e %p/no-file.exe 0 | \
----------------
aorlov wrote:
> jhenderson wrote:
> > 
> But note the library name is `symbolize`, not `symbolizer`.
Fair point. How about simply `## Show how library errors are reported in the output.`


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-data.test:5
+
+## Handle symbolize library error - file does not exist.
+# RUN: llvm-symbolizer "DATA %t-no-file.o 0" --output-style=JSON | \
----------------
Same comment as the code test.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-data.test:27
+
+## Test multiple addresses in the command line.
+# RUN: llvm-symbolizer -e=%t.o "DATA 2" "DATA 8" --output-style=JSON | \
----------------
Same comment as above.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-frame.c:24
+
+// RUN: clang --target=aarch64-linux-android -fsanitize=hwaddress -fsanitize-hwaddress-abi=platform %s -O -g -S -o %t.s
+// RUN: llvm-mc -filetype=obj -triple=aarch64-linux-android -o %t.o %t.s
----------------
As above. This won't work. You should use llvm-mc to build from assembly, as in other test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96883