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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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.
----------------
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 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)
----------------
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?


================
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 ]
+
----------------
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."


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

https://reviews.llvm.org/D64827