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

Wed Aug 24 18:12:21 PDT 2022

andreisfr 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)
----------------
jrtc27 wrote:
> 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.
Added comment about RELOC '7'

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

https://reviews.llvm.org/D64827