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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

The structure looks most good but I've got a lot of little requests.



================
Comment at: llvm/test/tools/llvm-elfabi/invalid-bin-target.test:7
+Arch: x86_64
+Symbols:
+  foo: { Type: Func }
----------------
There's no need to have these extra symbols here.


================
Comment at: llvm/test/tools/llvm-elfabi/missing-bin-target.test:7
+Arch: x86_64
+Symbols:
+  foo: { Type: Func }
----------------
ditto.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:96
+template <class ELFT>
+Error ELFBinaryWriterImpl<ELFT>::writeToBuffer(const ELFStub &Stub, char *Buf,
+                                               size_t Size) {
----------------
Not sure I like this interface but if you do want todo something like this 1) use uint8_t instead of 'char' and 2) MutableArrayRef wraps the pointer and size for you so you don't have to carry them around while still allowing you to modify the contents as you need to.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:98
+                                               size_t Size) {
+  if (Size < getBinarySize(Stub)) {
+    return createStringError(errc::invalid_argument,
----------------
Calling getBinarySize twice since the user already has to call it isn't ideal. Also if you want this check it seems like an assertion would be better. Also if you just return a Buffer from here rather than telling the user how big of a buffer to construct (though you force them to use a specific kind of buffer) then you can avoid the check all together.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:112
+
+template class ELFBinaryWriterImpl<ELF32LE>;
+template class ELFBinaryWriterImpl<ELF32BE>;
----------------
I think using this technique is most justified by larger code (like what you'll have later) so I'm cool keeping these but at this size it feels like it could just all go in a header.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:50
+template <class ELFT>
+Error writeELFBinary(const ELFStub &Stub, char *Buf, size_t Size) {
+  ELFBinaryWriterImpl<ELFT> Writer;
----------------
Seems like this can be inlined.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:71
+/// This class will NOT contain any state.
+template <class ELFT> class ELFBinaryWriterImpl {
+public:
----------------
The whole 'Writer' thing is supposed to allow for a base class to exist so you don't have to know what Writer you're using, but nothing like that is happening here. If we're not doing anything like that do you think we could just make these all functions?


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:73
+public:
+  using Elf_Ehdr = typename ELFT::Ehdr;
+
----------------
This can be private or if you turn these into functions, you can make them static in the defining object.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:77-89
+  /// Write an ELF binary to a buffer.
+  /// @param Stub Source ELFStub used to generate a binary stub.
+  /// @param Buf Target buffer to write to.
+  /// @param Size Size of target buffer.
+  Error writeToBuffer(const ELFStub &Stub, char *Buf, size_t Size);
+
+  /// This function calculates what the size of an ELF binary stub generated
----------------
Seems like you might be able to simplify this to just return a buffer of some kind.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:97
+  /// @param Machine Target architecture (e_machine from ELF specifications).
+  void initELFHeader(Elf_Ehdr &ElfHeader, uint16_t Machine);
+};
----------------
This can be private.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:78
+  std::unique_ptr<FileOutputBuffer> Buf = std::move(*BufOrError);
+  char *Data = reinterpret_cast<char *>(Buf->getBufferStart());
+  Error BinaryWriteError =
----------------
Use uint8_t instead of char for raw binary data. llvm doesn't even use full C++11 but in C++17 we'll use std::byte


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:94
+// Determine the appropriate ELFType and begin writing binary stub output.
+static Error writeBinaryWithTarget(StringRef FilePath, ELFStub &Stub) {
+  if (BinaryOutputTarget.getNumOccurrences() == 0) {
----------------
I think it would be nicer if this took the output format as a parameter.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:102
+  StringRef Target(BinaryOutputTarget);
+  if (Target.equals("elf32-little")) {
+    return writeELFBinary<ELF32LE>(FilePath, Stub);
----------------
Use == and don't construct a StringRef.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55839