[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/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;
----------------
jakehehrlich wrote:
> 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.
> 
> 
The issue in llvm-objcopy was that it was copying an existing file and discarding the EI_OSABI and EI_ABIVERSION. Certainly it might be useful in the future to be able to say what the value should be, but as this is not currently required, I think it can be delayed until there's a request for it.

On the other hand, converting a binary into a text file then back into a binary should probably support this at some point.


================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:257
+
+  // remainder of ELF header
+  ElfHeader.e_type = ET_DYN;
----------------
Nit: this should start with a capital letter and end with a full stop.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:151-152
     if (TBEWriteError) {
       WithColor::error() << TBEWriteError << "\n";
       exit(1);
     }
----------------
You might want to consider factoring these out into a single function that takes an `Error`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55839