[PATCH] D62957: [yaml2obj/obj2yaml] - Make RawContentSection::Content and RawContentSection::Size optional

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 6 09:01:09 PDT 2019


jhenderson added inline comments.


================
Comment at: test/tools/yaml2obj/section-size-content.yaml:8
+# RUN: not yaml2obj --docnum=1 %s -o %t 2>&1 | FileCheck %s --check-prefix=ERR
+# ERR: error: Section size must be greater or equal to the content size
+
----------------
greater -> greater than


================
Comment at: test/tools/yaml2obj/section-size-content.yaml:24
+## `Size` is equal to the content size. We check that this is allowed and
+## the output section has a correct size value.
+
----------------
and the -> and that the


================
Comment at: test/tools/yaml2obj/section-size-content.yaml:26
+
+# RUN: yaml2obj --docnum=2 %s -o %t
+# RUN: llvm-readobj %t --sections | FileCheck %s --check-prefix=CASE2
----------------
I've found it sometimes useful for each test case output to be a separate file i.e. use %t, %t2, %t3 etc instead of always %t.


================
Comment at: test/tools/yaml2obj/section-size-content.yaml:82
+## Check we can specify both `Size` and `Content` when size is greater
+## than or equal to content size. In this case the zeroes padding is
+## added after content specified. (Check "greater then" case here).
----------------
In this case the zeroes padding is added -> In this case zeroes are added as padding


================
Comment at: test/tools/yaml2obj/section-size-content.yaml:83
+## than or equal to content size. In this case the zeroes padding is
+## added after content specified. (Check "greater then" case here).
+
----------------
What is the bit in brackets referring to?


================
Comment at: test/tools/yaml2obj/section-size-content.yaml:103-105
+## Check we can specify both `Size` and `Content` when size is greater
+## than or equal to content size. In this case the zeroes padding is
+## added after content specified. (Check "equal" case here).
----------------
I think this case is the same as the --docnum=2 test case, right? You can use --section-data to get the section data if you want via llvm-readelf too.


================
Comment at: test/tools/yaml2obj/section-size-content.yaml:125
+
+## Check we emit an empty section if neither 'Content' or 'Size' were set.
+
----------------
or -> nor


================
Comment at: test/tools/yaml2obj/strtab-implicit-sections-size-content.yaml:4
+
+## When no `Size` or `Content` is specified for string table section,
+## yaml2obj writes the default implicitly created string table as a content.
----------------
for string -> for a string


================
Comment at: test/tools/yaml2obj/strtab-implicit-sections-size-content.yaml:5
+## When no `Size` or `Content` is specified for string table section,
+## yaml2obj writes the default implicitly created string table as a content.
+
----------------
I think you can simplify this to "yaml2obj writes the default content."


================
Comment at: test/tools/yaml2obj/strtab-implicit-sections-size-content.yaml:26
+
+## For string table sections, `Size` can be used along to override the
+## implicit string table data. Content is filled by zeroes in this case.
----------------
remove "along" (probably should be "alone" but still not neeed).


================
Comment at: test/tools/yaml2obj/strtab-implicit-sections-size-content.yaml:27
+## For string table sections, `Size` can be used along to override the
+## implicit string table data. Content is filled by zeroes in this case.
+
----------------
"The content is filled with zeroes in this case."


================
Comment at: test/tools/yaml2obj/strtab-implicit-sections-size-content.yaml:52
+
+## For string table sections, `Content` can be used along to override the
+## implicit string table data.
----------------
Remove "along"


================
Comment at: test/tools/yaml2obj/strtab-implicit-sections-size-content.yaml:76
+## For string table sections, check we can specify both `Size` and `Content`
+## when size is greater than or equal to content size. In this case the zeroes
+## padding is added after content specified. (Check "greater then" case here)
----------------
jhenderson wrote:
> "In this case zeroes are added as padding after the specified content"
> 
> Remove the bit in parentheses.
remove "or equal to"


================
Comment at: test/tools/yaml2obj/strtab-implicit-sections-size-content.yaml:76-77
+## For string table sections, check we can specify both `Size` and `Content`
+## when size is greater than or equal to content size. In this case the zeroes
+## padding is added after content specified. (Check "greater then" case here)
+
----------------
"In this case zeroes are added as padding after the specified content"

Remove the bit in parentheses.


================
Comment at: test/tools/yaml2obj/strtab-implicit-sections-size-content.yaml:101-102
+## For string table sections, check we can specify both `Size` and `Content`
+## when size is greater than or equal to content size. In this case the zeroes
+## padding is added after content specified. (Check "equal" case here)
+
----------------
"when size is equal to content size." Then delete the rest of the comment.


================
Comment at: test/tools/yaml2obj/symtab-implicit-sections-size-content.yaml:5
+## When no `Size` or `Content` is specified for symbol table section,
+## yaml2obj writes the default implicitly created symbol table as a content.
+# RUN: yaml2obj --docnum=1 %s -o %t
----------------
Same comments from the previous test apply here throughout this test.


================
Comment at: test/tools/yaml2obj/symtab-implicit-sections-size-content.yaml:63
+
+## Check we can use just `Content` to emit the custom data in the symbol table section.
+# RUN: yaml2obj --docnum=4 %s -o %t
----------------
emit the custom -> emit custom


================
Comment at: test/tools/yaml2obj/symtab-implicit-sections-size-content.yaml:82
+
+## Check we can use just `Size` to emit the custom data filled with zeroes
+## in the symbol table section.
----------------
Ditto


================
Comment at: test/tools/yaml2obj/symtab-implicit-sections-size-content.yaml:85
+# RUN: yaml2obj --docnum=5 %s -o %t
+# RUN: llvm-objdump %t -D | FileCheck %s --check-prefix=CASE5
+
----------------
You should be able to use llvm-readelf --section-data or --hex-dump here and below rather than this to check the output.


================
Comment at: test/tools/yaml2obj/symtab-implicit-sections-size-content.yaml:104-106
+## Check we can specify both `Size` and `Content` when size is greater
+## than or equal to content size. In this case the zeroes padding is
+## added after content specified. (Check "greater then" case here).
----------------
See comments in previous test for this and next test case.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:403
   bool IsStatic = STType == SymtabType::Static;
+  const auto &Symbols = IsStatic ? Doc.Symbols : Doc.DynamicSymbols;
+
----------------
You probably need some test cases for DynamicSymbols as well as static Symbols.


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

https://reviews.llvm.org/D62957





More information about the llvm-commits mailing list