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

Thu Jun 30 01:00:42 PDT 2022

jhenderson added inline comments.

================
Comment at: llvm/test/tools/llvm-objdump/Offloading/Inputs/malformed.yaml:1
+# RUN: yaml2obj %s | not obj2yaml 2>&1 | FileCheck %s
+!Offload
----------------
Is this file just meant to be the YAML? It's in an Inputs directory, so the RUN and CHECK lines aren't going to do anything...

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

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

================
Comment at: llvm/test/tools/llvm-objdump/Offloading/failure.test:17
+
+# CHECK: error: 'invalid data encountered while extracting offloading files'
----------------
Unrelated to my other comments, but why is this string in single quotes?

================
Comment at: llvm/test/tools/llvm-objdump/Offloading/failure.test:17
+
+# CHECK: Invalid data was encountered while parsing the file
----------------
jhuber6 wrote:
> jhenderson wrote:
> > 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.
> `ReportError` does indeed exit the program. There are a lot of other examples of `llvm-objdump` exiting on malformed input. I think it's reasonable to exit if the user requested `--offloading` and it's malformed to just exit. I will specify the checks and try to improve the message however.
A lot of those exits are from older code, but the general preference is to move away from the exit-immediately-on-malformed. Imagine the case where you have 4 different offloading binaries you wish to dump (e.g. `llvm-objdump --offloading 1.bin 2.bin 3.bin 4.bin`), and all 4 of these are malformed for different reasons. You'd end up getting an error on 1.bin and not knowing that the other 3 need fixing too, meaning you'd have to keep rerunning your code until you'd eventually flushed out all of the errors.

Similarly, imagine the binary was wrapped in an ELF, and you wanted to dump other parts of the ELF too. You'd end up only getting as far as the offloading dump before erroring, and not getting any other information you wanted.

================
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
+
----------------
Is it worth checking that the good binary was dumped successfully?

================
Comment at: llvm/test/tools/llvm-objdump/Offloading/warning.test:14
+
+# CHECK: warning: '[[FILENAME:.+]]': invalid data was found while parsing offloading files
+
----------------
You can use FileCheck's -D option to check the exact filename:

```
# RUN: ... | FileCheck -DFILE=%t.elf ...

# CHECK: warning: '[[FILE]]': ...
```

================
Comment at: llvm/test/tools/llvm-objdump/Offloading/warning.test:15
+# CHECK: warning: '[[FILENAME:.+]]': invalid data was found while parsing offloading files
+
----------------
Nit: too many blank lines at EOF.

================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:83
+    if (!BinaryOrErr)
+      reportError("invalid data encountered while extracting offloading files",
+                  O->getFileName());
----------------
I believe you'll end up with an assertion under the ABI breaking checks config (I think that's the name of it anyway), as you don't actually use the error within `BinaryOrErr`. I think it would still be good to include the message as reported by the underlying code, but wrapped in the additional context you've added. Also, would be good to include the input file name. Rough idea (uncertain on the exact invocation needed to get the string, as I'm too lazy to look it up right now!):
```
reportError("while extracting offloading files from \"" + O->getFileName() + "\": " + toString(BinaryOrErr));
```

================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:90-92
+      consumeError(std::move(Err));
+      reportWarning("invalid data was found while parsing offloading files",
+                    O->getFileName());
----------------
Same comment as above.

================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:102-106
+  if (Error Err = visitAllBinaries(OB)) {
+    consumeError(std::move(Err));
+    reportWarning("invalid data was found while parsing offloading files",
+                  OB->getFileName());
+  }
----------------
Ditto.

================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:78
+    if (!Contents)
+      reportError(Contents.takeError(), O->getFileName());
+
----------------
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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904