[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/lib/DebugInfo/Symbolize/DIPrinter.cpp:255
+  else {
+    json::OStream JOS(OS, Pretty ? 2 : 0);
+    JOS.value(std::move(J));
----------------
I didn't think of this earlier, but it makes a lot of sense to have the pretty printing form be more human readable, with indentation and new lines as appropriate. Thanks!


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:271
+  else {
+    json::OStream JOS(OS, Pretty ? 2 : 0);
+    JOS.value(std::move(J));
----------------
Did you consider having a single `json::OStream` as a member of the JSONPrinter class, so that it only needs constructing (with the `Pretty` check) in one place?


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:221
+  J.attribute("Address", toHex(Request.Address));
+  J.attributeObject("Error", [&] {
+    J.attribute("Code", ErrorCode);
----------------
aorlov wrote:
> 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.
Right, but basically the behaviour of checking whether "Error" exists is no more complicated in most languages than checking whether it has value zero.

I would kind of think there would be two possible JSON objects produced per query (possibly three if we handle invalid commands differently to other errors for JSON output):
```
{"Error":"some message/code/whatever"}
{"Source":"some path",...}
```
Where the response had an error, it is unlikely the other parameters can be relied upon in any meaningful way, so it's probably better to omit them than potentially cause confusion.
The pseudo-python logic for this  might look something like:
```
response = json.load(output)
if 'Error' in response:
  handleError(response)
else:
  handleNormalResponse(response)
```


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:222
+  J.attributeObject("Error", [&] {
+    J.attribute("Code", ErrorCode);
+    if (!ErrorMsg.empty())
----------------
aorlov wrote:
> 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.
The error codes contained in `llvm::Error` and `llvm::Expected` are quite often somewhat arbitrary, and inconsistent. It won't be possible to safely rely on these codes for any useful automated processing. Furthermore, some Errors could contain `inconvertibleErrorCode` which will cause a problem if these end up back here.

What's the use-case for handling different error kinds differently? I'm not saying there isn't a motivation for that, I'm just trying to understand how you plan to use it. If you have no such plan, it doesn't make sense to add additional logic to distinguish errors by code as well as 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)
----------------
aorlov wrote:
> 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.
That seems reasonable, thanks for the explanation.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:367-370
+        if (ArrayDelimJSON)
+          outs() << ",";
+        else
+          ArrayDelimJSON = true;
----------------
aorlov wrote:
> 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.
> 
Thanks, yes, I'd forgotten this was the higher-level printer area.

"group" doesn't obviously mean anything to me. Could you consider renaming it to something like "listBegin" etc? I think that more clearly indicates what you're doing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96883