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

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Oct 13 00:46:10 PDT 2021


jhenderson added a comment.

Some high-level comments:

1. Please include a link to the RFC email thread where this was discussed, in your commit/patch description, for ease of reference.
2. Please make sure to create your diff with full context, using `-U999999` when you make the diff.
3. Please make sure to clang-format all your changes.
4. I think it makes sense to split this down even further than you have done. I'd start with literally just adding the option and skeleton dumping structure, without any implementation of the dump at all. Your later patches can then flesh out each individual dump option. This also means that you can have lots of child patches that base off this one, which are entirely independent, so it doesn't matter what order they get implemented or committed in.
5. You should update the llvm-readobj and llvm-readelf .rst files in llvm/docs/CommandGuide to discuss the new output mode, perhaps adding something like "Not all options are currently supported by this mode" or something to that effect.

Also, if you wish to add other commits in a series, that are built on top of this patch, you can use the "Edit Related Revisions" option. These will then appear in a new tab in the revision summary panel called "Stack", so that it's possible to get a quick overview of where things are.

Regarding the overall output appearance, it seems to me like you should have the JSON for a file formatted as a single JSON object, with each output variety in its own child object, labelled according to the option name or similar. Since llvm-readobj and llvm-readelf also both support multiple input files, you need a strategy for how these might be dumped. My instinct says that you actually have the dump output for a file added as child members to a top-level object, labelled by their file names. Finally, you should give some thought to how archive members are printed. I think you could either have them printed as individual objects within a JSON object representing the whole archive, or just as regular files, but with the names disambiguated. I've gone with the latter approach below:

  {
      "object.o" : {
          "file-header" : { <output for file header> },
          "section-headers" : { <section header table> }
      },
      "archive.a(member1.o)": {
          "file-header" : { <output for file header> },
          "section-headers" : { <section header table> }
      },
      "archive.a(member2.o)": {
          "file-header" : { <output for file header> },
          "section-headers" : { <section header table> }
      }
  }

I hope the reasoning for this is clear, but if you've got other ideas, I'm happy to discuss them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111658



More information about the llvm-commits mailing list