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

[Joseph Huber via Phabricator via llvm-commits]

jhuber6 added inline comments.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:38
+
+static Error extractAllBinaries(const OffloadBinary *OB) {
+  uint64_t Offset = 0;
----------------
tra wrote:
> `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. 
> 
> 
How about this?
```
printOffloadBinary()
printOffloadBinaries()
dumpOffloadSections()
dumpOffloadBinaries()
```


================
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) {
----------------
tra wrote:
> 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.
Yeah, I meant it more like to print on the single file that was already extracted or something. But it can definitely contain multiple. The reason I chose this method is because I wanted something that worked even if these sections were concatenated through a relocatable link or something. So whenever we parse one of these we just check the sizes to make sure there's not another one concatenated to it. I can make the comment less confusing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904