[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/Object/ELFObjectFile.h:1173-1174
       return "elf32-amdgpu";
+    case ELF::EM_XTENSA:
+      return "elf32-xtensa";
     default:
----------------
I don't see any testing for this here?


================
Comment at: llvm/test/Object/Xtensa/elf-header-flags.yaml:4
+# RUN: obj2yaml %t.o.1 | FileCheck --check-prefixes=YAML-INSN %s
+# RUN: yaml2obj --docnum=2 %s -o %t.o.2
+# RUN: llvm-readobj -S --file-headers %t.o.2 | FileCheck --check-prefixes=ELF-ALL,ELF-LIT %s
----------------
You can avoid having a second YAML DOC by using yaml2obj's -D option, which allows you to define a variable. See other yaml2obj examples in various tests.


================
Comment at: llvm/test/Object/Xtensa/elf-header-flags.yaml:6
+# RUN: llvm-readobj -S --file-headers %t.o.2 | FileCheck --check-prefixes=ELF-ALL,ELF-LIT %s
+# RUN: obj2yaml %t.o.2 | FileCheck --check-prefixes=YAML-LIT %s
+
----------------
`obj2yaml` testing should be done in `test\tools\obj2yaml`. There's no need for it here too.


================
Comment at: llvm/test/Object/Xtensa/elf-header-flags.yaml:1
+# RUN: yaml2obj --docnum=1 %s -o %t.o.1
+# RUN: llvm-readobj -S --file-headers %t.o.1 | FileCheck --check-prefixes=ELF-ALL,ELF-INSN %s
----------------
Please add a comment to this file explaining what the test is testing.


================
Comment at: llvm/test/Object/Xtensa/lit.local.cfg:3
+    config.unsupported = True
+
----------------
andreisfr wrote:
> MaskRay wrote:
> > Delete this file.
> I suggest to add llvm/test/Object/Xtensa/elf-header-flags.yaml and llvm/test/Object/Xtensa/li.local.cfg to test llvm-readobj. What is your opinion on this?
The ability to check the ELF flags does not require the Xtensa target to be enabled. You only need to mark such tests as unsupported if they actually do require the enabling of that target, otherwise more people won't run the tests and there's a higher chance they'll break due to somebody's change, without them noticing.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-types-elf-xtensa.test:7
+
+# CHECK: 0x0 R_XTENSA_NONE - 0x0
+# CHECK-NEXT: 0x4 R_XTENSA_32 - 0x0
----------------
We normally add spaces to make the checked values line up neatly, as suggested inline.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-types-elf-xtensa.test:1
+## Test that llvm-readobj/llvm-readelf shows proper relocation type
+## names and values for xtensa target.
----------------
jhenderson wrote:
> No need for "elf" to be in the file name, since it's in the ELF subdirectory.
"elf" is still in the file name. Please remove it.


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

https://reviews.llvm.org/D64827