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

[Alex Orlov via Phabricator via llvm-commits]

aorlov marked 5 inline comments as done.
aorlov added a comment.

I do not think we should always include everything and anything into JSON. There is nothing wrong with skipping parameters with unknown values, not applicable data and such.

For example `DILocal` contains `Optional<int64_t> FrameOffset`. In JS it would be declared as `FrameOffset?: number;` and handled natively.

If FrameOffset is not specified we cannot print out the value, any number is a valid offset, and a non-number string would confuse the parses much more than just the optional field which is skipped when N/A.

Besides, our customers are happy with the proposed JSON and do not have any problem parsing optional data.



================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:221
+  J.attribute("Address", toHex(Request.Address));
+  J.attributeObject("Error", [&] {
+    J.attribute("Code", ErrorCode);
----------------
jhenderson wrote:
> I suspect from a user's perspective they won't expect to see an `Error` attribute if there's no error.
Error: {Code: 0} – is a standard way to say `success`. And it is a bad idea to omit Error if it should be checked first.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:222
+  J.attributeObject("Error", [&] {
+    J.attribute("Code", ErrorCode);
+    if (!ErrorMsg.empty())
----------------
jhenderson wrote:
> What is `ErrorCode` supposed to represent? How will a user benefit from it beyond the information provided in the message?
The `ErrorCode` is more important than a message when automating the error handling. The error message may be localized, depend on OS, etc.


================
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)
----------------
jhenderson wrote:
> 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 }
> ```
Ok, but I still omit empty Error Message and FrameOffset.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:367-370
+        if (ArrayDelimJSON)
+          outs() << ",";
+        else
+          ArrayDelimJSON = true;
----------------
jhenderson wrote:
> 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.
Did you forget about different commas in GNU/LLVM output style? I do not want any functional change in that area as a part of this patch.
I have added groupBegin() and groupEnd() to DIPrinter interface and moved all logic to JSONPrinter implementation.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96883