[PATCH] D64913: [yaml2obj] - Add a support for defining null sections in YAMLs.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/yaml2obj/elf-custom-null-section.yaml:6
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-readelf --sections %t1 | FileCheck %s --check-prefix=CASE1
+
----------------
Calling this prefix CASE1 when it is used by two test cases is a bit confusion. I recommend "DEFAULT" or something like that.


================
Comment at: test/tools/yaml2obj/elf-custom-null-section.yaml:22
+
+## Now define a SHT_NULL section which fields are all zeroed.
+## In this case it is equal to section normally created by default.
----------------
which fields are all zeroed -> with fields all zeroed


================
Comment at: test/tools/yaml2obj/elf-custom-null-section.yaml:23
+## Now define a SHT_NULL section which fields are all zeroed.
+## In this case it is equal to section normally created by default.
+
----------------
to the section created

(don't need both the "normally" and "by default").


================
Comment at: test/tools/yaml2obj/elf-custom-null-section.yaml:41
+    EntSize:      0x0
+    Link:         ""
+
----------------
Specify Link explicitly with an integer here (i.e. Link: 0).

What about Info, and Address and perhaps sh_offset?


================
Comment at: test/tools/yaml2obj/elf-custom-null-section.yaml:76
+# RUN: yaml2obj --docnum=4 %s -o %t4
+# RUN: llvm-readelf --sections %t4 | FileCheck %s --check-prefix=CASE3
+
----------------
Similar to above, this is called "CASE3" but is used in two different test cases. Perhaps REDEFINE?


================
Comment at: test/tools/yaml2obj/elf-custom-null-section.yaml:95
+
+
+## The same, but using a number as a Link value.
----------------
Nit: double blank line


================
Comment at: test/tools/yaml2obj/elf-custom-null-section.yaml:96
+
+## The same, but using a number as a Link value.
+
----------------
"The same as above"


================
Comment at: test/tools/yaml2obj/elf-custom-null-section.yaml:114
+
+## Check we report a error if null section sh_link field refers to unknown section.
+
----------------
a error -> an error
to unknown -> to an unknown


================
Comment at: test/tools/yaml2obj/elf-custom-null-section.yaml:130
+
+## Check that null section fields are zeroed by default.
+
----------------
Perhaps worth a slight rewording here:

"Check that null section fields are set to zero, if they are unspecified."


================
Comment at: test/tools/yaml2obj/elf-custom-null-section.yaml:142
+Sections:
+  - Type: SHT_NULL
----------------
How about a test case for a SHT_NULL section that is not the first listed section? I don't think there's anything in the ELF spec saying that you can't have one elsewhere in the section header table.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:301
   for (StringRef Name : State.implicitSectionNames())
-    if (State.SN2I.get(Name) > Doc.Sections.size())
+    if (State.SN2I.get(Name) > (Doc.Sections.size() - (DocNullSec ? 1 : 0)))
       ImplicitSections.push_back(Name);
----------------
I wonder if it would make sense to treat the null section the same as any other implicit section? What do you think?


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

https://reviews.llvm.org/D64913