[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:4
+
+## Check that file offset of usually implicit sections ".dynsym",
+## ".dynstr", ".symtab", ".strtab", ".shstrtab" is ascending.
----------------
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.


================
Comment at: test/tools/yaml2obj/implicit-sections.test:40
+DynamicSymbols:
+  - Name:    _Z3fooi
+    Binding: STB_GLOBAL
----------------
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.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:260
+                                        Elf_Shdr &Header, StringRef SecName,
+                                        ELFYAML::Section *DocSec) {
+  // Was already initialized.
----------------
Here and elsewhere, I'd probably rename DocSec to YAMLSec or similar. I think that's a little clearer.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:261
+                                        ELFYAML::Section *DocSec) {
+  // Was already initialized.
+  if (Header.sh_offset)
----------------
My immediate thought was "What was already initialized?" so this probably should be a bit more verbose and say "Check if the header was already initialized".


================
Comment at: tools/yaml2obj/yaml2elf.cpp:310
+    // We have a few sections like string or symbol tables that are added
+    // implicitly later. But if they are explicitly specified in the YAML, we
+    // want to write them right now. That allows keeping the correct file offset.
----------------
"later. But" -> Either "later, but" or "later. However," (I think either is more natural English).


================
Comment at: tools/yaml2obj/yaml2elf.cpp:311
+    // implicitly later. But if they are explicitly specified in the YAML, we
+    // want to write them right now. That allows keeping the correct file offset.
+    if (initImplicitHeader(State, CBA, SHeader, Sec->Name, Sec.get())) {
----------------
That allows keeping the correct file offset -> This ensures the file offset remains correct.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:353
+
+  // Populate SHeaders with implicit sections not present in the Doc
+  for (StringRef Name : State.implicitSectionNames())
----------------
Missing trailing full stop.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:358
+
+  // Initialize the implicit sections
+  initImplicitHeader(State, CBA, SHeaders[State.SN2I.get(".symtab")], ".symtab",
----------------
Missing trailing full stop.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:403
+  SHeader.sh_info =
+      RawSec ? (unsigned)RawSec->Info : findFirstNonGlobal(Symbols) + 1;
+  SHeader.sh_entsize = (DocSec && DocSec->EntSize)
----------------
What if the raw section has no info specified? I'd expect it to auto-derive the property in this case.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:405
+  SHeader.sh_entsize = (DocSec && DocSec->EntSize)
+                           ? (unsigned)(*DocSec->EntSize)
+                           : sizeof(Elf_Sym);
----------------
Casting to `unsigned` here seems wrong, since entsize is a 64 bit number...


================
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;
----------------
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.


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


================
Comment at: tools/yaml2obj/yaml2elf.cpp:428
+        makeArrayRef(Syms));
+    SHeader.sh_size = arrayDataSize(makeArrayRef(Syms));
+  }
----------------
What about a user who explicitly specifies sh_size, but not Content? Is that possible currently for symbol table sections?


================
Comment at: tools/yaml2obj/yaml2elf.cpp:440
   SHeader.sh_type = ELF::SHT_STRTAB;
-  STB.write(CBA.getOSAndAlignedOffset(SHeader.sh_offset, SHeader.sh_addralign));
-  SHeader.sh_size = STB.getSize();
-  SHeader.sh_addralign = 1;
+  SHeader.sh_addralign = DocSec ? (unsigned)DocSec->AddressAlign : 1;
+
----------------
See above comments re. AddressAlign casting/auto-deriving.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:444
+      dyn_cast_or_null<ELFYAML::RawContentSection>(DocSec);
+  if (RawSec && RawSec->Content.binary_size()) {
+    RawSec->Content.writeAsBinary(
----------------
Similar to above, do we need an error if a user has specified a Symbol and a custom string table?


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

https://reviews.llvm.org/D62809