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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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,
----------------
Instead of passing both `StrTabBuilder` abd `StName` you can just pass an offset.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:113
+
+public:
+  ELFSymbolTableBuilder() {
----------------
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;
};
```


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:195
+           sizeof(Elf_Dyn));
+  }
+};
----------------
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));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89432