[PATCH] D61767: [llvm-elfabi] Emit ELF header and string table section

[Armando Montanez via Phabricator via llvm-commits]

amontanez added a comment.

Initial thoughts from me are the code looks good, but this needs more documentation overall. The approach is clever, but not immediately intuitive so I feel there should be in-code comments to explain it at a high level. Sort of like your descriptions in D55864 <https://reviews.llvm.org/D55864>, but in-code so they are visible to everyone reading it.



================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:48
+// Lazy assumes that T is default constructable.
+// Lazy also acts like a read-only, move-only type.
+template <class T> class Lazy {
----------------
Add some comments with how this is intended to be used, and how cycles are detected.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:434-437
+  ElfHeader.e_ident[EI_MAG0] = 0x7f; // ELFMAG0
+  ElfHeader.e_ident[EI_MAG1] = 'E';  // ELFMAG1
+  ElfHeader.e_ident[EI_MAG2] = 'L';  // ELFMAG2
+  ElfHeader.e_ident[EI_MAG3] = 'F';  // ELFMAG3
----------------
Carry-over from my change: use ELFMAG constants instead of hard-coded values.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:474
+
+class ELFStringTableBuilder : public StringTableBuilder {
+public:
----------------
I'm assuming this just exists to make templating easier. If so, add a quick one-line comment to mention that's why it exists.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:495-505
+  template <class T>
+  static void WriteLazyVector(uint8_t *Data, const std::vector<Lazy<T>> &Vec) {
+    T *Iter = reinterpret_cast<T *>(Data);
+    for (const auto &Value : Vec) {
+      *Iter++ = *Value;
+    }
+  }
----------------
Probably can omit these until they're needed.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:521
+    const OutputSection<ELFT> *LastSection = Sections.back();
+    ElfHeader = [this, &Stub, LastSection](Elf_Ehdr &Ehdr) {
+      initELFHeader<ELFT>(Ehdr, Stub.Arch);
----------------
I'd like to see some in-code documentation here as this is where the overall approach is first introduced. It's not immediately clear how this block of code integrates into the overall solution.

Perhaps start with a top-level description for what the constructor does/how it is done, then add a comment to this lambda that describes how it fits into the big picture.

This will make the code significantly more approachable for anyone else who may have to read this code in the future.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:526
+      Ehdr.e_phnum = 0; 
+      Ehdr.e_phoff = sizeof(Elf_Ehdr);
+      if (LastSection->NoBits)
----------------
This should probably be 0 until program headers are added in a subsequent change.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:584
+  size_t GetSize() const {
+    return ElfHeader->e_shoff + ElfHeader->e_shnum * sizeof(Elf_Shdr);
+  }
----------------
This calculation doesn't look like it includes the string table.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61767