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

[Andrei Safronov via Phabricator via llvm-commits]

andreisfr added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:776
+  // Four-bit Xtensa machine type field.
+  EF_XTENSA_MACH = 0x0000000f,
+  // Various CPU types.
----------------
jhenderson wrote:
> Just trying to understand this: is this a mask for some other information? Perhaps worth saying that in the comment above if so (e.g. "Four-bit Xtensa machine type mask").
Comment is corrected


================
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)
----------------
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.


================
Comment at: llvm/test/Object/obj2yaml.test:667
+# ELF-XTENSA-NEXT:   Machine:         EM_XTENSA
+# ELF-XTENSA-NEXT:   Flags:           [ EF_XTENSA_MACH_BASE, EF_XTENSA_XT_LIT ]
+
----------------
jhenderson wrote:
> andreisfr wrote:
> > jhenderson wrote:
> > > It looks weird to me seeing a difference in the inptut Flags and printed Flags. Is EF_XTENSA_MACH_BASE a marker (like SHT_LORESERVE etc) or an actual flag? If it's only a marker, we shouldn't dump it.
> > The situation is that EF_XTENSA_MACH_NONE (EF_XTENSA_MACH_BASE) defines default architecture, this flag equals 0x00  and it printed even in case of empty input flags set. Similar situation can be observed in AMDGPU tests, for example in llvm\test\Object\AMDGPU\elf-header-flags-xnack.yaml flag EF_AMDGPU_MACH_NONE. But I added this flag for input flags in Xtensa test to avoid confusion.
> > it printed even in case of empty input flags set.
> 
> Is printed by what? GNU readelf?
> 
> I think not having it in the input flags would be fine, if this line is accompanied by a comment saying something like "As EF_XTENSA_MACH_NONE == 0, it is always printed."
 The EF_XTENSA_MACH_NONE removed from input flags and added comment which explains why EF_XTENSA_MACH_NONE is printed by obj2yaml.


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

https://reviews.llvm.org/D64827