This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] Output valid JSON for GroupSections
ClosedPublic

Authored by paulkirth on Oct 31 2022, 10:57 AM.

Details

Summary

The current implementation output the LLVM formatted heading for group
sections, which was not valid JSON. This patch provides two small
customization points that allow the heading to vary between the two
implementations, and another that allows the section members to be
output as valid JSON objects.

Diff Detail

Event Timeline

paulkirth created this revision.Oct 31 2022, 10:57 AM
Herald added a project: Restricted Project. · View Herald Transcript
paulkirth requested review of this revision.Oct 31 2022, 10:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 10:57 AM

Please provide before and after examples, and test cases for the fix. Also, please fix the title to use "llvm-readobj" in the commit message/patch description, not simply "readobj".

jhenderson requested changes to this revision.Nov 1 2022, 1:31 AM
This revision now requires changes to proceed.Nov 1 2022, 1:31 AM
paulkirth updated this revision to Diff 483704.Dec 16 2022, 6:41 PM
paulkirth retitled this revision from [readobj] Output valid JSON for GroupSections to [llvm-readobj] Output valid JSON for GroupSections.
paulkirth edited the summary of this revision. (Show Details)

Rebase

  • avoid duplication in original patch
  • add tests to check the JSON and LLVM differences
paulkirth updated this revision to Diff 484088.Dec 19 2022, 2:49 PM

Rebase to fix patch application

Looks essentially good. Just a few nits and my ongoing comment about testing.

llvm/test/tools/llvm-readobj/ELF/llvm-vs-json-format.test
414

As in the other cases, I think you're better off moving this test case into the standard test case for printing group sections.

llvm/tools/llvm-readobj/ELFDumper.cpp
719–722

I would consider reordering the order these methods are in. Specifically, getGroupSectionHeaderName is used before the others, so it should be first, and printEmptyGroupMessage is used last, so it should be last in order.

Also applies to the definition order and other places where the function overrides are declared.

747–748
paulkirth updated this revision to Diff 502327.Mar 3 2023, 6:07 PM

Rebase. I had accidentally merged this into an earlier revision in the stack.

paulkirth updated this revision to Diff 502331.Mar 3 2023, 6:24 PM
paulkirth marked 2 inline comments as done.

Reorder methods.

jhenderson accepted this revision.Mar 6 2023, 12:57 AM

LGTM, with nit.

llvm/tools/llvm-readobj/ELFDumper.cpp
7683

Nit: don't have an extra blank line at EOF (the file should end with precisely one \n).

This revision is now accepted and ready to land.Mar 6 2023, 12:57 AM
paulkirth updated this revision to Diff 502699.Mar 6 2023, 10:21 AM
paulkirth marked an inline comment as done.

Remove newline at EOF.

paulkirth updated this revision to Diff 502707.Mar 6 2023, 10:32 AM

Fix formatting

paulkirth updated this revision to Diff 506255.Mar 17 2023, 6:58 PM

Rebase.

  • remove const qualifiers from StringRef
paulkirth updated this revision to Diff 506261.Mar 17 2023, 7:08 PM

Fix Rebase

paulkirth updated this revision to Diff 506262.Mar 17 2023, 7:09 PM

arc diff git merge-base HEAD origin --update D137095

This revision was landed with ongoing or failed builds.Mar 17 2023, 8:14 PM
This revision was automatically updated to reflect the committed changes.