We use the OffloadBinary to create binary images of offloading files
and their corresonding metadata. This patch changes this to inherit from
the base Binary class. This allows us to create and insepect these
more generically. This patch includes all the necessary glue to
implement this as a new binary format, along with added the magic bytes
we use to distinguish the offloading binary to the file_magic
implementation.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LGTM in principle, with new nits.
llvm/include/llvm/Object/OffloadBinary.h | ||
---|---|---|
121–122 | We're losing information by passing only the start of the buffer. | |
llvm/lib/Object/OffloadBinary.cpp | ||
17 | Nit: using namespace seems a bit too heavy-handed when you only need one enum | |
18 | Nit: Offload binary is declared within llvm namespace. Removing namespace here and relying on using namespace llvm; works, but makes things a bit less obvious. I'd keep the namespace around. Maybe, even remove using namespace llvm above, too as it should not be needed if the code is within the actual namespace. |
Thanks for the comments, I'll address them real quick.
llvm/include/llvm/Object/OffloadBinary.h | ||
---|---|---|
121–122 | Forgot to remove this one. I just put this here for convenience's sake since I need to index the MemoryBufferRef directly as a char array. The underlying Binary type already contains the buffer ref so we're not losing it. |
llvm/lib/Object/OffloadBinary.cpp | ||
---|---|---|
18 | using namespace llvm; is actually the preferred style. See other files in lib/Object/ and various other components. I fixed the style in 2108f7a243a5018b4ffc09bcbc2a8bdbe3c9a4d1 |
llvm/lib/Object/OffloadBinary.cpp | ||
---|---|---|
18 | Than you. I was unaware of this: |
We're losing information by passing only the start of the buffer.
I'd change the type of Buffer field to MemoryBufferRef instead.