Page MenuHomePhabricator

[Xtensa 2/10] Add Xtensa ELF definitions.
Needs ReviewPublic

Authored by andreisfr on Jul 16 2019, 3:18 PM.

Details

Summary

Add file with Xtensa ELF relocations. Add Xtensa support to ELF.h,
ELFObject.h and ELFYAML.cpp. Add simple test of Xtensa ELF representation in YAML.

Diff Detail

Event Timeline

andreisfr created this revision.Jul 16 2019, 3:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2019, 3:18 PM
appcs added a reviewer: appcs.Jul 16 2019, 3:55 PM
appcs added inline comments.Jul 16 2019, 3:59 PM
llvm/include/llvm/BinaryFormat/ELF.h
775

Typo? (Just making sure it isn't).

llvm/include/llvm/BinaryFormat/ELFRelocs/Xtensa.def
11

Just making sure a '7' RELOC is not missing.

andreisfr marked 2 inline comments as done.Jul 17 2019, 3:09 PM
andreisfr added inline comments.
llvm/include/llvm/BinaryFormat/ELF.h
775

I took as a basis naming conventions of the e_flags from binutils (binutils/include/elf/xtensa.h). There used similar names for the architecture mask and base machine architecture with only differenece in prefix (E_ or EF_).

llvm/include/llvm/BinaryFormat/ELFRelocs/Xtensa.def
11

It is missing also in binutils (binutils/include/elf/xtensa.h). So I left it as is.

aykevl added a subscriber: aykevl.Sat, Aug 17, 5:07 PM

LGTM, but I know very little about the LLVM backend.
Also, I see that this patch lacks one change that are included by the very similar RISC-V patch: https://github.com/lowRISC/riscv-llvm/blob/master/0003-RISCV-Add-RISC-V-ELF-defines.patch, namely the equivalent of ElfHeaderRISCVFlags in ELFDumper.cpp. I don't know whether that has a reason but mentioning it just in case.

llvm/include/llvm/BinaryFormat/ELF.h
775

Looking at https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=include/elf/msp430.h for comparison, they use EF_<arch>_MACH for the mask and E_<arch>_MACH_<sub> for what seems to be the sub-architecture. I see a very similar pattern for AVR. In both cases, LLVM still uses the EF_ prefix for the subarch.
For consistency with the other architectures in LLVM, it might be better to use something like EF_XTENSA_MACH_BASE. However, I have no idea why these constants are given these names in LLVM or binutils.

MaskRay added inline comments.
llvm/include/llvm/BinaryFormat/ELF.h
772

Use // as the rest of the file uses.

llvm/include/llvm/BinaryFormat/ELFRelocs/Xtensa.def
1

Add a test test/tools/llvm-readobj/reloc-types-elf-xtensa.test

llvm/test/Object/Xtensa/elf-flags.yaml
1

Delete this file. Add to test/Object/obj2yaml.test instead. You may follow the ELF-X86-64: yaml2obj+obj2yaml test.

llvm/test/Object/Xtensa/lit.local.cfg
3

Delete this file.