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

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 6 09:26:55 PDT 2019


grimar marked 3 inline comments as done.
grimar added inline comments.


================
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).
+
----------------
jhenderson wrote:
> What is the bit in brackets referring to?
> What is the bit in brackets referring to?

We have content of size 1 and `Size` == 3:

```
    Content: "FF"
    Size:    3
```

I.e. I check here that "we can specify both `Size` and `Content` when size is **greater than**...."

Since you've noticed (thanks!) that test below (`--docnum=6`) is actually a duplicate of `--docnum=2`,
I'll rewrite this comment.



================
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).
----------------
jhenderson wrote:
> 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.
> I think this case is the same as the --docnum=2.
Oh, indeed. I'll fix.


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

Do you think it worth to add tests for other 2 string sections (`.shstrtab`, `.dynstr`) too?



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

https://reviews.llvm.org/D62957





More information about the llvm-commits mailing list