[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:
> 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.
My understanding is the following:

E.g. `Elf_Shdr consists of `Elf_Word`/`Elf_Addr`/`Elf_Off` fields,
which are of `support::detail::packed_endian_specific_integral` type:


```
using Elf_Shdr_Base<ELFT>::sh_size;
  
template <endianness TargetEndianness>
struct Elf_Shdr_Base<ELFType<TargetEndianness, false>> {
  LLVM_ELF_IMPORT_TYPES(TargetEndianness, false)
  Elf_Word sh_name;      // Section name (index into string table)
  Elf_Word sh_type;      // Section type (SHT_*)
  Elf_Word sh_flags;     // Section flags (SHF_*)
  Elf_Addr sh_addr;      // Address where section is to be loaded
  Elf_Off sh_offset;     // File offset of section data, in bytes
  Elf_Word sh_size;      // Size of section, in bytes
  Elf_Word sh_link;      // Section type-specific header table index link
  Elf_Word sh_info;      // Section type-specific extra information
  Elf_Word sh_addralign; // Section address alignment
  Elf_Word sh_entsize;   // Size of records contained within the section
};

#define LLVM_ELF_IMPORT_TYPES_ELFT(ELFT)                                       \
  using Elf_Addr = typename ELFT::Addr;                                        \
  using Elf_Off = typename ELFT::Off;                                          \
  using Elf_Half = typename ELFT::Half;                                        \
  using Elf_Word = typename ELFT::Word;                                        \
  using Elf_Sword = typename ELFT::Sword;                                      \
  using Elf_Xword = typename ELFT::Xword;                                      \
  using Elf_Sxword = typename ELFT::Sxword;

  using Half = packed<uint16_t>;
  using Word = packed<uint32_t>;
  using Sword = packed<int32_t>;
  using Xword = packed<uint64_t>;
  using Sxword = packed<int64_t>;
  using Addr = packed<uint>;
  using Off = packed<uint>;
 
  template <typename Ty>
  using packed = support::detail::packed_endian_specific_integral<Ty, E, 1>;
```
 
This type respects endianess on assign:


```
struct packed_endian_specific_integral {
....
  void operator=(value_type newValue) {
    endian::write<value_type, endian, alignment>(
      (void*)Value.buffer, newValue);
  }

/// Write a value to memory with a particular endianness.
template <typename value_type, std::size_t alignment>
inline void write(void *memory, value_type value, endianness endian) {
  value = byte_swap<value_type>(value, endian);
  memcpy(LLVM_ASSUME_ALIGNED(
             memory, (detail::PickAlignment<value_type, alignment>::value)),
         &value, sizeof(value_type));
}

template <typename value_type>
inline value_type byte_swap(value_type value, endianness endian) {
  if ((endian != native) && (endian != system_endianness()))
    sys::swapByteOrder(value);
  return value;
}
```


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:384
 
-  return createStringError(errc::not_supported, "Unsupported binary format");
 }
----------------
jhenderson wrote:
> 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...?
Oh. I missed that there is `|| InputFileFormat == FileFormat::ELF`, not `&& InputFileFormat == FileFormat::ELF`.
This place looks a bit wierd to me..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61767