[PATCH] D64827: [Xtensa 2/10] Add Xtensa ELF definitions.

Fri Aug 19 11:38:29 PDT 2022

jrtc27 added inline comments.

================
Comment at: llvm/include/llvm/BinaryFormat/ELFRelocs/Xtensa.def:11
+ELF_RELOC (R_XTENSA_RELATIVE, 5)
+ELF_RELOC (R_XTENSA_PLT, 6)
+ELF_RELOC (R_XTENSA_OP0, 8)
----------------
saugustine wrote:
> andreisfr wrote:
> > jhenderson wrote:
> > > andreisfr wrote:
> > > > appcs wrote:
> > > > > Just making sure a '7' RELOC is not missing.
> > > > It is missing also in binutils (binutils/include/elf/xtensa.h). So I left it as is.
> > > Is there an xtensa specification anywhere?
> > There is some description of the Xtensa relocations in GNU Binutils ( in binutils/bfd/doc/bfd.info file and source code). As I understand most of ELF relocations for other architectures in LLVM are imported from binutils, so the same approach was used for Xtensa.
> To the best of my knowledge Xtensa does not have a formal elf specification. They definitely didn't a in the years I worked there. The binutils implementation governed. 
> 
> My recollection is very hazy almost twenty years later, but I believe 7 was used in some experiments during the original port and we later determined it wasn't useful. But by the time we removed it, the other assignments had already been made.
> 
> I think instead of leaving it unused and blank, we should use something like:
> 
> ELF_RELOC(R_XTENSA_RESERVED_7,          7)
> 
> which will better show that this isn't a mistake. and probably what we should have done originally.
I disagree, such constants achieve nothing, as nothing should be using them. Just put a comment if you want to make it clear it’s deliberately not assigned a name here.

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

https://reviews.llvm.org/D64827