[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/ELFRelocs/Xtensa.def:1
+#ifndef ELF_RELOC
+#error "ELF_RELOC must be defined"
----------------
jhenderson wrote:
> (Not specific to your change, but I don't understand why .def files don't seem to have any licence information)
I also noticed that and I thought that this is some kind of coding style and decided to follow it.


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


================
Comment at: llvm/test/Object/obj2yaml.test:675
+  Machine:         EM_XTENSA
+  Flags:           [ EF_XTENSA_XT_LIT ]
+
----------------
jhenderson wrote:
> This should specify all EF_XTENSA flags, to show that they all work.
Added rest Xtensa ELF flags


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

https://reviews.llvm.org/D64827