[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 inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/OffloadYAML.h:38
+
+  std::vector<Member> Members;
+};
----------------
jhenderson wrote:
> The spec discsusses a wider range of fields than just the members themselves. In particular, I'd expect to see an optional Version field, so that you can test using an arbitrary version number. Having optional size and offset fields would also allow you to override the defaults of these values, allowing you to test error paths better (e.g. to show that if the offset is past the end of the file, an error message is reported). The one field you probably don't need is the `Magic` field, in my opinion.
Those field are internal to the binary format, and testing them is done by even being able to extract the members correctly in `obj2yaml`. I'd prefer not to add in ways to change it as it would massively clutter up the API for creating them. I don't see the utility in changing any of the offsets because there's no way to know if they are valid without crashing or printing out garbage, so we just assume every binary with a valid magic number is valid. The version only exists to change the ABI in the future if we ever need to, it's a constant as far as I'm concerned and if it's not 1 we just hard error immediately. I could add it in if you really, really, really think it's worth testing these paths. But I would rather not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127776