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

[Haowei Wu via Phabricator via llvm-commits]

haowei added inline comments.


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


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:183
+    return this->addValue(DT_SONAME, StrTab.Content.getOffset(SOName));
+  }
+  size_t getSize() const {
----------------
grimar wrote:
> It is not clear why these e methods are needed? You could just call `addValue` on the caller side.
I feel these methods are better for readability. I removed those in latest patch.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89432