This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump] Update offload dumping to use SHT_LLVM_OFFLOADING
ClosedPublic

Authored by jhuber6 on Jul 3 2022, 3:56 PM.

Details

Summary

In order to be more in-line with ELF semantics, a previous patch added
support for a new ELF section type to indicate if a section contains
offloading data. This allows us to now check using this rather than
checking the section name directly. This patch updates the logic to
check the type now instead.

I chose to make this emit a warning if the input is not an ELF-object
file. I could have made the logic fall-back to the section name, but
this offloading in LLVM is currently not supported on any other targets
so it's probably best to emit a warning until we improve support.

Depends on D129052

Diff Detail

Event Timeline

jhuber6 created this revision.Jul 3 2022, 3:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2022, 3:56 PM
jhuber6 requested review of this revision.Jul 3 2022, 3:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2022, 3:56 PM
jhenderson accepted this revision.Jul 4 2022, 12:17 AM

LGTM, with a couple of suggested tweaks.

If you ever did add support to other object file formats, or you choose to add --offloading to llvm-readobj too, you might want to move the code to fetch the data from an ObjectFile into the ObjectFile interface somewhere, but that's not necessary now. Just something to consider for the future.

llvm/test/tools/llvm-objdump/Offloading/binary.test
7

FWIW, you might want to consider whether it would be useful for llvm-objcopy to be able to create an SHT_LLVM_OFFLOADING section using --set-section-flags, or even by a direct recognition of the name (I imagine it isn't worth it though).

llvm/test/tools/llvm-objdump/Offloading/non-elf.test
15

I'd suggest this slight tweak to the message.

llvm/tools/llvm-objdump/OffloadDump.cpp
20–21

Can you get rid of this now?

This revision is now accepted and ready to land.Jul 4 2022, 12:17 AM
jhuber6 updated this revision to Diff 442067.Jul 4 2022, 4:34 AM

Removing unused string and updating warning.

jhuber6 added inline comments.Jul 4 2022, 4:36 AM
llvm/test/tools/llvm-objdump/Offloading/binary.test
7

I was expecting llvm-objcopy to have some sort of --set-section-type functionality, maybe that could be added in.

jhenderson added inline comments.Jul 4 2022, 4:37 AM
llvm/test/tools/llvm-objdump/Offloading/binary.test
7

I don't think I'd be opposed to the idea - it seems much more flexible than the --set-section-flags option.

This revision was landed with ongoing or failed builds.Jul 7 2022, 9:21 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.Jul 7 2022, 3:44 PM
llvm/test/tools/llvm-objdump/Offloading/binary.test
7

I am preparing a patch to add --set-section-type to llvm-objcopy. There was a previous need changing .init_array.N to SHT_INIT_ARRAY. I worked around the issue by using GNU objcopy. Since there are similar needs, maybe time to add it.