[PATCH] D62809: [yaml2obj] - Change how we handle implicit sections.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/yaml2obj/implicit-sections.test:3
+## ".dynstr", ".symtab", ".strtab", ".shstrtab".
+## These sections usually added implicitly, but here we add
+## them explicitly in YAML and verify.
----------------
sections usually -> sections are usually


================
Comment at: test/tools/yaml2obj/implicit-sections.test:49
+## Check that yaml2obj creates empty .dynstr and .dynsym sections
+## in case when no dynamic symbols were specified and Content wasn't set,
+## but sections were explicitly listed. Check their VAs are correct.
----------------
in case -> for the case


================
Comment at: test/tools/yaml2obj/implicit-sections.test:50
+## in case when no dynamic symbols were specified and Content wasn't set,
+## but sections were explicitly listed. Check their VAs are correct.
+
----------------
but sections -> but the sections


================
Comment at: tools/yaml2obj/yaml2elf.cpp:403
+  SHeader.sh_info =
+      RawSec ? (unsigned)RawSec->Info : findFirstNonGlobal(Symbols) + 1;
+  SHeader.sh_entsize = (DocSec && DocSec->EntSize)
----------------
grimar wrote:
> 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).
Okay, sounds good.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:405
+  SHeader.sh_entsize = (DocSec && DocSec->EntSize)
+                           ? (unsigned)(*DocSec->EntSize)
+                           : sizeof(Elf_Sym);
----------------
grimar wrote:
> jhenderson wrote:
> > Casting to `unsigned` here seems wrong, since entsize is a 64 bit number...
> Hmm, right.
Actually, I lied slightly, in that sh_entsize's size is dependent on the ELF format (32 versus 64 bits). Not sure what the right approach here is though. We could truncate (as I think this will do) or emit an error (but the latter might be a little annoying to implement. Truncation is probably fine for now.


================
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;
----------------
grimar wrote:
> 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).
Okay.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:410
+
+  if (RawSec && RawSec->Content.binary_size()) {
+    RawSec->Content.writeAsBinary(
----------------
grimar wrote:
> 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.
Sounds good.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:428
+        makeArrayRef(Syms));
+    SHeader.sh_size = arrayDataSize(makeArrayRef(Syms));
+  }
----------------
grimar wrote:
> 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.
> 
> 
> 
Hmmm... I'm not comfortable about this behaviour change, since this could test failures/spurious passes. If there are no uses of this approach currently, I guess it's okay though.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:310-311
+    // We have a few sections like string or symbol tables that are added
+    // implicitly later. However, if they are explicitly specified in the YAML, we
+    // want to write them right now. This ensures the file offset remains correct.
+    if (initImplicitHeader(State, CBA, SHeader, Sec->Name, Sec.get())) {
----------------
clang-format?


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

https://reviews.llvm.org/D62809