Page MenuHomePhabricator

[llvm-objdump] Print the DEBUG type under `--section-headers`.
ClosedPublic

Authored by Esme on May 17 2021, 2:58 AM.

Details

Summary

Under the option --section-headers, we can only print the section types of TEXT, DATA, and BSS for now. This patch adds the DEBUG type.

Diff Detail

Event Timeline

Esme created this revision.May 17 2021, 2:58 AM
Esme requested review of this revision.May 17 2021, 2:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2021, 2:58 AM
Higuoxing added inline comments.May 17 2021, 7:56 PM
llvm/test/tools/llvm-objdump/ELF/PowerPC/section-headers-debug.test
30 ↗(On Diff #345798)

You can specify the 'Size' field of the section rather than the 'Content' field, e.g.,

- Name:   .data
  Type:   SHT_PROGBITS
  Flags:  [ SHF_WRITE, SHF_ALLOC ]
  Size:   0x10
43 ↗(On Diff #345798)

Nit: remove the blank line.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1771

I think we need some tests to check the output when the section has multiple types, e.g., 'DATA' and 'DEBUG'.

jhenderson added inline comments.May 18 2021, 1:04 AM
llvm/test/tools/llvm-objdump/ELF/PowerPC/section-headers-debug.test
1 ↗(On Diff #345798)

Rather than having a test dedicated to a specific type field, it would make more sense to have a test that tests all type fields, probably as part of a generic --section-headers test. As such, before you add the DEBUG type, I'd recommend writing a new test/expanding an existing test for --section-headers, to test that option thoroughly (a quick skim of test/tools/llvm-objdump suggests there is no such test there, but there is objdump-sectionheaders.test in llvm/test/Object, which should probably be moved). This would be a separate patch, prior to this one.

Another point: the behaviour is not specific to PowerPC, and the test doesn't rely on the PowerPC target being available, so move the test into the ELF directory.

llvm/test/tools/llvm-objdump/XCOFF/xcoff-section-headers-debug.test
1 ↗(On Diff #345798)

Similar comments as above. Don't have a dedicated test for the DEBUG type. Just expand the existing test.

Also, it would be preferable to use yaml2obj for this testing in the first place, rather than a canned binary, so rebase this patch on top of the related patches and wait for those to land.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1771

+1 - also I think this code is actually slightly incompatible with GNU output, which uses comma separators. It should be fixed to match.

Esme updated this revision to Diff 347569.May 24 2021, 9:20 PM

Address comments.

Esme added a comment.May 24 2021, 9:20 PM
This comment was removed by Esme.
llvm/test/tools/llvm-objdump/ELF/PowerPC/section-headers-debug.test
1 ↗(On Diff #345798)

There is generic test, llvm/test/tools/llvm-objdump/section-headers.test, so I just expanding it with adding the test for type fields.

Esme updated this revision to Diff 347578.May 24 2021, 9:52 PM

Format.

Higuoxing added inline comments.May 24 2021, 11:10 PM
llvm/test/tools/llvm-objdump/XCOFF/section-headers.test
1–5 ↗(On Diff #347578)

It looks that xcoff-section-headers.o and xcoff-long-sec-names.o are useless after this change. Can we remove them?

9–17 ↗(On Diff #347578)

Please insert a blank space between # and TYPE-NEXT.

Esme updated this revision to Diff 347588.May 24 2021, 11:25 PM

Address comment.

llvm/test/tools/llvm-objdump/XCOFF/section-headers.test
1–5 ↗(On Diff #347578)

xcoff-long-sec-names.o is useless, but xcoff-section-headers.o is used by other tests. Those tests may be replaced by yaml-docs in the future, which should be another work.

Seems to me like this is two separate patches now: 1) adding generic DEBUG printing support to llvm-objdump, then 2) adding XCOFF recognition of debug section types.

llvm/test/tools/llvm-objdump/XCOFF/section-headers-truncate.test
2 ↗(On Diff #347588)

You can avoid a new canned binary here by using python to truncate another file. See for example llvm/test/tools/llvm-objcopy/ELF/invalid-e_phoff.test for an example of how to do this.

llvm/test/tools/llvm-objdump/XCOFF/section-headers.test
24 ↗(On Diff #347588)

We probably need to add addresses to some of these too. If memory serves correctly, ELF yaml2obj auto-increments addresses for allocatable sections, if an adderss is not explicitly specified. Perhaps a similar feature could be added to XCOFF yaml2obj in a separate patch? What do you think? In the meantime, the addresses can presumably be done manually here.

llvm/test/tools/llvm-objdump/section-headers.test
154–155

I don't think we need an entirely new dedicated test case here. I think you would be better off expanding the first yaml doc in this file to cover the new DEBUG type and a case with multiple types (to pick up the separator change), and then just updating the first set of checks.

Esme updated this revision to Diff 347616.May 25 2021, 2:52 AM
Esme added a comment.May 25 2021, 2:53 AM

Seems to me like this is two separate patches now: 1) adding generic DEBUG printing support to llvm-objdump, then 2) adding XCOFF recognition of debug section types.

Split the patch and will post the second patch for XCOFF soon.

llvm/test/tools/llvm-objdump/XCOFF/section-headers.test
24 ↗(On Diff #347588)

Perhaps a similar feature could be added to XCOFF yaml2obj in a separate patch?

Sounds good to me. I will implement the feature for XCOFF yaml2obj in a follow-up work.

jhenderson accepted this revision.May 25 2021, 3:16 AM

LGTM, thanks!

This revision is now accepted and ready to land.May 25 2021, 3:16 AM
Higuoxing accepted this revision.May 25 2021, 3:26 AM
This revision was landed with ongoing or failed builds.May 26 2021, 9:54 PM
This revision was automatically updated to reflect the committed changes.