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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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;
----------------
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.


================
Comment at: llvm/lib/ObjectYAML/COFFYAML.cpp:559
+void mapLoadConfigMember(IO &IO, T &LoadConfig, const char *Name, M &Member) {
+  // Map only members that mach a specified size.
+  if (reinterpret_cast<char *>(&Member) -
----------------



================
Comment at: llvm/test/tools/yaml2obj/COFF/load-config.yaml:1
+## Test all members of 64-bit load config.
+# RUN: yaml2obj --docnum=1 %s -o %t
----------------
With multi-case tests like this, if they aren't sharing the same checks/inputs, I tend to write them like this:

```
## Test case comment.
# RUN: ...
# RUN: ... | FileChcek %s --check-prefix=FOO

# FOO: ...

<yaml for test case 1>

## Test case 2 comment.
# RUN: ...
# RUN: ... | FileChcek %s --check-prefix=BOO

# BOO: ...

<yaml for test case 2>
```

etc, as this makes it easier to see the parts of the respective test case. OTOH, if the inputs are shared between many test cases, possibly through the use of yaml2obj's -D option, I tend to put the input at the end of the block of test caes that use it.


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

https://reviews.llvm.org/D149440