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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/Offloading/binary.test:2
+# RUN: yaml2obj %S/Inputs/binary.yaml -o %t
+# RUN: llvm-objdump --offloading %t | FileCheck %s --strict-whitespace --implicit-check-not={{.}}
+
----------------
You need `--match-full-lines` too to ensure the whitespace indentation is strictly enforced at the start and end of the lines too. This will require you to reformat your check patterns slightly, because the space after the ":" is then a part of the pattern:
```
#      CHECK:OFFLOADING IMAGE [0]:
# CHECK-NEXT:kind            llvm ir
```
etc (I've added extra whitespace in the `# CHECK:` directive, to make the colons line up nicely, enhancing the readability and emphasising the indentation.


================
Comment at: llvm/test/tools/llvm-objdump/Offloading/elf.test:1
+# RUN: yaml2obj %S/Inputs/binary.yaml -o %t.bin
+# RUN: yaml2obj %s -o %t
----------------
Now that I'm looking at this and the other test case, it seems like you'd be better off having them in the same file, because the CHECK-* patterns are identical. That would presumably still apply after adding COFF etc support.


================
Comment at: llvm/test/tools/llvm-objdump/Offloading/elf.test:4
+# RUN: llvm-objcopy --add-section .llvm.offloading=%t.bin %t
+# RUN: llvm-objdump --offloading %t | FileCheck %s --strict-whitespace --implicit-check-not={{.}}
+
----------------
Same comment as above.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:86-87
+
+    // Print out all the binaries that could be contained here. If we fail to
+    // find one after the first, just give up instead of throwing an error.
+    if (Error Err = visitAllBinaries(&Binary))
----------------
The question this comment needs to answer is WHY we should give up, rather than reporting the error (as a warning) and ending printing? Same goes below.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:35-36
+    return "ptx";
+  default:
+    return "<None>";
+  }
----------------
jhenderson wrote:
> I think it would be sensible to test the default case (it's a common behaviour pattern for the default case to result in an error, so it seems sensible to demonstrate that it isn't in this case).
This comment was marked as done, but I don't see a test case for it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904