[PATCH] D108825: [yaml2obj] Allow variable number of directories

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D108825#2983834 <https://reviews.llvm.org/D108825#2983834>, @alfonsosanchezbeato wrote:

> Comments addressed. I've added tests as described, including a new yaml as `NumberOfRvaAndSize = [[VAR=<none>]]` does not seem to work.

That's unfortunate, and seems to be an artefact of how the header properties are used internally (they need to be of type `Optional<T>` to work with `<none>`). That sounds like something that could be improved in COFF yaml2obj in the future, but not right now. (Also, it should have been `NumberOfRvaAndSize: [[VAR=<none>]]` FWIW).



================
Comment at: llvm/lib/ObjectYAML/COFFEmitter.cpp:461-480
+    uint32_t NumDir = 0;
     for (const Optional<COFF::DataDirectory> &DD :
          CP.Obj.OptionalHeader->DataDirectories) {
+      if (NumDir >= CP.Obj.OptionalHeader->Header.NumberOfRvaAndSize)
+        break;
       if (!DD.hasValue()) {
         OS << zeros(uint32_t(0));
----------------
I think the suggested edit is equivalent, and also simpler overall.


================
Comment at: llvm/test/tools/yaml2obj/COFF/default-number-rva.yaml:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj --file-headers %t | FileCheck %s
----------------
yaml2obj provides the --docnum option to allow multiple YAML documents in the same file. That will allow you to move this test case into the same test file as the variable rva cases. That in turn may enable you to reduce the amount of CHECK statements needed, by sharing them between different FileCheck invocations.


================
Comment at: llvm/test/tools/yaml2obj/COFF/variable-number-rva.yaml:5
+
+# CHECK0:      NumberOfRvaAndSize: 0
+
----------------
Does the `DataDirectory` list get printed (as an empty list) in this case? It would be good to test that if so.


================
Comment at: llvm/test/tools/yaml2obj/COFF/variable-number-rva.yaml:133
+# ROUNDTRIP2-NEXT:    RelativeVirtualAddress: 0
+# ROUNDTRIP2-NEXT:    Size:            0
+
----------------
You need to show that there are no further attributes printed after those you check for. Applies to all cases.


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

https://reviews.llvm.org/D108825