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

[Joseph Huber via Phabricator via llvm-commits]

jhuber6 marked 3 inline comments as done.
jhuber6 added a comment.

Thanks for the comments, I'll update the ObjectYAML implementation and propagate it to here.



================
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))
----------------
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.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:35-36
+    return "ptx";
+  default:
+    return "<None>";
+  }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904