[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:
> > > > 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.
> > 
> The EF_XTENSA_MACH_NONE is printed only by obj2yaml and it is printed only in case when ((ELF_FLAGS & EF_XTENSA_MACH) == 0 ). I checked this situation using dummy architecture flag  like (EF_XTENSA_ARCH_1 = 0x1) and verified that obj2yaml prints only EF_XTENSA_ARCH_1 without EF_XTENSA_MACH_NONE.
> 
> I'm not sure that we must add Xtensa ELF flags to llvm-readobj, because it seems that in ELFDumper.cpp only RISCV / AMDGPU / Mips ELF flags are processed.
> ...  and verified that obj2yaml prints only EF_XTENSA_ARCH_1 without EF_XTENSA_MACH_NONE.

Thanks.

> I'm not sure that we must add Xtensa ELF flags to llvm-readobj, because it seems that in ELFDumper.cpp only RISCV / AMDGPU / Mips ELF flags are processed.

This seems like a circular argument to me. Surely it only supports those other flags, because Xtensa flags haven't been implemented in it yet?

FWIW, adding support should probably be a separate change, but please do implement that patch, as otherwise I consider this functionality incomplete.


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

https://reviews.llvm.org/D64827