[PATCH] D126904: [llvm-objdump] Add support for dumping embedded offloading data

[James Henderson via Phabricator via llvm-commits]

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

Looks good now, with a couple of nits to be addressed prior to being committed, and a slight change to a test.



================
Comment at: llvm/test/tools/llvm-objdump/Offloading/binary.test:1
+# Check to see that we can dump an offloading binary directly.
+# RUN: yaml2obj %S/Inputs/binary.yaml -o %t.bin
----------------
Nit: in newer tests in the binary tools at least, we use `##` for test comments to help them stand out from the RUN and CHECK directives. Same throughout the new comments.


================
Comment at: llvm/test/tools/llvm-objdump/Offloading/binary.test:5
+
+# Check to see that we can dump an offloading binary insode of an ELF section.
+# RUN: yaml2obj %s -o %t.elf
----------------
Ditto. Also typo: "insode" -> "inside".


================
Comment at: llvm/test/tools/llvm-objdump/Offloading/warning.test:7-8
+# RUN: llvm-objcopy --add-section .llvm.offloading=%t-good.bin %t.elf
+# RUN: llvm-objdump --offloading %t.elf | FileCheck %s -DFILENAME=%t.elf
+# RUN: llvm-objdump --offloading %t.elf 2>&1 | FileCheck %s --check-prefix=CHECK-WARN -DFILENAME=%t.elf
+
----------------
These should be a single invocation and just check that the warning appears in the right place in the output with respect to the regular output. Sorry if that wasn't clear from my earlier comments.

If you want, you can also abbreviate your other checks to e.g. just the `OFFLOADING IMAGE` line - as you rightly point out, we test that the dumping works properly elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904