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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've not looked at the testing yet. Here are comments on the code to keep you going though.



================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:214
 
+std::string toHex(uint64_t V) { return ("0x" + Twine::utohexstr(V)).str(); }
+
----------------
`static`


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:216
+
+void toJSON(json::OStream &J, const Request &Request, int ErrorCode = 0,
+            const StringRef ErrorMsg = "") {
----------------
`static`


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:221
+  J.attribute("Address", toHex(Request.Address));
+  J.attributeObject("Error", [&] {
+    J.attribute("Code", ErrorCode);
----------------
I suspect from a user's perspective they won't expect to see an `Error` attribute if there's no error.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:222
+  J.attributeObject("Error", [&] {
+    J.attribute("Code", ErrorCode);
+    if (!ErrorMsg.empty())
----------------
What is `ErrorCode` supposed to represent? How will a user benefit from it beyond the information provided in the message?


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:240
+    J.attribute("FileName", Info.FileName);
+  // Print only valid line and column to reduce noise in the output.
+  if (Info.Line)
----------------
This is intended to be machine readable. "Noise" in the output isn't an issue. In fact, as previously mentioned, not having these and other attributes is actively harmful to the user experience, as it makes it harder to write a parser that consumes this JSON.

In the case where you have a BadString output, I'd just print an empty string. Example:
```
{ "Source" : "", "FunctionName" : "", ... , "Line" : 0, "Column" : 0, "Discriminator" : 0 }
```


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:262
+  }
+  OS << '\n';
+}
----------------
What's the point of the new line? Same goes elsewhere where you are adding new lines. If the output is intended to be machine readable, there is no need for the new lines.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:313
+      J.objectBegin();
+      if (!L.FunctionName.empty())
+        J.attribute("FunctionName", L.FunctionName);
----------------
Same as above - just print everything.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:161-164
     Printer.printInvalidCommand(
         {ModuleName, Offset},
         StringError(InputString,
                     std::make_error_code(std::errc::invalid_argument)));
----------------
I missed this in the other review. We don't need to make an `ErrorInfo` here at all. Just pass the `InputString` in instead. That will simplify both the call-site and the `printInvalidCommand` function.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:360-363
+    if (Style == OutputStyle::JSON && InputAddresses.size() > 1) {
+      ArrayJSON = true;
+      outs() << "[";
+    }
----------------
I would make it always a list, not just for multiple input addresses. Also, shouldn't this be using the json stream arrayBegin/End methods?


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:366
+    for (StringRef Address : InputAddresses) {
+      if (ArrayJSON) {
+        if (ArrayDelimJSON)
----------------
There's no need for this outer if.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:367-370
+        if (ArrayDelimJSON)
+          outs() << ",";
+        else
+          ArrayDelimJSON = true;
----------------
How about:

```
StringRef Sep = "";
for (StringRef Address : InputAddresses) {
  outs() << Sep;
  Sep = ",";
  symbolizeInput(Args, AdjustVMA, IsAddr2Line, Style, Address, Symbolizer,
              *Printer);
}
```

This may be moot however, if you are using the JSON stream to use the proper JSON array methods.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96883