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

[Joseph Huber via Phabricator via llvm-commits]

jhuber6 added a comment.

In D126904#3554819 <https://reviews.llvm.org/D126904#3554819>, @tra wrote:

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

Yeah, I could probably make a new option that dumps the contents specified by the index. Users can then just write that to a file via `| xxd -r` or something.



================
Comment at: llvm/tools/llvm-objdump/ObjdumpOpts.td:84
 
+def offloading : Flag<["--"], "offload">,
+  HelpText<"Display the content of the offloading section">;
----------------
tra wrote:
> `def offload`? The name of the option usually matches its spelling.
I changed one but not the other, I'll fix it.


================
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">;
----------------
tra wrote:
> 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.
I would definitely like to have a nice single-letter shorthand for this, but you're right that it's a somewhat niche option. Most of the other dumping options have a single letter alias, but someone may want the `-O` in the future, or another tool introduces a `-O` and we can't be compatible. It'd be nice to get some other opinions on this, but I'm not super attached.


================
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
----------------
tra wrote:
> 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.
Fair enough, I pretty much just made the function to print it out and realized that I could just keep calling it. Should be easy enough to split out.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2814
     ArchiveHeaders = FileHeaders = PrivateHeaders = Relocations =
         SectionHeaders = SymbolTable = true;
 
----------------
tra wrote:
> Should `Offloading` be added here to be included into output when `--all-headers` is in effect?
I'm not sure, I would say no since this is more for all the `ELF` contents and the offloading stuff is simply data contained in there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904