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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D126904#3624691 <https://reviews.llvm.org/D126904#3624691>, @jhuber6 wrote:

> The only comments I'm aware of that I haven't addressed is early exiting on failure and a check on failing to get the section. I think if we truly want to avoid early exits we should put it in a separate patch, I'm not doing anything that the rest of `llvm-objdump` doesn't already do. I do not think it's necessary to add an error check if we fail to get a section, this is tested already in many places and I see no point to add a completely redundant test to the LLVM test-suite. I can remove the quotation marks prior to landing if you want. If you have any further objections let me know, otherwise I would greatly appreciate being able to move forward with this.

I've gone through and highlighted all the inline comments that still need addressing/answering. I can live with the early exiting in this patch, but still think you need the test I've requested (see inline for explanation).

I'm still not really comfortable with relying on section name to identify the offloading binaries, as it could potentially require tens or even hundreds of thousands of string comparisons under some cases, plus it's not really in the spirit of the ELF file format*. It's also worth noting that for other platforms, the naming schemes are different - for example in Mach-O files, section names are usually `__some_section_name` instead of `.some_section_name`, so you'll still have to have at least some platform-specific code in order to retrieve the section. Most (all?) new sections that have any dumping support are distinguished by type for ELF, although curiously, dumping support has only been added for them in llvm-readobj rather than llvm-objdump, so it's not a clear-cut point.

(* The ELF gABI states about the sh_type field: "This member categorizes the section's contents and semantics." and then defines the SHT_PROGBITS type (which I assume the current offloading binaries are) as "The section holds information defined by the program, whose format and meaning are determined solely by the program." Given that the offloading format is not specific to the program, it seems incorrect to say that it is SHT_PROGBITS.)



================
Comment at: llvm/test/tools/llvm-objdump/Offloading/binary.test:1
+# RUN: yaml2obj %S/Inputs/binary.yaml -o %t
+# RUN: llvm-objdump --offloading %t | FileCheck %s --match-full-lines --strict-whitespace --implicit-check-not={{.}}
----------------
jhenderson wrote:
> jhenderson wrote:
> > In line 3, you run yaml2obj to create the .bin file again. I suggest you simply rename this %t to %t.bin and then delete the later yaml2obj invocation.
> > 
> > It'd probably be useful to have a brief comment at the start of this individual test case, e.g. "Show can dump offloading binaries directly." and an equivalent one at the start of the wrapped-in-ELF case, e.g. "Show can dump offloading binaries embedded in ELF.".
> > It'd probably be useful to have a brief comment at the start of this individual test case, e.g. "Show can dump offloading binaries directly." and an equivalent one at the start of the wrapped-in-ELF case, e.g. "Show can dump offloading binaries embedded in ELF.".
> 
> Looks like this bit hasn't been addressed?
Not addressed.


================
Comment at: llvm/test/tools/llvm-objdump/Offloading/binary.test:4
+# RUN: yaml2obj %S/Inputs/binary.yaml -o %t.bin
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy --add-section .llvm.offloading=%t.bin %t
----------------
jhenderson wrote:
> jhenderson wrote:
> > Nit: Perhaps worth renaming %t to %t.elf for this test case to help make it clearer what this code is doing in contrast to the previous case.
> > 
> > Also, I'd add a single blank line (followed by the suggested comment above) between the lines to do with the raw binary and ELF cases.
> Again, the second half of this comment hasn't been addressed.
Not addressed.


================
Comment at: llvm/test/tools/llvm-objdump/Offloading/warning.test:6
+# RUN: llvm-objcopy --add-section .llvm.offloading=%t-good.bin %t.elf
+# RUN: llvm-objdump --offloading %t.elf 2>&1 | FileCheck %s
+
----------------
jhenderson wrote:
> Is it worth checking that the good binary was dumped successfully?
Not addressed.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:78
+    if (!Contents)
+      reportError(Contents.takeError(), O->getFileName());
+
----------------
jhenderson wrote:
> jhenderson wrote:
> > You should exercise this error path by adding a `SHOffset` key to your ELF YAML with an invalid value.
> Marked as done but I don't see it?
The reason this needs addressing, is because this specific code path in llvm-objdump is otherwise untested. The test is needed to show that llvm-objdump //under these specific circumstances// properly handles errors if `Contents` is in an error state. This is actually important because if there is no such test, a change to the `reportError` whereby `Contents` isn't used in the message, could result in unchecked errors, but without a test case, these would only manifest under real usage, rather than under testing like they should be.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:86
+
+    // Print out all the binaries that are contained at this buffer. We want to
+    // visit each offloading binary we can find, so failing to find one is not
----------------
jhenderson wrote:
> jhenderson wrote:
> > 
> Not addressed yet.
Not addressed.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:60-62
+    Expected<StringRef> Name = Sec.getName();
+    if (!Name || !Name->startswith(OFFLOAD_SECTION_MAGIC_STR))
+      continue;
----------------
jhenderson wrote:
> jhenderson wrote:
> > jhuber6 wrote:
> > > jhenderson wrote:
> > > > It's extremely unfortunate that this relies on section names rather than section types. In my opinion, it would be far more appropriate to have a SHT_LLVM_OFFLOAD section type (or similar), so that section names don't need looking up and comparing to find the relevant section. ELF is designed really to use types for section comparison, not really names.
> > > I wanted to keep it somewhat generic if we ever want to get this to work on COFF / MACH-O and names were the easiest solution at the time. I can try to make a change for that in the future, it would definitely be a better solution than checking a magic string.
> > I'm not familiar with Mach-O, although I know that COFF does rely on strings, but it's quite normal to vary this somewhat between file formats, due to the different features available in those formats.
> > 
> > Changing to a specific type for ELF may be a prerequisite for a proper yaml2obj implementation anyway, so that yaml2obj knows how to parse the YAML for such sections.
> Could I clarify when you said making a change in the future for the name -> type bit that you mean in a future patch?
Not answered.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904