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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-elfabi/invalid-bin-target.test:10
+
+#CHECK: llvm-elfabi: for the -output-target option: Cannot find option named 'nope'!
----------------
Super nit: Space between # and CHECK. Same applies to other tests.


================
Comment at: llvm/test/tools/llvm-elfabi/write-elf32be-ehdr.test:10-12
+#CHECK:      Format: ELF32-x86-64
+#CHECK-NEXT: Arch: x86_64
+#CHECK-NEXT: AddressSize: 32bit
----------------
I believe this information is derived from the ElfHeader, so there's not much point in testing it, since you already test it below when testing the ElfHeader itself.

Same applies to other tests.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:194
 
+/// This function calculates what the size a binary ELF stub will be.
+/// `Stub` is used to determine the exact binary size. This calculation includes
----------------
"calculates what the size" -> "calculates the size"


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:242
+  ElfHeader.e_ehsize = sizeof(Elf_Ehdr);
+}
+
----------------
You should probably set the e_shentsize and e_phentsize fields too, since those are constant.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:271
+template <class ELFT>
+static Error writeELFBinaryToFile(StringRef FilePath, ELFStub &Stub) {
+  // Open file for writing.
----------------
Same comment as above. It seems weird for `Stub` to be a non-const reference.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:283
+    return createStringError(errc::invalid_argument, Stream.str().c_str());
+  }
+  // Write binary to file.
----------------
Add a blank line here.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:287-288
+  MutableArrayRef<uint8_t> BufRef(Buf->getBufferStart(), Buf->getBufferSize());
+  Error BinaryWriteError = writeELFBinaryToBuffer<ELFT>(Stub, BufRef);
+  if (BinaryWriteError) {
+    return BinaryWriteError;
----------------
You can do these two lines in one, and lose the braces too, i.e:

```
if (Error BinaryWriteError = writeELFBinaryToBuffer<ELFT>(Stub, BufRef))
  return BinaryWriteError;
```


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:292-293
+
+  Error FileWriteError = Buf->commit();
+  if (FileWriteError) {
+    return FileWriteError;
----------------
Ditto.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:39
+/// This function determines appropriate ELFType using the passed ELFTarget and
+/// then begins the process of writing a binary ELF stub.
+///
----------------
"begins the process"? Sounds to me like this function should do the whole process, given its name.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:44
+/// @param OutputFormat Target ELFType to write binary as.
+Error writeBinaryStub(StringRef FilePath, ELFStub &Stub,
+                      ELFTarget OutputFormat);
----------------
It seems odd to me that Stub is a non-const reference. I wouldn't expect this function to modify it based on the current description.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:146
+  }
   // Write out .tbe file.
   if (EmitTBE.getNumOccurrences() == 1) {
----------------
It probably reads easier if there's a new line between each if block.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55839