[PATCH] D55839: [elfabi] Add support for writing ELF header for binary stubs

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:220
+/// @return Size (in bytes) of the final binary stub.
+template <class ELFT> static size_t getBinarySize(const ELFStub &Stub) {
+  using Elf_Ehdr = typename ELFT::Ehdr;
----------------
Calculating the size and writing can be a bit fragile. That said you can't write until you have enough space allocated and because we want to use a FileBuffer and avoid copying, we only want to write once. A trick I've seen used is to think about 'WriteCommands' write commands know the maximum index that they write to and can perform the write themselves. So rather than performing size calculation and writes separately, you construct a list of write commands. From this list you then traverse it to get the maximum index written to, and then traverse it once more to write into the now allocated buffer. This way you don't have parallel code but you avoid reallocating a mapped buffer.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:246
+  // ELF identification
+  ElfHeader.e_ident[EI_MAG0] = 0x7f; // ELFMAG0
+  ElfHeader.e_ident[EI_MAG1] = 'E';  // ELFMAG1
----------------
Use ELFMAG* instead of each of the actual constants here.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:254
+  ElfHeader.e_ident[EI_VERSION] = EV_CURRENT;
+  ElfHeader.e_ident[EI_OSABI] = ELFOSABI_NONE;
+  ElfHeader.e_ident[EI_ABIVERSION] = 0;
----------------
It turns out that there are use cases where this can be other things. This is a good default but sometimes the user should have to specify this. @jhenderson has hit this issue in BSD land. I can't seem to find the code for that however. Hopefully James can weigh in.

Either way I don't think its something you need to worry about right now but it was possibly an oversight on our part.




================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:305-306
+    Stream << " when trying to open `" << FilePath <<"` for writing";
+    consumeError(std::move(FileReadError));
+    return createStringError(errc::invalid_argument, Stream.str().c_str());
+  }
----------------
You can append to an error I believe so that you don't have to consume it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55839