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

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

> I decided to use a custom format rather than using an existing format (ELF, JSON, etc) because of the specialty use-case of this.

Will we ever need to process the files with tools built with a different LLVM version? E.g clang and lld may be built separately from different LLVM trees.
If so, then maintaining compatibility of the binary format will become more painful over time, compared to using json.

Considering that we already have json parsing and serialization in LLVM, I don't see much benefit growing yet another on-disk format. AFAICT, JSON should do the job encoding relevant data just fine and would need less maintenance long term.



================
Comment at: clang/include/clang/Basic/Offloading.h:87
+private:
+  struct Header {
+    uint8_t Magic[4] = {0x10, 0xFF, 0x10, 0xAD};
----------------
tra wrote:
> Given that it's an on-disk format I think these should be explicitly packed and carry a comment that it's an on-disk format which needs extra caution during future changes.
> 
> 
On-disk format could use more comments.


================
Comment at: clang/include/clang/Basic/Offloading.h:87-104
+  struct Header {
+    uint8_t Magic[4] = {0x10, 0xFF, 0x10, 0xAD};
+    uint32_t Version;
+    uint64_t Size;
+    uint64_t EntryOffset;
+  };
+
----------------
Given that it's an on-disk format I think these should be explicitly packed and carry a comment that it's an on-disk format which needs extra caution during future changes.




================
Comment at: clang/include/clang/Basic/Offloading.h:90
+    uint32_t Version;
+    uint64_t Size;
+    uint64_t EntryOffset;
----------------
What does `Size` cover and what units it uses -- bytes, number of Entries, something else?


================
Comment at: clang/include/clang/Basic/Offloading.h:95-96
+  struct Entry {
+    uint16_t ImageKind;
+    uint16_t OffloadKind;
+    uint32_t Flags;
----------------
Should we use the matching enum types here? 
We're using 16-bit enums, so it would save us on casts when we use those fields and would make it harder to set a wrong value by mistake.


================
Comment at: clang/include/clang/Basic/Offloading.h:97
+    uint16_t OffloadKind;
+    uint32_t Flags;
+    uint64_t TripleOffset;
----------------
Do these flags have defined meaning?


================
Comment at: clang/include/clang/Basic/Offloading.h:98
+    uint32_t Flags;
+    uint64_t TripleOffset;
+    uint64_t ArchOffset;
----------------
What are the offsets relative to?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122069