[PATCH] D129053: [llvm-objdump] Update offload dumping to use SHT_LLVM_OFFLOADING

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: llvm/test/tools/llvm-objdump/Offloading/binary.test:7
 # RUN: yaml2obj %s -o %t.elf
-# RUN: llvm-objcopy --add-section .llvm.offloading=%t.bin %t.elf
-# RUN: llvm-objcopy --set-section-alignment .llvm.offloading=8 %t.elf
+# RUN: llvm-objcopy --update-section .llvm.offloading=%t.bin %t.elf
 # RUN: llvm-objdump --offloading %t.elf | FileCheck %s --check-prefixes=CHECK,ELF --match-full-lines --strict-whitespace --implicit-check-not={{.}}
----------------
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).


================
Comment at: llvm/test/tools/llvm-objdump/Offloading/non-elf.test:14
+
+# CHECK: warning: '[[FILENAME]]': offloading is currently only supported for ELF targets
----------------
I'd suggest this slight tweak to the message.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:21
 
 constexpr const char OffloadSectionString[] = ".llvm.offloading";
 
----------------
Can you get rid of this now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129053