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

[Artem Belevich via Phabricator via llvm-commits]

tra added a comment.

> This patch only supports printing these sections, later we will want to 
> support dumping files the user may be interested in via another flag. I
> am unsure if this should go here in llvm-objdump or llvm-objcopy.

`objcopy -O binary --only-section=.text` seems to be the closest to the file extraction.



================
Comment at: llvm/tools/llvm-objdump/ObjdumpOpts.td:84
 
+def offloading : Flag<["--"], "offload">,
+  HelpText<"Display the content of the offloading section">;
----------------
`def offload`? The name of the option usually matches its spelling.


================
Comment at: llvm/tools/llvm-objdump/ObjdumpOpts.td:86
+  HelpText<"Display the content of the offloading section">;
+def : Flag<["-"], "O">, Alias<offloading>,
+  HelpText<"Alias for --offload">;
----------------
Single-letter options are a  limited resource. I'd rather not create a new one unless there's a specific need for it (e.g. compatibility with existing tool).
Using it for a niche option like `--offload` does not seem to be a good use for it, however convenient that may be for the few of us who may care.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:28
+
+/// Recursively walk the offloading binary to extract all the members. When we
+/// create an offloading object we only parse the first entry we find, if there
----------------
Nit: I would prefer to split it into a function or lambda doing a printout for one binary and the main function which does the iteration over the container of those binaries. 
While recursion is cool, and will likely be optimized into a loop in this case, it makes the code a bit harder to read and understand.

In this case things are simple enough so it does not make too much of a difference, so I'll leave it up to you.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2814
     ArchiveHeaders = FileHeaders = PrivateHeaders = Relocations =
         SectionHeaders = SymbolTable = true;
 
----------------
Should `Offloading` be added here to be included into output when `--all-headers` is in effect?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904