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

[Alex Orlov via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h:56
   DIPrinter &operator<<(const DIGlobal &Global);
-  DIPrinter &operator<<(const DILocal &Local);
+  DIPrinter &operator<<(const std::vector<DILocal> &Locals);
+
----------------
jhenderson wrote:
> Why not `ArrayRef` for `Locals`?
I'm using the exact type that symbolizeFrame() returns.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:165
+    J.attribute("Start", int64_t(Global.Start));
+    J.attribute("Size", int64_t(Global.Size));
+    J.objectEnd();
----------------
jhenderson wrote:
> grimar wrote:
> > aorlov wrote:
> > > grimar wrote:
> > > > Why do you convert `uint64_t` to `int64_t` here and in many places below?
> > > There is no json::OStream::attribute() version for uint32_t and uint64_t. 
> > > I can use an implicit conversion for uint32_t, but I need to convert uint64_t to int64_t explicitly.
> > > 
> > But this is simply wrong and might print a wrong result isn't?
> > 
> > E.g. imagine you do:
> > 
> > ```
> >     uint64_t XXX = 0xffffffffeeeeeeee;
> >     J.attribute("Start", int64_t(XXX));
> > ```
> > 
> > The output is:
> > 
> > ```
> > {"Name":"foo","Start":-286331154
> > ```
> > 
> > I think `Start` shouldn't have a negative value.
> > Seems you need to update the `json::OStream` implementation to fix it.
> > You also need to add a test for such situation(s).
> > 
> > Also. should `Start`/`Size` be printed as hex? I think it is very common to use hex for addresses/sizes.
> > There is no json::OStream::attribute() version for uint32_t and uint64_t. 
> 
> You could always add them!
Please note JSON allows 53-bits numbers. So we print huge numbers as strings "0x...".


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:165
+      Printer << StringError(std::make_error_code(std::errc::invalid_argument),
+                             "(address)");
+    } else {
----------------
jhenderson wrote:
> grimar wrote:
> > aorlov wrote:
> > > grimar wrote:
> > > > Can you use `createStringError` from `Error.h`?
> > > No, because it is hard to use Error to get the error code and message just for printout. 
> > > I'm using ErrorInfoBase as a holder instead.
> > If we introduce the `printErrorJSON` that I suggested in a different comment, then here it will be possible to write:
> > 
> > ```
> > Printer.printErrorJSON("unable to parse arguments: " + InputString,
> >   std::make_error_code(std::errc::invalid_argument));
> > ```
> > 
> > What is better, because allows to provide more context and more customizable error messages for JSON output.
> > What do you think?
> > No, because it is hard to use Error to get the error code and message just for printout. 
> 
> It really isn't that hard to do this. See either `toString` in Error.h (converts an `Error` to a `std::string`) or `handleAllErrors` (which allows you to handle `Error` in various ways, such as by getting its message.
> 
> But I see you already know how to do this in `printResOrErr`. Why not just pass the Error directly to the function??
We need the error code for the clear logic while error handling. The error message may be localized, etc.
DIPrinter must only print the data but do not handle errors. 
It is impossible to get the error code from the Error without handling.
Initially I have used ErrorInfoBase instead to pass the error information to DIPrinter. 
Now DICommon contains ErrorCode (it can be used as "success" flag too).
The error message is stored in DICommon<std::string>.Result only in case of error.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96883