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

[Joseph Huber via Phabricator via llvm-commits]

jhuber6 marked 4 inline comments as done.
jhuber6 added a comment.

Thanks for the review, I'll try to address your comments soon.



================
Comment at: llvm/test/tools/llvm-objdump/Offload/offload.test:1-2
+# RUN: llvm-objdump --offloading %S/Inputs/offload-binary.out | FileCheck %s --check-prefix=OFFLOAD
+# RUN: llvm-objdump --offloading %S/Inputs/offload-elf.o | FileCheck %s --check-prefix=OFFLOAD
+
----------------
jhenderson wrote:
> As this is a new output format, we probably want to use the FileCheck options `--match-full-lines --strict-whitespace --implicit-check-not={{.}}` to ensure the entire output (including indentation etc) is checked-for and there is nothing additional being printed that shouldn't be.
> 
> As there's only one check pattern used in the file, drop the `--check-prefix` argument and just use `CHECK:`, `CHECK-NEXT:` etc.
> 
> Using pre-canned binaries is less than ideal. Can you use yaml2obj to generate the test inputs at runtime? This will make them less opaque and easier to maintain long-term. Depending on the section's complexity, it probably makes sense to add support for it to yaml2obj explicitly, so that it's easy to specify kind/arch/triple/producer etc in dedicated fields.
I'll adjust the tests. I could try to make a yaml2ojb implementation for this. I already have a tool that creates these but it lives in Clang. Would implementing the yaml2obj go in a separate patch?


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


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:66
+    if (!Contents)
+      reportError(Contents.takeError(), O->getFileName());
+
----------------
jhenderson wrote:
> Error path test?
Since I needed to use prepackaged binaries it was a little hard to make the test cases contain errors. This path specifically is just for an ELF that's broken somehow and can't get the section contents so I don't think it's relevant to the feature.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:71
+    if (!BinaryOrErr)
+      reportError(BinaryOrErr.takeError(), O->getFileName());
+    OffloadBinary &Binary = **BinaryOrErr;
----------------
jhenderson wrote:
> Error path test?
This basically only happens if the section doesn't have the necessary magic bytes or is too small, I'll try to add a test for that.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:75
+    if (Error Err = visitAllBinaries(&Binary))
+      handleAllErrors(std::move(Err), [](const ErrorInfoBase &EI) {});
+  }
----------------
jhenderson wrote:
> This seems to be just throwing away all errors. Are you sure that's what you meant to do, and not to report them with `reportError`? If so, did you mean to use `consumeError` (or possibly even `cantFail`)?
Those are probably better options, thanks. Yes, throwing away the errors is the intended solution here. These binaries are individual files stored in a big blob, when we parse one we check the rest of the buffer to see if there are more. I did it this way so when sections get merged via `ld -r foo.o bar.o` we can still find the files. It leads to a little weirdness with parsing however. Basically I only check errors for the first file in the section, if we fail to find any after that one we shouldn't treat it as an error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904