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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/OffloadYAML.h:38
+
+  std::vector<Member> Members;
+};
----------------
jhuber6 wrote:
> 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.
The purpose of having yaml2obj support for file formats is primarily to be able to create test cases for both common use-cases and edge cases, particularly malformed input cases. For example, in the ELF YAML format, we allow overriding the sh_offset and other section header fields with values that wouldn't make sense. This in turn allows us to write tests for things like "llvm-readobj emits an error (instead of crashing) if the section header offset points past the end of the file)". So, the utility for changing the offsets is to be able to craft test cases that show that tools don't crash and instead give a sensible error if being given invalid values (clearly we can't usually tell the difference between a valid offset and another offset that points within the file, but we can at least make sure the offset is not past the end of the file). Similarly, how do you test that the tool correctly emits an error if a version > 1 is detected without having the ability to change the version field? Certainly in tools like llvm-readobj and llvm-objdump, we expect test cases for every code path (including error paths). This is important because we have to be able to rely on these low-level tools because they are so widely used in testing other tools.

obj2yaml support is not all that important typically, but just sort of falls out of the wash typically because of how it interacts with yaml2obj.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127776