[PATCH] D122069: [Object] Add binary format for bundling offloading metadata

[Jon Chesterfield via Phabricator via llvm-commits]

JonChesterfield accepted this revision.
JonChesterfield added a comment.
This revision is now accepted and ready to land.

Couple of nits above but basically I'm convinced. The gnarly part of binary formats is string tables and I'm delighted that part of MC was readily reusable. Wrapping the string table in different bytes to align with the elf format may still be a good idea but it's not an obvious correctness hazard.

I like msgpack as a format but the writing machinery in llvm is not very reusable. Likewise elf is a great format but quite interwoven with MC. Protobuf seems to have the nice property of concatenating objects yielding a valid protobuf but the cost of codegen that isn't presently part of the llvm core, and is a slightly hairy dependency to pull in.

Medium term, factoring out parts of the elf handling for use here (and in lld?) is probably reasonable, but the leading magic bytes here are sufficient that we could detect that in backwards-compat fashion if the release gets ahead of us. The format here is essentially a string map which is likely to meet future requirements from other platforms adequately.

Thanks for sticking with this!



================
Comment at: llvm/include/llvm/Object/OffloadBinary.h:74
+
+  uint16_t getImageKind() const { return TheEntry->ImageKind; }
+  uint16_t getOffloadKind() const { return TheEntry->OffloadKind; }
----------------
these should probably be returning the enum types


================
Comment at: llvm/include/llvm/Object/OffloadBinary.h:97
+  struct Entry {
+    uint16_t ImageKind;    // The kind of the image stored.
+    uint16_t OffloadKind;  // The producer of this image.
----------------
enums here as well? They have uint16_t specified in the type so layout is stable


================
Comment at: llvm/lib/Object/OffloadBinary.cpp:20
+
+Expected<std::unique_ptr<OffloadBinary>>
+OffloadBinary::create(MemoryBufferRef Buf) {
----------------
Not sure Expected<> helps hugely here - stuff only goes wrong as 'parse_failed' or failed to allocate, which is kind of the same thing - so we could return a default-initialized (null) unique_ptr on failure without loss of information


================
Comment at: llvm/lib/Object/OffloadBinary.cpp:41
+  // Create a null-terminated string table with all the used strings.
+  StringTableBuilder StrTab(StringTableBuilder::ELF);
+  for (auto &KeyAndValue : OffloadingData.StringData) {
----------------
this is good, string table building is by far the most tedious part of formats like this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122069