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

[Alex Orlov via Phabricator via llvm-commits]

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


================
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();
----------------
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.



================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:89
+                          DIPrinter::OutputStyle OutputStyle,
+                          DIPrinter &Printer, bool DefIfErr = true) {
+  if (ResOrErr) {
----------------
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.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:165
+      Printer << StringError(std::make_error_code(std::errc::invalid_argument),
+                             "(address)");
+    } else {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96883