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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thinking about it further, I think the JSON output stream is conceptually a higher-level output style than just for ELF files. In particular, you could imagine the future support being added for e.g. COFF and Mach-O objects (they'd have a different internal layout, but they'd still be objects to be printed using the JSON printer). Given that, it makes sense for this printer to be owned higher up the stack, i.e. as you're suggesting, where the ScopedPrinter currently lives. You'd still want a JSON object dumper implementation. Thus, you'd have two levels: your "ScopedPrinter" replacement would be responsible for printing the outer braces and object names (and handle archives probably too, although from a pure design aesthetic, an ArchiveDumper class might be nicer to handle this), with then individual ObjDumper instances handling printing the contenst of an object (including its corresponding outer braces). This is then nice, because the ObjDumper could be used to print a JSON object of the relevant type in other situations, not just within the outer ScopedPrinter instance. Ultimately, this sounds more-or-less like what you're proposing, which is great.

One other thought I had: I think we may need to be careful about duplicate objects in the input list: at the moment, they are dumped once per instance, i.e. `llvm-readobj input.o input.o` prints `input.o` twice. We probably should replicate this behaviour, which means we can't use object names as keys directly. I think this means we actually want a top-level array, with a name field in each object, which slightly breaks my design idea above. Alternatively, you could have something like:

  [
    {
      "object.o" : {
        ... # Contents of object.o
      },
    }
  ...
  ]

There's essentially the same issue with archives: it's potentially possible for an archive to have two members with the same name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111658