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

[Eric Christopher via Phabricator via llvm-commits]

echristo added a comment.

Mostly looks good, some inline comments and questions.

Thanks!

-eric



================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:60
 
+constexpr uint8_t ELFMAG0 = 0x7f;
+constexpr uint8_t ELFMAG1 = 'E';
----------------
This should at least be near the other magic version parts?


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:32
+
+using namespace llvm::elfabi;
 
----------------
What's with the change?


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:58
+// This type is not currently threadsafe.
+template <class T> class Lazy {
+private:
----------------
The name is somewhat inscrutable, perhaps something else?


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:194
   if (Dyn.SONameOffset.hasValue() && *Dyn.SONameOffset >= Dyn.StrSize) {
-    return createStringError(
-        object_error::parse_failed,
-        "DT_SONAME string offset (0x%016" PRIx64
-        ") outside of dynamic string table",
-        *Dyn.SONameOffset);
+    return createStringError(object_error::parse_failed,
+                             "DT_SONAME string offset (0x%016" PRIx64
----------------
The reformatting looks like it could be done separately?


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:436
+template <class ELFT>
+void initELFHeader(typename ELFT::Ehdr &ElfHeader, uint16_t Machine) {
+  using Elf_Ehdr = typename ELFT::Ehdr;
----------------
There's already a lot of code in ELFWriter to do this, perhaps you could use it? Or at least support::endian::Writer? In that I see where you're going with this, but perhaps it could be done in a different way rather than redoing existing work (there are only so many times we need to have a new way of writing an elf header :)


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:644
+                      ELFTarget OutputFormat) {
+  if (OutputFormat == ELFTarget::ELF32LE) {
+    return writeELFBinaryToFile<ELF32LE>(FilePath, Stub);
----------------
llvm style is no braces for a single line.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61767