[PATCH] D69709: [yaml2obj] - Add a way to describe the custom data that is not part of an output section.

Thu Nov 7 01:45:40 PST 2019

jhenderson added inline comments.

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:2
+## Here we check that we are able to define sections with a type of "CustomFiller".
+## Fillers are custom pieces of data that can be placed anywhere just like normal
+## output sections. But they are not real output sections and you'll never see them in
----------------
Fillers -> Fills (here and everywhere else in this file).

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:3
+## Fillers are custom pieces of data that can be placed anywhere just like normal
+## output sections. But they are not real output sections and you'll never see them in
+## the section headers.
----------------
either "sections, but" or "sections. However," (I marginally prefer the first)

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:8
+## to describe the data emitted.
+## Check the data emitted and how it affects on regular sections offsets.
+## Check that "Name" field is optional for fillers.
----------------
affects on -> affects

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:9
+## Check the data emitted and how it affects on regular sections offsets.
+## Check that "Name" field is optional for fillers.
+## Check that "Size" can be greater than or equal to the pattern data size.
----------------
the "Name" field

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:17-18
+# BASIC:   [Nr] Name Type     Address          Off    Size
+# BASIC:   [ 1] .foo PROGBITS 0000000000000000 000043 000002
+# BASIC:   [ 2] .bar PROGBITS 0000000000000000 000049 000001
+
----------------
Maybe use BASIC-NEXT and test that there are no more entries apart from the expected .shstrtab (in other words, prove that there are no extra section headers for the fills).

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:20
+
+# RUN: od -t x1 -v %t1 | FileCheck %s --ignore-case --check-prefix=DATA
+# DATA: aa bb aa 11 22 cc dd cc dd ff
----------------
Here and elsewhere in the test, consider using the -j and -N options for od to test the specific area you care about. Also test the offset field, to show that you are definitely checking the right bit.

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:40-42
+  - Name:    .bar
+    Type:    SHT_PROGBITS
+    Content: "FF"
----------------
Maybe worth one last fill at the end?

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:44
+
+## Check we can have no explicit regular sections in the YAML description, and describe
+## the content with the use of fillers only.
----------------
and can describe

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:55
+# NOSECTIONS:   [ 1] .strtab   STRTAB 0000000000000000 000045 000001 00     0   0  1
+# NOSECTIONS:   [ 2] .shstrtab STRTAB 0000000000000000 000046 000013 00     0   0  1
+
----------------
Similar comment to above. Show that there are no other sections. Perhaps test e_shnum to achieve this?

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:58
+# RUN: od -t x1 -v %t2 | FileCheck %s --ignore-case --check-prefix=NOSECTIONS-DATA
+# NOSECTIONS-DATA: aa bb cc dd ee 00
+
----------------
This needs a comment explaining why the 00 is guaranteed to be that (i.e. the .strtab section starts at 0x46 and always has a nul byte at the start).

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:75
+## Check we can use named fillers when describing program headers.
+## Check that fillers consume the file size and therefore affect on p_filesz fields of segments.
+## Check that the address alignment of the filler is 1 (check the p_align field for that).
----------------
affect the

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:76
+## Check that fillers consume the file size and therefore affect on p_filesz fields of segments.
+## Check that the address alignment of the filler is 1 (check the p_align field for that).
+
----------------
Maybe change this sentence to "Check that the fill does not affect the p_align field of the segment."

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:110
+  - Type: PT_LOAD
+    Flags: [ PF_R ]
+    Sections:
----------------
Flags is unnecessary (here and below).

You probably should have an address specified though, because the section address doesn't match up with the segment address. This is maybe something we should fix in yaml2obj (i.e. segment addresses are derived from contained sections if not specified, and possibly vice versa).

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:112
+    Sections:
+      - Section: .bar
+      - Section: filler
----------------
Maybe worth having a fill before .bar too, to show that they aren't just always placed at the end of a segment or something like that. It would also show that the correct fill can be selected this way (by having different names).

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:114
+      - Section: filler
+  - Type: PT_LOAD
+    Flags: [ PF_R ]
----------------
Nit: nested PT_LOADs are odd. Change this to something else like PT_GNU_RELRO or PT_TLS.

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:149
+
+## Check that fillers disallowed to have duplicate names.
+# RUN: not yaml2obj --docnum=6 2>&1 %s | FileCheck %s --check-prefix=UNIQUE-NAME
----------------
disallowed to have -> are not allowed to have

================
Comment at: llvm/test/tools/yaml2obj/custom-filler.yaml:233
+
+# RUN: not yaml2obj --docnum=10 2>&1 %s | FileCheck %s --check-prefix=UNK-ERR
+# UNK-ERR: error: unknown section or filler referenced: 'filler' by program header
----------------
Maybe UNKNOWN-ERR would be clearer

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

https://reviews.llvm.org/D69709