[llvm] [llvm-readobj][COFF] Add JSON Output Style (PR #95074)

[James Henderson via llvm-commits]

jh7370 wrote:

> > Should we deprecate the existing --elf-output-style option in favour of a new generic --output-style option?
> 
> I considered this option and I opted against it to keep the changes localized to `COFF` for the sake of the PR. I'd be happy to make this change as a separate PR per your suggestion. I'd add we probably also need to handle `MachO`. I think simply reporting that the style is unsupported (ie. `gnu`) is a safer choice. The same type of error reporting would likely fit well when considering `MachO` until someone needs it implemented there.

My only concern with an error in the event of invalid output style options is if someone provides input files of different formats on the command-line they might get an error. For JSON output, this might make sense, but for other output styles it probably doesn't. If memory serves me rightly, we don't warn or error for --elf-output-style + COFF, for example.

As an aside, I don't think we should drop `--elf-output-style` and instead should just mark it as deprecated in the user guide and release note: there are too many tests out there using it and it's likely to be reasonably widespread in downstream consumers too, so removing it causes a lot of churn with only minimal gain.

> > I think this PR needs splitting into multiple individual ones.
> 
> I agree. I wasn't sure what the process is to merge in features piecewise before things are fully complete. An alternative would be for me to split the commits in this PR following the same suggestions you proposed. I'm happy to go with either approach.

GitHub doesn't support stacked/chained PRs well when the branches aren't in the same repository as the actual code being merged into, and LLVM doesn't allow for user-created branches in its repository. There's been a lot of conversation about it (search for "llvm stacked pr") and nobody really came up with a great solution. The best one going at the moment is the use of `spr`. I'd suggest you look into that (somewhere there was supposed to be some documentation on this with regards to LLVM's usage, but I wasn't able to find it when searching; @MaskRay may be able to help).

In essence, LLVM has a "one PR per final commit in the repository" policy, with the idea being that you review the PR as a whole, rather than individual commits within the PR. This then allows for easier updates to the PR with subsequent fixup commits, rather than constant rebases (note that rebasing and force pushes mess things up so that it's no longer possible to "view changes since last review", which is an important part of a review workflow; prefer merging in changes from main etc over rebasing). Spr seems to work reasonably well with this workflow and stacked PRs too.

> > The testing for ELF JSON output is mostly in the same files as the other output styles. I'm not sure why file-summary-json.test isn't, but it seems to be the exception rather than the norm.
> 
> Fair enough. This shouldn't be a problem to change. I can additionally update `file-summary-json.test` in a separate PR to make it consistent if you think it prudent.

No particular need to change `file-summary-json.test`. It's possible there's a good reason for it to be different that I don't remember.

> > Finally, I'm sure there's a valid one, but could you outline your motivation for having JSON support for COFF files, please?
> 
> Of course. We're using `llvm-readobj` to analyze our build artifacts for various properties to make sure things match certain expectations (eg. which imports are being used, etc). Instead of writing a script to do string parsing on the `LLVM` style output, extending the tool to be able to output JSON makes the process of scripting the output a much more simple/robust process.

Thanks, makes sense.

> Also, not sure what the process is, but while we have these discussions, I figured I should change the PR to a draft. Let me know if you prefer something different.

I don't think there's a formal policy, but in general, I treat "Draft" PRs as "not ready for review" unless somebody specifically asks me to take a look at it. I suspect I'm not the only one either. I have this vague memory that subscribers don't receive notifications when a draft PR is updated either.

https://github.com/llvm/llvm-project/pull/95074