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

[George Rimar via Phabricator via llvm-commits]

grimar 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;
+  }
----------------
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?


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:384
 
-  return createStringError(errc::not_supported, "Unsupported binary format");
 }
----------------
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());
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61767