[PATCH] D89432: [llvm-elfabi] Emit ELF .dynsym and .dynamic sections

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr accepted this revision.
mcgrathr added a comment.
This revision is now accepted and ready to land.

LGTM.  The section ordering details are conventional but not supposed to be semantically meaningful anyway.



================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:115
+  ELFSymbolTableBuilder() {
+    Elf_Sym S;
+    S.st_name = 0;
----------------
Seems safer to use `s{}`.



================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:128
+           uint16_t StShndx) {
+    Elf_Sym S;
+    S.st_name = StrTabBuilder.getOffset(StName);
----------------
Making this `S{}` both obviates the need to set any values explicitly to zero and also ensures no field was forgotten.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:143
+    size_t Offset = 0;
+    for (const auto &Sym : Symbols) {
+      memcpy(Buf + Offset, &Sym, sizeof(Elf_Sym));
----------------
This could be a single memcpy call using Symbols.data().



================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:203
+    size_t Offset = 0;
+    for (const auto &Entry : Entries) {
+      memcpy(Buf + Offset, &Entry, sizeof(Elf_Dyn));
----------------
This could be a single memcpy call using Entries.data().


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:209
+    Elf_Dyn Terminator;
+    memset(&Terminator, 0, sizeof(Elf_Dyn));
+    memcpy(Buf + Offset, &Terminator, sizeof(Elf_Dyn));
----------------
Just `constexpr Elf_Dyn{}` is enough here.  You could also just memset directly instead of copying some zero bytes with memcpy.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:249
+    SymTab.Shdr.sh_type = ShType;
+    // TODO: Verify if other flags are needed.
+    SymTab.Shdr.sh_flags = SHF_ALLOC;
----------------
No other flags are expected for SHT_DYNSYM or SHT_SYMTAB.



================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:263
+    DynTab.Shdr.sh_type = SHT_DYNAMIC;
+    DynTab.Shdr.sh_flags = SHF_ALLOC | SHF_WRITE;
+    DynTab.Shdr.sh_addr = DynTab.Addr;
----------------
This need not set SHF_WRITE.  e.g. Fuchsia uses `-z rodynamic` and some other platforms use R/O .dynamic sections.  The writability does not matter at link time.



================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:289
+    DynSym.Name = ".dynsym";
+    DynSym.Align = sizeof(Elf_Sym) == 16 ? 4 : 8;
+    DynTab.Name = ".dynamic";
----------------
`sizeof(Elf_Addr)` seems clearest here and in other such cases.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:303
+    llvm::SmallVector<OutputSection<ELFT> *, 8> Sections;
     Sections.push_back(&ShStrTab);
     Sections.push_back(&StrTab);
----------------
Traditionally the ordering is to put all SHF_ALLOC sections before all others.  .shstrtab in particular is usually the last section.



================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:305
     Sections.push_back(&StrTab);
+    Sections.push_back(&DynSym);
+    Sections.push_back(&DynTab);
----------------
Traditional ordering between these is: .dynsym, .dynstr, .dynamic.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89432