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

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:38
+
+static Error extractAllBinaries(const OffloadBinary *OB) {
+  uint64_t Offset = 0;
----------------
`extract` is not quite what we're doing here.  `printAllBinaries` ? But then we already have `printOffloadBinary` below, so it's a bit confusing.

IIUIC the hierarchy looks roughly like this:
-  Object file
   - N offload sections
      - M offload binaries

So, this function prints content of one section (i.e. a collection of offload binaries), while the function below prints contents of the object file (i.e. a collection of offload sections) and the names currently do not reflect that. 




================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:78
+
+/// Print the contents of a single offload binary file \p OB.
+void llvm::printOffloadBinary(const OffloadBinary *OB) {
----------------
I don't think the 'single' part of this assertion is true. AFAICT, `extractAllBinaries` will happily print all subsequent binaries if it finds them in the buffer. I think this should call `printBinary` instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904