[PATCH] D111658: Add JSON output skeleton to llvm-readelf

[Jayson Yan via Phabricator via llvm-commits]

Jaysonyan added a comment.

In D111658#3117710 <https://reviews.llvm.org/D111658#3117710>, @jhenderson wrote:

> Sorry, I'm a little bit busy, and have a number of other reviews that are calling for my attention too. I haven't had a chance to give this too much further thought yet. One initial thought I had was that things like `printLine` might just be a no-op for JSON output, although I think it might be nicer if we could wrap these sorts of calls behind another interface suitable for generic llvm-readobj usage. Unfortunately, I don't have the time right now to take a look at all usages, to see if there's a way we could represent such functions in a cleaner manner. I'll try to come back to this later this week, or next week, if I can.

That's totally understandable and I appreciate the time you're taking to review this change. For the sake of saving time, I'll provide all of my thoughts on the design as well as my reasoning behind them in the hopes that it could save some back and forth.

I think that having `JSONScopedPrinter` as a subclass of `ScopedPrinter` would have more downsides than upsides without major refactoring to how we interact with `ScopedPrinter` in `llvm-readobj`. I believe the biggest driving force to having `JSONScopedPrinter` as a subclass to `ScopedPrinter` is with the hope that each implementation of `ObjDumper` can be agnostic to the type of `ScopedPrinter` it is using (json or regular). Although I don't believe that this is possible for a few reasons:

1. `ScopedPrinter` has a few template methods. I know template and virtual methods don't mix well. I'm not sure if there is a way around this that LLVM has used but I thought it'd be valuable to bring up if this is an issue.
2. The `startLine()` method as mentioned previously, provides access to the underlying `raw_ostream`. While it is possible to provide this functionality in the `JSONScopedPrinter` (through calling json::OStream::rawValueBegin <https://llvm.org/doxygen/classllvm_1_1json_1_1OStream.html#a792749e73c83a275ccff99e769c64569>), it would be non-trivial to ensure that the usage of this results in valid json (we would also need to find a way to call json::OStream::rawValueEnd <https://llvm.org/doxygen/classllvm_1_1json_1_1OStream.html#ad3381dc4020a5d5353451a2348e3ec24> afterwards). Alternatively, making `startline()` in `JSONScopedPrinter` an no-op may result in necessary information being omitted.
3. The structure of json can trip up certain usage of `ScopedPrinter`. One example is attributes inside of lists, here is an example of code inside `LLVMElfDumper`:

  template <class ELFT> void LLVMELFDumper<ELFT>::printSectionHeaders() {
    ...
    if (opts::SectionSymbols) {
      ListScope D(W, "Symbols");
      ...
      for (const Elf_Sym &Sym : Symbols) {
        ...
        printSymbol(...);
      }
    ...
  }
  
  void LLVMELFDumper<ELFT>::printSymbol(...) {
    ...
    DictScope Group(W, "Symbol");
    W.printNumber("Name", FullSymbolName, Symbol.st_name);
    W.printHex("Value", Symbol.st_value);
    ...
  }

So with a `ScopedPrinter` this could produce something like:

  Symbols [
    Symbol {
      Name: foo (1)
      Value: 0x0
    }
    Symbol {
      Name: bar (2)
      Value: 0x0
    }
  ]

Although the equivalent json would not be valid due to attributes inside of a list:

  "Symbols": [
    "Symbol": {
      "Name": "foo",
      "Value": "0x0"
    },
    "Symbol": {
      "Name": "bar",
      "Value": "0x0"
    }
  ] 

So for these reasons I don't think we'd be able to have `ScopedPrinter`-agnostic `ObjDumper` implementation without significant refactors to how we interact with `ScopedPrinter` to ensure we're printing valid json while maintaining the current LLVM format remains unchanged. And if we don't receive this benefit I believe the downsides of having `JSONScopedPrinter` inherit from `ScopedPrinter` (`JSONScopedPrinter` needing offer all methods `ScopedPrinter` offers, many of which would be unused) outweigh the benefits.

Ultimately I think it would be ideal to reach a point where an `ObjDumper` can be printer-agnostic but it would require non-trivial refactors in most `ObjDumper` implementations to reach that point. And since currently our only need is for `llvm-readelf` to output json, I believe the current implementation is sufficient for addressing our needs.


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

https://reviews.llvm.org/D111658