[PATCH] D89432: [llvm-elfabi] Emit ELF .dynsym and .dynamic sections
Roland McGrath via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 23 10:43:19 PDT 2020
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
More information about the llvm-commits
mailing list