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

[Haowei Wu via Phabricator via llvm-commits]

haowei marked 2 inline comments as done.
haowei added inline comments.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:111
+private:
+  llvm::SmallVector<Elf_Sym, 64> Symbols;
+
----------------
MaskRay wrote:
> 64 is too large. This costs 24*64 bytes to the class. 0 may be fine assuming you always need a dynamic allocation (which is not a big deal)
I changed the value to 8, similar to other SmallVector used in this file. I think it should be small enough now.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:119
+
+  void add(ELFStringTableBuilder &StrTabBuilder, StringRef StName,
+           uint64_t StSize, uint8_t StBind, uint8_t StType, uint8_t StOther,
----------------
grimar wrote:
> Instead of passing both `StrTabBuilder` abd `StName` you can just pass an offset.
Done.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:304
+    // Poplulate dynamic table.
+    size_t DynSymIndex = DynTab.Content.addAddr(DT_SYMTAB, 0);
+    size_t DynStrIndex = DynTab.Content.addAddr(DT_STRTAB, 0);
----------------
MaskRay wrote:
> Linkers don't read DT_SYMTAB or DT_STRTAB.
I tried locally to remove DT_SYMTAB and DT_STRTAB and confirm it does not affect linking. But I would prefer to keep these 2 entries since the generated Stub SO file does have .dynstr and .dynsym sections. There might be other tools would look for these references in .dynamic table and it would yield unexpected results if they are removed. 


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:113
+
+public:
+  ELFSymbolTableBuilder() {
----------------
grimar wrote:
> haowei wrote:
> > grimar wrote:
> > > You can avoid having 2 `public:` areas by merging them.
> > The private section needs the "Elf_Sym" type alias declared in the first public section and the second public section requires the private "Symbols" variable. So I don't think I can merge the two public sections as it is a chicken and egg issue.
> There is no chicken and egg issue here. C++ doesn't work like that.
> You don`t have ro declare the `Symbols` member before using it in member functions.
> So, you can do the following:
> 
> ```
> template <class ELFT> class ELFSymbolTableBuilder {
> public:
>   using Elf_Sym = typename ELFT::Sym;
> 
>   ELFSymbolTableBuilder() {
>     Elf_Sym S{};
>     Symbols.push_back(S);
>   }
> 
>   void add(ELFStringTableBuilder &StrTabBuilder, StringRef StName,
>            uint64_t StSize, uint8_t StBind, uint8_t StType, uint8_t StOther,
>            uint16_t StShndx) {
>     Elf_Sym S{};
>     S.st_name = StrTabBuilder.getOffset(StName);
>     S.st_size = StSize;
>     S.st_info = (StBind << 4) | (StType & 0xf);
>     S.st_other = StOther;
>     S.st_shndx = StShndx;
>     Symbols.push_back(S);
>   }
> 
>   size_t getSize() const { return Symbols.size() * sizeof(Elf_Sym); }
> 
>   void write(uint8_t *Buf) const {
>     memcpy(Buf, Symbols.data(), sizeof(Elf_Sym) * Symbols.size());
>   }
> 
> private:
>   llvm::SmallVector<Elf_Sym, 64> Symbols;
> };
> ```
Thanks for pointing out. I have merged 2 public sections.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:195
+           sizeof(Elf_Dyn));
+  }
+};
----------------
grimar wrote:
> haowei wrote:
> > grimar wrote:
> > > If you add `DT_NULL` explicitly in the code, then you can simplify these 2 methods to:
> > > 
> > > 
> > > ```
> > > size_t getSize() const { return Entries.size() * sizeof(Elf_Dyn); }
> > >  
> > > void write(uint8_t *Buf) const { memcpy(Buf, Entries.data(), getSize()); }
> > > ```
> > > 
> > > 
> > > 
> > I prefer to keep "add DT_NULL" in the builder class instead of explicitly in the code. It reduces the chance a maintainer add new entries after DT_NULL entry in the future.
> I think, this is a design issue. With this approach `Entries` doesn't contain all entries written.
> `DT_NULL` is a terminator entry, but it is still an entry.
> 
> Though, if you want go this way, then you should be able to use `memset`:
> `memset(Buf + sizeof(Elf_Dyn) * Entries.size(), 0, sizeof(Elf_Dyn));`
> 
> instead of
> 
> ```
>     constexpr Elf_Dyn Terminator{};
>     memcpy(Buf + sizeof(Elf_Dyn) * Entries.size(), &Terminator,
>            sizeof(Elf_Dyn));
> ```
I changed it to memset.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89432