[PATCH] D62809: [yaml2obj] - Change how we handle implicit sections.
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 4 06:03:21 PDT 2019
grimar added inline comments.
================
Comment at: test/tools/yaml2obj/implicit-sections.test:4
+
+## Check that file offset of usually implicit sections ".dynsym",
+## ".dynstr", ".symtab", ".strtab", ".shstrtab" is ascending.
----------------
jhenderson wrote:
> Your comment says that you are checking the file offset of these sections, but the CHECK tests the other properties too. Should the comment be expanded to say something like "Check the section header properties of ..." or should the text being checked for be relaxed (e.g. by regexing out the Type and Address columns, and removing the Size and Nr columns?
>
> FWIW, I think there is probably value in checking the Address, Size and Type output, if they aren't explicitly tested elsewhere, so I think the comment is the thing that needs changing.
>
> Also, does the offset really need to be ascending? I don't think that's strictly required.
I rewrote the comment.
> FWIW, I think there is probably value in checking the Address, Size and Type output, if they aren't explicitly tested elsewhere, so I think the comment is the thing that needs changing.
I think testing the `Size` is usefull, because allows to check how the `Off` was calculated.
Testing `Address` is valueable for demonstration of the correlation between `Off` and `Address`.
It is not harmfull to check the 'Type` since how section header is written only once (i.e. bug was fixed and
logic changed significantly. Previously they were written as `RawContentSection` at first)
and this test case is probably only one when we show the behavior of all implicit sections in the
situation when them were all of them are explicitly described in YAML.
> Also, does the offset really need to be ascending? I don't think that's strictly required.
I think the order of sections should match the YAML.
Without this patch offsets would be unordered, what looks obviously wrong to me:
```
Section Headers:
[Nr] Name Type Address Offset
Size EntSize Flags Link Info Align
[ 0] NULL 0000000000000000 00000000
0000000000000000 0000000000000000 0 0 0
[ 1] .dynstr STRTAB 0000000000000100 00000280
0000000000000009 0000000000000000 A 0 0 1
[ 2] .dynsym DYNSYM 0000000000000150 00000250
0000000000000030 0000000000000018 A 1 0 8
[ 3] .symtab SYMTAB 0000000000000000 00000200
0000000000000018 0000000000000018 4 0 8
[ 4] .strtab STRTAB 0000000000000000 00000218
0000000000000001 0000000000000000 0 0 1
[ 5] .shstrtab STRTAB 0000000000000000 00000219
0000000000000035 0000000000000000 0 0 1
[ 6] .text.foo PROGBITS 0000000000000200 00000200
0000000000000000 0000000000000000 0 0 0
```
And that it what my comment tried to say - if there was not described something very special in YAML (i.e. offsets were not explicitly set somehow),
then I think it is very natural to place sections in the order they described and make them have ascending file offset by default.
(that is what all linkers do I believe, for example).
The requirement I know is:
"To obtain this efficiency in practice, executable and shared object files must have segment images whose file offsets and virtual addresses are congruent, modulo the page size."
(https://docs.oracle.com/cd/E19683-01/816-1386/chapter6-34713/index.html)
yaml2obj does not follow it it seems. At least for sample above. But still having offset(.dynstr) > offset(.dynsym) and
VA(.dynstr) < VA(.dynsym) at the same time it something very strange/unnatural IMO.
================
Comment at: test/tools/yaml2obj/implicit-sections.test:40
+DynamicSymbols:
+ - Name: _Z3fooi
+ Binding: STB_GLOBAL
----------------
jhenderson wrote:
> Maybe worth putting a comment to say that the DynamicSymbols block is required for the .dynsym to be generated?
>
> It might also be good to have a test case for no dynamic symbol.
Done.
================
Comment at: tools/yaml2obj/yaml2elf.cpp:403
+ SHeader.sh_info =
+ RawSec ? (unsigned)RawSec->Info : findFirstNonGlobal(Symbols) + 1;
+ SHeader.sh_entsize = (DocSec && DocSec->EntSize)
----------------
jhenderson wrote:
> What if the raw section has no info specified? I'd expect it to auto-derive the property in this case.
Yes, but we probably need to make `RawSec->Info` be `Optional` for doing that. It is not true atm.
(I am planning to improve this and other situations in a followups).
================
Comment at: tools/yaml2obj/yaml2elf.cpp:405
+ SHeader.sh_entsize = (DocSec && DocSec->EntSize)
+ ? (unsigned)(*DocSec->EntSize)
+ : sizeof(Elf_Sym);
----------------
jhenderson wrote:
> Casting to `unsigned` here seems wrong, since entsize is a 64 bit number...
Hmm, right.
================
Comment at: tools/yaml2obj/yaml2elf.cpp:407
+ : sizeof(Elf_Sym);
+ SHeader.sh_addralign = DocSec ? (unsigned)DocSec->AddressAlign : 8;
+ SHeader.sh_addr = DocSec ? (unsigned)DocSec->Address : 0;
----------------
jhenderson wrote:
> Ditto on both points. AddressAlign is a 64-bit field, so casting to `unsigned` seems wrong. Additionally, a user might expect yaml2obj to derive the AddressAlign field automatically if they don't explicitly specify it.
Casting was fixed.
> Additionally, a user might expect yaml2obj to derive the AddressAlign field automatically if they don't explicitly specify it.
Again, it seems `AddressAlign` needs to be optional. I'd leave it for a follow up(s).
================
Comment at: tools/yaml2obj/yaml2elf.cpp:410
+
+ if (RawSec && RawSec->Content.binary_size()) {
+ RawSec->Content.writeAsBinary(
----------------
jhenderson wrote:
> Not sure exactly where it belongs, but I think we need some sort of error in case somebody explicitly specifies content for a symbol table, but also specifies static symbols.
Yes, sure I though about this too. I would like to do it in a follow up because I guess we
need to make `Content` be `Optional` to do that correctly.
================
Comment at: tools/yaml2obj/yaml2elf.cpp:428
+ makeArrayRef(Syms));
+ SHeader.sh_size = arrayDataSize(makeArrayRef(Syms));
+ }
----------------
jhenderson wrote:
> What about a user who explicitly specifies sh_size, but not Content? Is that possible currently for symbol table sections?
It was possible before this patch to do:
```
!ELF
FileHeader:
Class: ELFCLASS64
Data: ELFDATA2LSB
Type: ET_DYN
Machine: EM_X86_64
Sections:
- Name: .dynsym
Type: SHT_DYNSYM
Address: 0x150
Size: 0x200
```
And get the .dynsym' of size 0x200. With this patch the default symbol table will be created
(because there is no `Content`) in this case and `Size` will be ignored.
Again, it seems to me that for correct implementation `Size` need to be `Optional`.
Otherwise we can't distinguish "No Size was set" and "Size was set to zero" cases.
================
Comment at: tools/yaml2obj/yaml2elf.cpp:444
+ dyn_cast_or_null<ELFYAML::RawContentSection>(DocSec);
+ if (RawSec && RawSec->Content.binary_size()) {
+ RawSec->Content.writeAsBinary(
----------------
jhenderson wrote:
> Similar to above, do we need an error if a user has specified a Symbol and a custom string table?
Similar answer :]
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62809/new/
https://reviews.llvm.org/D62809
More information about the llvm-commits
mailing list