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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:55
+  using Elf_Phdr = typename ELFT::Phdr;
+  using Elf_Shdr = typename ELFT::Shdr;
+
----------------
I think you can get rid of these usings.

`Elf_Phdr ` and `Elf_Shdr` are used only once, so you can inline them:
```
  ElfHeader.e_phentsize = sizeof(typename ELFT::Phdr);
  ElfHeader.e_shentsize = sizeof(typename ELFT::Shdr);
```

The same for `Elf_Ehdr` + you can change `memset` to

```
memset(&ElfHeader, 0, sizeof(ElfHeader));
```


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:113
+  using Elf_Addr = typename ELFT::Addr;
+  using Elf_Dyn = typename ELFT::Dyn;
+
----------------
You can use `TYPEDEF_ELF_TYPES(ELFT)` instead of adding these usings.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:124
+    StrTab.Align = 0;
+    for (const auto &Sym : Stub.Symbols)
+      StrTab.Content.add(Sym.Name);
----------------
Please don't use `auto` where type is not obvious.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:129
+    Sections.push_back(&ShStrTab);
+    Sections.push_back(&StrTab);
+    const OutputSection<ELFT> *LastSection = Sections.back();
----------------
```
std::vector<OutputSection<ELFT> *> Sections = { &ShStrTab, &StrTab };
```


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:133
+    uint64_t Index = 1;
+    for (auto Sec : Sections) {
+      Sec->Index = Index++;
----------------
Don't use auto.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:136
+      if (!Sec->Name.empty())
+        ShStrTab.Content.add(Sec->Name);
+    }
----------------
It should be OK to add an empty string to ELF string tables. Do you really need `if (!Sec->Name.empty())` condition?


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:142
+    StrTab.Size = StrTab.Content.getSize();
+    // Calculate sections' addrs and offsets.
+    const OutputSection<ELFT> *Prev = nullptr;
----------------
`addrs` -> `addresses`


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:145
+    uint64_t StartOffset = sizeof(Elf_Ehdr);
+    for (auto Sec : Sections) {
+      uint64_t Align = Sec->Align ? Sec->Align : 1;
----------------
Don't use auto.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:148
+      if (Sec->NoBits)
+        Align = 1;
+      if (Prev == nullptr)
----------------
Why nobits sections are special?

Also I don't understand:
1) There are just 2 sections currently: `ShStrTab` and `StrTab`. Why do you need the code for `NoBits`?
2) Why can't you set `Align = 1` from start for them instead of having `Sec->Align ? Sec->Align : 1;` logic?


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:169
+    ElfHeader.e_phnum = 0;
+    ElfHeader.e_phoff = 0;
+    if (LastSection->NoBits)
----------------
You don't need these 2 lines, because you do `memset(&ElfHeader, 0, sizeof(Elf_Ehdr));` in `initELFHeader`.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:560
+    raw_string_ostream Stream(Message);
+    Stream << FileReadError;
+    Stream << " when trying to open `" << FilePath << "` for writing";
----------------
You should be able to use `std::string toString(Error E)`:

```
Stream << toString(BufOrError.takeError());
```

Then you don't need the `FileReadError` variable and `consumeError`.

I think you should be able to do just something like:

```
if (!BufOrError)
  return createStringError(errc::invalid_argument,
    toString(BufOrError.takeError()) + " when trying to open `" +  FilePath + "` for writing");
```



================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:561
+    Stream << FileReadError;
+    Stream << " when trying to open `" << FilePath << "` for writing";
+    consumeError(std::move(FileReadError));
----------------
You don't have a test for this I think.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:570
+
+  if (Error FileWriteError = Buf->commit())
+    return FileWriteError;
----------------
It is more common naming I think.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:609
+
+  return createStringError(errc::invalid_argument,
+                           "Invalid binary output target");
----------------
this should be `llvm_unreachable` I think?


================
Comment at: llvm/test/tools/llvm-elfabi/write-elf32be-ehdr.test:15
+# CHECK-NEXT:     DataEncoding: BigEndian (0x2)
+# CHECK-NEXT:     FileVersion: 1{{$}}
+# CHECK-NEXT:     OS/ABI: SystemV (0x0)
----------------
Instead of using `{{$}}` everywhere, consider using `FileCheck --match-full-lines`.


================
Comment at: llvm/test/tools/llvm-elfabi/write-elf32be-ehdr.test:29
+# CHECK:        SectionHeaderEntrySize: 40{{$}}
\ No newline at end of file

----------------
No new line.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:48
+           cl::desc("Manually set the DT_SONAME entry of any emitted files"),
+           cl::value_desc("name"));
+cl::opt<ELFTarget> BinaryOutputTarget(
----------------
All above are unrelated formatting changes I think.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:141
+    TargetStub->SoName = SOName;
+  }
+
----------------
You don't need curly bracers around a single line:

```
 if (SOName.getNumOccurrences() == 1)
    TargetStub->SoName = SOName;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61767