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

[Alex Orlov via Phabricator via llvm-commits]

aorlov marked 11 inline comments as done.
aorlov added inline comments.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:48
+  if (Info.Column)
+    J.attribute("Column", Info.Column);
+  if (Info.Discriminator)
----------------
grimar wrote:
> grimar wrote:
> > So, the `Column` key is only emitted when a column is not `0` (`Info.Column` is `uint32_t`). Is it expected behavior?
> > Is the intention is to omit the key to reduce the noise in the output? This needs a comment if so.
> Thanks for adding the comment. You should probably also add an explicit test case to test/document the output when the `Column` value is `0` and the case when it is not.
> 
Done. Covered by the "Test JSON output style of empty DILineInfo" test along with the case of missing line information.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:89
+                          DIPrinter::OutputStyle OutputStyle,
+                          DIPrinter &Printer, bool DefIfErr = true) {
+  if (ResOrErr) {
----------------
grimar wrote:
> aorlov wrote:
> > grimar wrote:
> > > The meaning of `DefIfErr` name is not very clear, also it is only used for non-JSON case,
> > > what is a bit consusing I think, because the argument is just ignored for the `JSON` case.
> > > It makes the signature to bit a bit dirty.
> > > 
> > > Perhaps, instead of having this function, I'd intoduce a helper like the following in `symbolizeInput`:
> > > 
> > > ```
> > > auto Print = [&](Expected<T> &ResOrErr){
> > >   if (ResOrErr) {
> > >     Printer << *ResOrErr;
> > >     return;
> > >   }
> > > 
> > >   if (OutputStyle == DIPrinter::OutputStyle::JSON) {
> > >     handleAllErrors(std::move(ResOrErr.takeError()),
> > >       [&](const ErrorInfoBase &EI) { Printer << EI; });
> > >     return;
> > >   }
> > > 
> > >   error(ResOrErr);
> > > 
> > >   // Nice and helpful comment about why the Command::Frame is exception....
> > >   if (Cmd == Command::Frame)
> > >      Printer << T();
> > > };
> > > ```
> > > 
> > > Will it work?
> > No, because of template <typename T>. I have renamed DefIfErr to BackwardCompatibleErr.
> > I have no idea why there is no printout of the empty struct in case of Cmd == Command::Frame, probably it is a bug.
> > But it is unrelated to this patch. I just keep the current behavior for other OutputStyle.
> > I have no idea why there is no printout of the empty struct in case of Cmd == Command::Frame, probably it is a bug.
> > But it is unrelated to this patch. I just keep the current behavior for other OutputStyle.
> 
> It is true, but probably we shouldn't introduce a new strangely named `BackwardCompatibleErr` variable in this case.
> We know that `Command::Frame`, one of commands,  has a different specific behavior.
> I'd write the code to make it more obvious with a `Cmd` argument.
> Also, perhaps would add few early returns. Something like:
> 
> 
> ```
> template <typename T>
> static void printResultOrError(Expected<T> &ResOrErr,
>                                DIPrinter::OutputStyle OutputStyle,
>                                DIPrinter &Printer, Command Cmd) {
>   if (ResOrErr) {
>     Printer << ResOrErr.get();
>     return;
>   }
> 
>   if (OutputStyle == DIPrinter::OutputStyle::JSON) {
>     Printer.printErrorJSON(toString(ResOrErr.takeError()),
>                            std::make_error_code(std::errc::invalid_argument));
>     return;
>   }
> 
>   if (Cmd != Command::Frame)
>     Printer << T();
> }
> ```
Finally I have used std::is_same<T, std::vector<DILocal>>::value for compile time optimization.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96883