[PATCH] D126904: [llvm-objdump] Add support for dumping embedded offloading data
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 14 01:26:47 PDT 2022
jhenderson added a comment.
In D126904#3577758 <https://reviews.llvm.org/D126904#3577758>, @jhuber6 wrote:
> Addressing some comments for now. Let me know if this is sufficient for now, or if I should move to address the other main comments, the section identification scheme and using binary blobs.
I'd be concerned if a dumping implementation were written and then needed regular modification due to the section format not being stable yet. Usually, we'd try to get the details of teh section format sorted before writing the dumping implementation, to reduce churn.
> Improving the tests further would require an implementation of yaml2obj for these binaries. It would be nice to have but I'd need to figure out how to write it, if you have any materials on that it would help.
The only material I can offer is the yaml2obj source code - there are several other section kinds that have different dumping behaviour, that should be fairly straightforward to use as a basis for the implementation for this section. Take a look at how, for example, SHT_RELA sections are handled in yaml2obj.
> Similarly, it would help if you could point me to where we determine a section's type when we do code generation in LLVM.
When you say "determine" do you mean, where we write the type out in assembly, or something else? I'm not all that familiar with the code generation layer of LLVM (I mostly focus on the tools and file format sections).
================
Comment at: llvm/test/tools/llvm-objdump/Offload/offload-failure.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: not llvm-objdump --offloading %t 2>&1 | FileCheck %s
----------------
1) The option is `offloading` so the test (and directory) names should be updated to match.
2) There's not much point in naming a test "offloading-..." or similar, if it's already in an equivalent folder name.
3) It's useful to have a brief comment at the top of a test to explain what exactly it is testing. In this case, what causes the failure? (In newer tests in llvm-objdump we use `##` for comments, to distinguish them from RUN and CHECK lines).
================
Comment at: llvm/test/tools/llvm-objdump/Offload/offload-failure.test:9
+ Type: ET_EXEC
+ Machine: EM_X86_64
+Sections:
----------------
`Machine` type is usually optional, so you can omit it to reduce test noise.
================
Comment at: llvm/test/tools/llvm-objdump/Offload/offload-failure.test:11-17
+ - Name: .text
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ Address: 0x1010
+ AddressAlign: 0x0000000000000010
+ Content: "01234567"
+ Size: 4
----------------
Do you need the .text section for this test case?
================
Comment at: llvm/test/tools/llvm-objdump/Offload/offload-failure.test:20-24
+ Flags: [ SHF_EXCLUDE ]
+ Address: 0x1020
+ AddressAlign: 0x0000000000000008
+ Content: "00000000"
+ Size: 4
----------------
This can be simplified:
1. The flags aren't related to the offloading section printing, so get rid of them.
2. Is the address needed? I suspect not from my understanding of what the program is doing.
3. I suspect the AddressAlign field is also unnecessary for printing purposes, and can be omitted.
4. You don't need both Content and Size, unless the section size needs to be bigger than the specified content. You can omit the Content field and the Size parameter will set the section size still, with the content set to null bytes to fill it up.
================
Comment at: llvm/test/tools/llvm-objdump/Offload/offload-failure.test:25
+ Size: 4
+Symbols: []
+
----------------
As you don't have any Symbols, you can omit this line.
================
Comment at: llvm/test/tools/llvm-objdump/Offload/offload.test:11
+ Type: ET_EXEC
+ Machine: EM_X86_64
+Sections:
----------------
I think you may be able to omit the Machine line.
================
Comment at: llvm/test/tools/llvm-objdump/Offload/offload.test:12-20
+Sections:
+ - Name: .text
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ Address: 0x1010
+ AddressAlign: 0x0000000000000010
+ Content: "01234567"
----------------
As in the other test, you don't need the .text section (presumably) or the empty Symbols array.
================
Comment at: llvm/test/tools/llvm-objdump/Offload/offload.test:28
+# CHECK-EMPTY:
+# CHECK: OFFLOADING IMAGE [1]:
+# CHECK-NEXT: kind llvm ir
----------------
Use `CHECK-NEXT` here and in the following `OFFLOADING IMAGE` lines.
================
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
+
----------------
jhuber6 wrote:
> 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?
Normally, yaml2obj support would be a separate, prerequisite patch, but it depends on how straightforward it is to test the implementation without introducing a circular dependency (as the section is fairly simple, you might be able to test by just inspecting the raw bytes). If it's not straightforward to devise necessary test cases, I'd manually test it using some pregenerated output, and then include the actual implementation in this patch (so that the automated testing can use llvm-objdump).
It looks like you've not added the extra FileCheck switches mentioned in my first paragraph?
================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:35-36
+ return "ptx";
+ default:
+ return "<None>";
+ }
----------------
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).
================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:87
+ if (Error Err = visitAllBinaries(&Binary))
+ consumeError(std::move(Err));
+ }
----------------
Generally, we add a comment where we're deliberately throwing away errors, to explain why this is a good idea.
================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:95
+ if (Error Err = visitAllBinaries(OB))
+ consumeError(std::move(Err));
+}
----------------
Ditto.
================
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:
> > 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.
================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:66
+ if (!Contents)
+ reportError(Contents.takeError(), O->getFileName());
+
----------------
jhuber6 wrote:
> 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.
The fact that you've had to add code to handle the error shows that it is relevant, otherwise if the error handling is broken, you won't know. It should be fairly easy to test this using yaml2obj, which has the ability to overwrite the sh_offset field of the section header (using a "SHOffset" field name, if I remember rightly - look for examples).
================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:71
+ if (!BinaryOrErr)
+ reportError(BinaryOrErr.takeError(), O->getFileName());
+ OffloadBinary &Binary = **BinaryOrErr;
----------------
jhuber6 wrote:
> 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.
Usually it's a good idea to add more context to error messages that come out of the low-level libraries (see https://llvm.org/docs/CodingStandards.html#error-and-warning-messages). There are a number of examples of how this is done elsewhere in places like llvm-objdump and especially the llvm-readobj code. For example, the existing test case indicates simply that there's a problem with some encoding somewhere, but it's not clear which section that applies to (imagine if you were dumping multiple different sections at the same time).
================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:75
+ if (Error Err = visitAllBinaries(&Binary))
+ handleAllErrors(std::move(Err), [](const ErrorInfoBase &EI) {});
+ }
----------------
jhuber6 wrote:
> 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.
The usual pattern in more up-to-date dumping tools is to report warnings rather than errors, and then to continue parsing as best as possible (or bailing out of the section if it's impossible to do so). This allows us to get the maximum information out possible
================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:78
+
+/// Print the contents of a single offload binary file \p OB.
+void llvm::printOffloadBinary(const OffloadBinary *OB) {
----------------
jhuber6 wrote:
> jhenderson wrote:
> > jhuber6 wrote:
> > > tra wrote:
> > > > jhuber6 wrote:
> > > > > tra wrote:
> > > > > > I don't think the 'single' part of this assertion is true. AFAICT, `extractAllBinaries` will happily print all subsequent binaries if it finds them in the buffer. I think this should call `printBinary` instead.
> > > > > Yeah, I meant it more like to print on the single file that was already extracted or something. But it can definitely contain multiple. The reason I chose this method is because I wanted something that worked even if these sections were concatenated through a relocatable link or something. So whenever we parse one of these we just check the sizes to make sure there's not another one concatenated to it. I can make the comment less confusing.
> > > > I think the root of the problem here is that we're treating `OffloadBinary` as both the pointer to the binary itself and as a pointer to collection of such binaries.
> > > >
> > > > I think it's not a good API -- extractAllBinaries gets to look under the hood of the implmentation -- check if containing buffer has extra space beyond the OffloadBinary it's been passed. What if the user places something else in the memory buffer right behind the OffloadBinary object user passed to printOffloadBinary ? They would be within their rights to do so as the function would be expected to care about the content of the `*OB` only.
> > > >
> > > > I think we should be a bit more pedantic about such things. If we expect to operate on a collection, the API should reflect that. E.g. use SmallVector<OffloadBinary*>.
> > > > I think implementing `ObjectFile::offload_sections()` and `OffloadSection::offload_binaries()` would help both here and above. Or, possibly, just `ObjectFile::offload_binaries()``if we don't need to care about how binaries are stored in the object file and just wanr offload binaries themselves.
> > > >
> > > So the problem is we don't know how many of these are in here until we parse it. This requires getting the `size` field within the `OffloadBinary`. So even if we abstracted it to this iterator, it would still need some parsing like this behind the scenes. I could have made the binary format contain many within a single binary image, but like I said I wanted this to be stable under arbitrary concatenation by the linker. I'm not sure if we could have a different API considering the parsing requirements.
> > >
> > > This can definitely be problematic, depending on usage. I'm assuming if a user initialized an object on a memory buffer containing a bunch of junk it would probably be fine and just stop once the file is fully parsed. We could probably just ignore a parsing error, basically just stop tryingto read things if we don't catch the magic bytes or there's not enough space left over, but that's probably not ideal.
> > >
> > > It's definitely a little obtuse, but I'm not sure if there's a good way to make it work better considering how we parse them.
> > > I said I wanted this to be stable under arbitrary concatenation by the linker
> >
> > Have you looked at how DWARF debug sections like .debug_line or .debug_aranges are structured? Typically, these sections have a header which contains information like total size of that section (or number of entries in the section) and version information. These sections are still concatenated, with the length simply representing the contribution from a single CU.
> Right now I have a binary that knows its own size, and if the size of the buffer is greater than the size of that binary we look for another one. Forgive me if I'm misunderstanding here, but the linker will only concatenate sections right? Do these sections simply work as some kind of buffer whose size indicated how many sections were concatenated? That is, for every `.llvm.offloading` section I'd have some other reference section that just contains a single byte whose size I can check? Otherwise I'm not sure how we could figure out how many of these sections have been concatenated without parsing them first.
To be clear, I know very little about the new section type, how it is used and so on, so what I'm suggesting may not make much sense. Linkers concatenate sections blindly (in general). As such, if you had 1.o and 2.o each with a .llvm.offloading section, and you combine them into out.elf, you'd end up with a single output section containing the concatenation of the two. Presumably this means you'll end up with something that looks a bit like this?
```
.llvm.offloading
.llvm.offloading(1.o) - size field
.llvm.offloading(1.o) - rest of section
.llvm.offloading(2.o) - size field
.llvm.offloading(2.o) - rest of section
```
Is that correct? If so, I don't think there's anything to do here, assuming the section size is not guaranteed to be the same for all input sections.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:204
static bool RawClangAST;
+static bool Offloading;
bool objdump::Relocations;
----------------
Put it the line before RawClanAST, since Offloading appears before RawClangAST lexicographically.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126904/new/
https://reviews.llvm.org/D126904
More information about the llvm-commits
mailing list