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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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:
> > 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.
You never answered this question:

> Is printed by what? GNU readelf?

>From what I piece together from the comments and flag names, EF_XTENSA_MACH_NONE is effectively saying "there are no other EF_XTENSA_MACH_* bits sets". Given that, I would expect to NOT see this flag in obj2yaml or llvm-readobj output, if any of the bits covered by the EF_XTENSA_MACH mask are set. This of course might require some extra logic.



================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-types-xtensa.test:2
+## Test that llvm-readobj/llvm-readelf shows proper relocation type
+## names and values for xtensa target.
+
----------------
for the Xtensa


================
Comment at: llvm/test/tools/llvm-readobj/ELF/reloc-types-xtensa.test:5
+# RUN: yaml2obj %s -o %t-xtensa.o
+# RUN: llvm-readobj -r --expand-relocs %t-xtensa.o | FileCheck %s
+
----------------
Your comment says both llvm-readobj and llvm-readelf, but you only test one of those. As the code is generic, that's fine, but probably best to remove the llvm-readelf reference from the comment.


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

https://reviews.llvm.org/D64827