[PATCH] D55864: [elfabi] Write program headers, .dynamic, .dynstr, and .shstrtab

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

You may have put the rationale somewhere but they are not easily searchable. Please allow me to ask why there is a desire to write yet another ELF emitter. Can't we invest more in existing tools like yaml2obj? Is there any inherent limitation that prevents it from being more dynamic?



================
Comment at: llvm/llvm/test/tools/llvm-elfabi/binary-write-pheaders.test:18
+# PHDRS-NEXT:   PhysicalAddress: 0x0{{$}}
+# PHDRS-NEXT:   FileSize: {{.*$}}
+# PHDRS-NEXT:   MemSize: {{.*$}}
----------------
`{{.*$}}` can just be omitted. Many other tests do this.

If you want to use `FileCheck --match-full-lines` to omit the regex in `4096{{$}}`, they have to be kept.


================
Comment at: llvm/llvm/test/tools/llvm-elfabi/binary-write-sheaders.test:60
+# SECTIONS-NEXT:   Index: 3{{$}}
+# SECTIONS-NEXT:   Name: .shstrtab (18)
+# SECTIONS-NEXT:   Type: SHT_STRTAB (0x3)
----------------
It'd be great to get rid of `.shstrtab` and just use `.strtab`.

Newer llvm (since 2015) no longer produces `.shstrtab` as a size optimization.


================
Comment at: llvm/test/tools/llvm-elfabi/write-elf32be-ehdr.test:6
+TbeVersion: 1.0
+Arch: x86_64
+Symbols: {}
----------------
Change `x86_64` to `powerpc`, `armv7a`, etc to make big-endian more realistic.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:54
+  mutable Optional<T> Value;
+  mutable bool Blackhole = false;
+
----------------
Other than having a mutable `Blackhole`, you can make `Func = []() { llvm_unreachable(...); };` while evaluating the thunk.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:87
+      return *Value;
+    else {
+      Blackhole = true;
----------------
Early return?


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:89
+      Blackhole = true;
+      Value = T();
+      Func(*Value);
----------------
`Value.emplace()`.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:523
+  static void Write(uint8_t *Data, const T& Value) {
+    *reinterpret_cast<T*>(Data) = Value;
+  }
----------------
`T*` -> `T *`


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:583
+    // Define the non-TLS section.
+    DefSec.Name = ".def";
+    DefSec.Shdr = [this](Elf_Shdr& Shdr) {
----------------
What is this section used for?


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:599
+    // Define the TLS section.
+    TlsSec.Name = ".tls";
+    TlsSec.Shdr = [this](Elf_Shdr& Shdr) {
----------------
Which architecture uses the section name `.tls`?

Did you mean `.tbss`?


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

https://reviews.llvm.org/D55864