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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:90-91
+  ElfHeader.e_version = EV_CURRENT;
+  ElfHeader.e_entry = 0x00;
+  ElfHeader.e_flags = 0u;
+  ElfHeader.e_ehsize = sizeof(Elf_Ehdr);
----------------
0x00 and 0u are all just 0, so I'd just write "0". Sign-extending 0 yields just 0, so "u" is redundant.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.h:71
+/// This class will NOT contain any state.
+template <class ELFT> class ELFBinaryWriterImpl {
+public:
----------------
jakehehrlich wrote:
> 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?
I agree with Jake that because this can be done with functions it's probably better to do this using functions without a class. In addition to that, it looks like a "Impl" class that doesn't inherit any class a bit weird, because usually an "Impl" class implements an abstract interface of some other class (I'm not suggesting you define an abstract class and an implementation class, but just pointing out that the current name is perhaps not that good.)


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:185-186
+  if (BinaryOutputFilePath.getNumOccurrences() == 1) {
+    Error BinaryWriteError =
+        writeBinaryWithTarget(BinaryOutputFilePath, *TargetStub);
+    if (BinaryWriteError) {
----------------
It might be discussed before, but what is a value of returning an Error from a function and then print that error & exit from the main function? Propagating an error all the way to the main function makes function signatures more complex (any function that can fail or calls a function that can fail has to have a function signature of ErrorOr<T> instead of just T). If this is just a command, then maybe just printing out an error message and exit is better?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55839