[PATCH] D61767: [llvm-elfabi] Emit ELF header and string table section
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 12 02:41:11 PST 2020
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
More information about the llvm-commits
mailing list