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

Fri Jul 1 05:32:56 PDT 2022

jhuber6 added a comment.

Do you consider these blocking issues beyond fixing the typos and adding comments? I'm getting very tired of needing to constantly update this patch that others have already signed off on and I'm beginning to feel like this is a fruitless endeavor. If you have no intention of ever letting this through let me know and I'll abandon it so we don't waste any more of our time.

================
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:
> > 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.
Sure, I'll add a comment.

================
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:
> > 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.
Sure I'll add a comment.

================
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:
> jhenderson wrote:
> > Is it worth checking that the good binary was dumped successfully?
> Not addressed.
No, this is the same test checks as the other file and it bloats the test. We already know that it will print the good one.

================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:78
+    if (!Contents)
+      reportError(Contents.takeError(), O->getFileName());
+
----------------
jhenderson wrote:
> 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.
There's already usage of `getContents()` like this in `llvm-objdump`, I don't see why this is a special case. If someone changed the `reportError` function to not report errors it should show up somewhere. The point of this patch is not about checking if the ELF works, and we know from similar usage in `llvm-objdump` that this pattern reports errors if it's malformed. I can add a test if you really want me t, but I fail to see the point even with your hypothetical situation.

================
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:
> > jhenderson wrote:
> > > 
> > Not addressed yet.
> Not addressed.
I'll fix the typo.

================
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:
> > 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.
Yes, I was saying we could do this in a future patch as I didn't think it was a blocking issue for the functionality of this patch. The main reason I did this is just because it was the easiest common solution between extracting these from LLVM-IR and an ELF, that is the section's string will be the same. I was just planning on adding it to the list of existing ones in `ELF.h`, but as this worked overall I thought it was sufficient to land this patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904