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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D96883#2627307 <https://reviews.llvm.org/D96883#2627307>, @dblaikie wrote:

> In D96883#2625490 <https://reviews.llvm.org/D96883#2625490>, @jhenderson wrote:
>
>> I'd like to have @MaskRay and @dblaikie comment on this before this gets committed, since they both were involved with the YAML version of this.
>
> I'm keeping an eye on it & appreciate your review so far.

+1

>> Sorry for the delay in getting to this. Having given it a bit of thought, I don't think this approach is a particularly good design, personally. Is the `DIPrinter` only used for the symbolization code? If it is, I think we need a wider refactor of it before adding this new output format, to avoid the need for repeated `if(OutputStyle == JSON)` kind of things. My opinion is to have a separate class per output format (JSON, GNU, LLVM), which share a common interface. These could either all be derivations of DIPrinter itself or a new object that DIPrinter owns (possibly it is templated on it, or possibly there is a separate output style base class they derive from), I'm not sure it matters too much. The ELF dumper for llvm-readobj is a good example of what I mean (it has separate GNU and LLVM versions). It's a bigger piece of work, but I think it makes sense to do, as it'll make the code easier to change in the future, should we either a) add another output format to it (imagine if you wanted to add YAML output style as well as JSON), or b) extend with more printing options.
>
> Yeah, sounds good. I don't think it's a huge amount of code/`if`s as-is, but equally that means not a huge amount of code to refactor & good to do it now, as you say.

These `DIPrinter &operator<<` overloads have very few call sites. They are only called in `llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp`.

  51   DIPrinter &operator<<(const DILineInfo &Info); 3 refs                                                                                                                                                                                                                                
  52   DIPrinter &operator<<(const DIInliningInfo &Info); 1 ref                                                                                                                                                                                                                             
  53   DIPrinter &operator<<(const DIGlobal &Global); 1 ref                                                                                                                                                                                                                                 
  54   DIPrinter &operator<<(const DILocal &Local); 1 ref 



>> Some general thoughts about the output style:
>>
>> - How do you handle multiple different input files? I think it would make a lot of sense to include the input file and address in the JSON. I think we could ignore the corresponding llvm-symbolizer options that enable/disable printing these for other formats.
>> - How is a parser supposed to distinguish between an error message output for an address and a valid output? It might make sense to include a "success" field or similar, I think.
>> - Strictly speaking, if multiple input addresses are used, I don't think the following is valid JSON:
>>
>>   {
>>     // output for address1
>>   }
>>   {
>>     // output for address2
>>   }
>>   ...
>>
>> I think it should be in a list:
>>
>>   [
>>     {
>>       // output for address1
>>     },
>>     {
>>       // output for address2
>>     },
>>     ...
>>   ]
>>
>> You could omit the list for a single input address, but I think for consistency purposes, I'd not do that.
>>
>> - That being said, for interactive mode where addresses are fed on stdin via a user (and not via a file redirected in), we probably want to omit the list aspect, since you won't know when to close the list.
>
> presumably when the user closes the input stream you can close the list - and before then, a user could still copy/paste a single list entry as valid json

Sounds good. The JSON text needs to be one value (https://tools.ietf.org/html/rfc8259#section-2).
Otherwise some editors's highlighting features may warn.



================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-frame.test:6
+## Handle symbolize library error - file does not exist.
+# RUN: llvm-symbolizer "FRAME %t-no-file.o 0" --output-style=JSON \
+# RUN:   | FileCheck %s -DMSG=%errc_ENOENT --check-prefix=NO-FILE --strict-whitespace --match-full-lines --implicit-check-not={{.}}
----------------
Nit: place ` | ` in the end instead of the beginning of the continuation line. This is a more common style in binary utilities.

The idea is that without `\` it is still clear the line needs continaution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96883