[PATCH] D136796: [llvm-objdump][Offload] Use common offload extraction method

[James Henderson via Phabricator via cfe-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/Offloading/elf_dynamic.test:1
-## Check that we can dump an offloading binary directly.
-# RUN: yaml2obj %S/Inputs/binary.yaml -o %t.bin
-# RUN: llvm-objdump --offloading %t.bin | FileCheck %s --match-full-lines --strict-whitespace --implicit-check-not={{.}}
-
 ## Check that we can dump an offloading binary inside of an ELF section.
+# RUN: yaml2obj %S/Inputs/binary.yaml -o %t.bin
----------------
Rather than duplicating this and the ET_REL cases, you can use yaml2obj's -D option to parameterise the ELF type. Rough code:

```
# RUN: yaml2obj %S/Inputs/binary.yaml -D TYPE=ET_DYN -o %t.so
# RUN: yaml2obj %S/Inputs/binary.yaml -D TYPE=ET_EXEC -o %t.bin
# RUN: yaml2obj %S/Inputs/binary.yaml -D TYPE=ET_REL -o %t.o

...
  Data: ELFDATA2LSB
  Type: [[TYPE]]
...
```


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:63
 
-    // Print out all the binaries that are contained in this buffer. If we fail
-    // to parse a binary before reaching the end of the buffer emit a warning.
-    if (Error Err = visitAllBinaries(Binary))
-      reportWarning("while parsing offloading files: " +
-                        toString(std::move(Err)),
-                    O.getFileName());
-  }
+  // Print out all the binaries that are contained at this buffer.
+  for (uint64_t I = 0, E = Binaries.size(); I != E; ++I)
----------------
Not sure why this was changed.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:76
+
+  // Print out all the binaries that are contained at this buffer.
+  for (uint64_t I = 0, E = Binaries.size(); I != E; ++I)
----------------
Should this be "contained in this buffer" rather than "at"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136796