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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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

> Do you consider these blocking issues beyond fixing the typos and adding comments?

The missing test case is the issue that I care about most.

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

With all due respect to the others who have signed off on this patch already, they aren't routine contributors to this area of the LLVM code-base, so don't know the norms, expected code quality etc of it. Please understand that I am just trying to keep the quality as high as possible. This often means that things have to go through several iterations until they are right. Please also be aware that this patch is not the only patch I am reviewing and I have to balance my regular responsibilities alongside these reviews too, so I don't have time to do a back-and-forth on the patch multiple times a day. I'm certainly not blocking this for the sake of doing so, especially given the time I've spent reviewing the patch.

In D126904#3624693 <https://reviews.llvm.org/D126904#3624693>, @JonChesterfield wrote:

> I'm absolutely OK with the high volume of minor comments going unaddressed, especially since they're being added incrementally over a prolonged period of time.

Most of these comments are as a result of changes in each iteration, where the later iteration either doesn't fully resolve the point I had raised, or introduced other issues. It takes time to get things right. I'd also point out that some inline comments were left without responses for several iterations of the patch, which is part of the reason for this taking this long.

> I'm also OK with James doing some post commit cleanup to make things better conform to his ideal.

This isn't how reviews work. It's not my responsibility to address issues introduced with a patch that someone else has just landed. If you think it is, please raise this on the LLVM forums and see what others have to say.



================
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
+
----------------
jhuber6 wrote:
> 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.
My thinking was that it would be useful, to show that good binaries are dumped despite the warning. The question is really, do you consider it a guarantee that all good binaries will be dumped even if a later one is bad, and if not, will users be concerned if the behaviour ever changed (by accident or otherwise) to check all the binaries are good before dumping any of them?


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:78
+    if (!Contents)
+      reportError(Contents.takeError(), O->getFileName());
+
----------------
jhuber6 wrote:
> 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.
Imagine if the code were changed to the following by somebody:
```
    if (!Contents)
      reportError("failed to get offloading section contents", O->getFileName());
```
There would be no test failure under any situation, because there is no test. You //would// get a crash though if someone were to try to use llvm-objdump with the enhanced error checks enabled, and ran into a malformed binary.

This isn't really a contrived example either: in an earlier revision of this patch, there was a similar situation, where the error was thrown away without getting its message, so I don't think it's unreasonable to assume that it could occur in a later revision of this code.

I'm not asking for testing that errors are reported when getting malformed contents, I'm asking for testing that the error returned by the lower-level function is handled by this higher-level one.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:60-62
+    Expected<StringRef> Name = Sec.getName();
+    if (!Name || !Name->startswith(OFFLOAD_SECTION_MAGIC_STR))
+      continue;
----------------
jhuber6 wrote:
> 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.
Okay.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904