[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:6
+# RUN: llvm-objcopy --add-section .llvm.offloading=%t.bin %t
+# RUN: llvm-objdump --offloading %t | FileCheck %s --check-prefix=CHECK --check-prefix=ELF --match-full-lines --strict-whitespace --implicit-check-not={{.}}
+
----------------
Nit: It's more common in newer tests to use --check-prefixes instead of multiple --check-prefix options.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:86
+
+    // Print out all the binaries that are contained at this buffer. We want to
+    // visit each offloading binary we can find, so failing to find one is not
----------------



================
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))
----------------
jhuber6 wrote:
> jhenderson wrote:
> > 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.
> I'll try to clarify it.
The clarification is clear enough, but it doesn't explain why you can't at least print a warning. This wouldn't prevent the code continuing.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:35-36
+    return "ptx";
+  default:
+    return "<None>";
+  }
----------------
jhuber6 wrote:
> jhenderson wrote:
> > 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?
> Because I added some validation to `obj2yaml` it became impossible to test it by creating one without a valid value, but I'm going to get rid of that to simplify this and add it back in.
As a general rule, yaml2obj should be as lax as possible in what it allows, as it allows testing these corner cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904