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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Sorry for the delay - I've had a lot on my plate.

It seems like most (all?) of my last round of inline comments haven't been addressed?



================
Comment at: llvm/test/tools/llvm-objdump/Offloading/Inputs/binary.yaml:31
+      Value:          "sm_70"
+
----------------
Nit: Looks like one too many blank lines at EOF.


================
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={{.}}
----------------
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.".


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


================
Comment at: llvm/test/tools/llvm-objdump/Offloading/binary.test:16
+# ELF-EMPTY:
+# CHECK:OFFLOADING IMAGE [0]:
+# CHECK-NEXT:kind            llvm ir
----------------
Nit: by adding this suggested spacing, your output then all lines line up, which I think improves test readability.


================
Comment at: llvm/test/tools/llvm-objdump/Offloading/failure.test:17
+
+# CHECK: Invalid data was encountered while parsing the file
----------------
This doesn't tell us whether the message is a warning or error, so please include the "warning:" or "error:" prefix in the check.

Additionally, the error message as checked doesn't give sufficient context as to what has gone wrong. For example, it doesn't mention the input file or that it is the offloading code that has failed. See https://llvm.org/docs/CodingStandards.html#error-and-warning-messages. A couple of options include modifying the underlying code to give better error messages, or to "catch" the error and rewrap it with some additional context in the message. llvm-readobj has a number of examples of where this is done that I know of, for example (llvm-objdump may do too).

Finally, in dumping tools, especially in newer code, we try to avoid having hard errors. The reason for this is because it is often useful to be able to see the reainder of the output from other files and/or options, whereas reporting an error is usually a hard end to the program (NB: I haven't double-checked the llvm-objdump behaviour to confirm that the error reporting ends the program, so apologies if this is a bit of misdirection).

I made the second and third of these points in previous inline comments, but they don't seem to have been addressed.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:20
+
+#define OFFLOAD_SECTION_MAGIC_STR ".llvm.offloading"
+
----------------
Why is this a macro rather than just a `static const char *`?


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:71
+void llvm::dumpOffloadBinary(const ObjectFile *O) {
+  for (const SectionRef &Sec : O->sections()) {
+    Expected<StringRef> Name = Sec.getName();
----------------
Nit: `SectionRef` is designed to be lightweight and copyable (like `llvm::StringRef`) so there's no particular need to use `const &` here.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:78
+    if (!Contents)
+      reportError(Contents.takeError(), O->getFileName());
+
----------------
You should exercise this error path by adding a `SHOffset` key to your ELF YAML with an invalid value.


================
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:
> 
Not addressed yet.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:35-36
+    return "ptx";
+  default:
+    return "<None>";
+  }
----------------
jhenderson wrote:
> jhuber6 wrote:
> > jhenderson wrote:
> > > jhenderson wrote:
> > > > I think it would be sensible to test the default case (it's a common behaviour pattern for the default case to result in an error, so it seems sensible to demonstrate that it isn't in this case).
> > > This comment was marked as done, but I don't see a test case for it?
> > Because I added some validation to `obj2yaml` it became impossible to test it by creating one without a valid value, but I'm going to get rid of that to simplify this and add it back in.
> As a general rule, yaml2obj should be as lax as possible in what it allows, as it allows testing these corner cases.
With the changes made to yaml2obj, can we now test this default case?


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904