This is an archive of the discontinued LLVM Phabricator instance.

[Binary] Promote OffloadBinary to inherit from Binary
ClosedPublic

Authored by jhuber6 on Jun 1 2022, 12:49 PM.

Details

Summary

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.

Diff Detail

Event Timeline

jhuber6 created this revision.Jun 1 2022, 12:49 PM
Herald added a project: Restricted Project. · View Herald Transcript
jhuber6 requested review of this revision.Jun 1 2022, 12:49 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 1 2022, 12:49 PM
jhuber6 updated this revision to Diff 433504.Jun 1 2022, 1:01 PM

Fix test

tra added a comment.Jun 1 2022, 1:26 PM

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.
I'd change the type of Buffer field to MemoryBufferRef instead.

llvm/lib/Object/OffloadBinary.cpp
17

Nit: using namespace seems a bit too heavy-handed when you only need one enum
using object_error = llvm::object::object_error; would probably do.
TBH, the use of fully qualified enum in only two instances also looked OK to me.

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.

jhuber6 updated this revision to Diff 433534.Jun 1 2022, 1:43 PM

Addressing comments.

tra accepted this revision.Jun 1 2022, 2:08 PM
tra added inline comments.
llvm/lib/Object/OffloadBinary.cpp
28

It could be just object_error::parse_failed now. Applies to the return above, too.

llvm/unittests/Object/OffloadingTest.cpp
8

Nit: using namespace llvm::object;

This revision is now accepted and ready to land.Jun 1 2022, 2:08 PM
This revision was landed with ongoing or failed builds.Jun 1 2022, 3:41 PM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.Jun 1 2022, 5:07 PM
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

tra added inline comments.Jun 2 2022, 10:07 AM