[PATCH] D149440: [yaml2obj] Add support for load config section data.

[Jacek Caban via Phabricator via llvm-commits]

jacek marked an inline comment as done.
jacek added inline comments.


================
Comment at: llvm/lib/ObjectYAML/COFFYAML.cpp:565-566
+  IO.mapOptional("Size", S.Size, support::ulittle32_t(sizeof(S)));
+  if (S.Size < sizeof(S.Size)) {
+    IO.setError("Size must be at least " + Twine(sizeof(S.Size)));
+    return;
----------------
jhenderson wrote:
> jacek wrote:
> > jhenderson wrote:
> > > Are you sure that wants to be `sizeof(S.Size)` in these two lines? Would it be worth giving `S` a more descriptive name?
> > Yes, that's what I meant. Size is the first member of load config structs. Load config that's too small to even fit the size field itself wouldn't make much sense. I will add a comment to clarify that, but I can also remove the check if you prefer (the check is not required from yaml2obj's point of view, I added it for completeness, it has a chance to catch some user errors).
> In other areas of yaml2obj (at least in the ELF side), we try to be as permissable as possible, as long as the object could be reasonably representable in the file format. For example, you could have an ELF symbol table without any symbols at all, even though the ELF file format states that there must be at least one symbol at index 0. So in this case, it might be reasonable to allow a zero-sized LoadConfig structure, so that you could test that consumer tools (e.g. llvm-readobj) can handle such a situation (even if it's just a case of reporting an error).
> 
> TL;DR: I don't really mind what you do. It largely depends how you want to use the tool.
For sizes like 2 I think it would be cleaner to use '-Binary: 0200' instead of LoadConfig and there is not much that llvm-readobj can do about such config: it will try to interpret bytes following load config as part of the size field. My opinion is not strong, but it felt right to have the check if we also have size checks for other members, so since you don't mind, I will leave it.


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

https://reviews.llvm.org/D149440