[PATCH] D61767: [llvm-elfabi] Emit ELF header and string table section

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:179
+  template <class T> static void write(uint8_t *Data, const T &Value) {
+    *reinterpret_cast<T *>(Data) = Value;
+  }
----------------
grimar wrote:
> jhenderson wrote:
> > This line makes me very nervous that this code will fail miserably where the host endianness doesn't match the target endianness with non-single byte `T` types.
> It is only used for writing `Elf_Shdr`, `Elf_Ehdr` etc types, what I think should work fine on any hosts.
> Is your concern that somebody might try to use `write` for regular types?
We're probably talking past each other, or I might be forgetting something, but the fields within `Elf_Shdr`, `Elf_Ehdr` etc will be in little/big endian order right depending on the host, right? This code here is essentially just doing a byte-wise copy, by my understanding.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:384
 
-  return createStringError(errc::not_supported, "Unsupported binary format");
 }
----------------
grimar wrote:
> jhenderson wrote:
> > I'm concerned that this has been changed to `llvm_unreachable`. Is this function only able to be called with ELF binaries? What happens if a user passes in e.g. an archive or COFF file? `createBinary` can return more than just ELF types, which would lead to an assertion here. This case will want testing too. Also, it seem unrelated to this patch?
> I believe this code is unreachable because `readELFFile` is only called for ELFs:
> 
> ```
>   // First try to read as a binary (fails fast if not binary).
>   if (InputFileFormat.getNumOccurrences() == 0 ||
>       InputFileFormat == FileFormat::ELF) {
>     Expected<std::unique_ptr<ELFStub>> StubFromELF =
>         readELFFile(FileReadBuffer->getMemBufferRef());
> ```
`InputFileFormat.getNumOccurrences() == 0` - doesn't that mean an unspecified input format will be attempted to be read as ELF, but there's no guarantee it actually is ELF...?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61767