[PATCH] D127776: [ObjectYAML] Add offloading binary implementations for obj2yaml and yaml2obj

[Joseph Huber via Phabricator via llvm-commits]

jhuber6 marked 2 inline comments as done.
jhuber6 added a comment.

In D127776#3584397 <https://reviews.llvm.org/D127776#3584397>, @jhenderson wrote:

> Have you got any reference material that I can read for the format of offloading binaries? I can't really review that this implementation does the right thing without knowing the format!

Sure, it's documented here https://clang.llvm.org/docs/ClangOffloadPackager.html. More or less, it's just a few integers followed by a list of offsets into an Elf-like string table  that forms a string map.



================
Comment at: llvm/lib/ObjectYAML/OffloadYAML.cpp:26
+  IO.mapTag("!Offload", true);
+  IO.mapOptional("Members", O.Members);
+  IO.setContext(nullptr);
----------------
jhenderson wrote:
> Here and with all the other fields, you need test cases that show that, if they're optional, the fields can be omitted (and what their default values are if so), and if they're required, an error is emitted if the field is missing.
It's not really important that these be optional since there's not that many of them. I may just make them all required to avoid the hassle.


================
Comment at: llvm/lib/ObjectYAML/OffloadYAML.cpp:51
+      (object::getOffloadKind(*M.OffloadKind) == object::OFK_None)) {
+    return "Invalid offload kind provided";
+  }
----------------
jhenderson wrote:
> Here and below, these error messages should start with a lower-case letter, I suspect (see the Coding Standards for error message format).
> 
> That being said, in ELF yaml2obj, we allow specification of values that may not be permitted by the spec but could theoretically appear in the field, if something wrote the wrong value. This allows for testing of error paths more effectively. I can't give further pointers though without knowing the format and what makes sense to allow to handcraft.
Honestly this error message isn't strictly necessary. In the linker wrapper where we actually use these binaries we handle unknown cases, so I may just remove these validation steps.


================
Comment at: llvm/tools/obj2yaml/offload2yaml.cpp:1
+//===------ utils/offload2yaml.cpp - obj2yaml conversion tool ---*- C++ -*-===//
+//
----------------
jhenderson wrote:
> This file isn't in "utils", so the header tag here is wrong.
Copied the header from `archive2yaml.cpp` which should probably be fixed as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127776