Add file with Xtensa ELF relocations. Add Xtensa support to ELF.h,
ELFObject.h and ELFYAML.cpp. Add simple test of Xtensa ELF representation in YAML.
You suggest to create separate obj2yaml test for Xtensa?
There is no risk in yaml2obj picking the wrong yaml block. You can use the --docnum=<index> switch to specify which one to use, where <index> indicates which doc to use (e.g. --docnum=1 for the first, --docnum=2 for the second etc). See various other examples in the llvm-readobj tests for how.
There should be an llvm-readobj test for the xtensa file header (machine type + flags).
(Not specific to your change, but I don't understand why .def files don't seem to have any licence information)
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.
This should specify all EF_XTENSA flags, to show that they all work.
I also noticed that and I thought that this is some kind of coding style and decided to follow 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.
Added rest Xtensa ELF flags
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").
Is there an xtensa specification anywhere?
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."
Comment is corrected
There is some description of the Xtensa relocations in GNU Binutils ( in binutils/bfd/doc/bfd.info file and source code). As I understand most of ELF relocations for other architectures in LLVM are imported from binutils, so the same approach was used for Xtensa.
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:
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.
for the Xtensa
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.
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.
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.
|3 ↗||(On Diff #210194)|
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?
I don't see any testing for this here?
|1 ↗||(On Diff #328692)|
Please add a comment to this file explaining what the test is testing.
|4 ↗||(On Diff #329497)|
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.
|6 ↗||(On Diff #329497)|
obj2yaml testing should be done in test\tools\obj2yaml. There's no need for it here too.
|3 ↗||(On Diff #210194)|
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.
|7 ↗||(On Diff #329497)|
We normally add spaces to make the checked values line up neatly, as suggested inline.
|1 ↗||(On Diff #243466)|
"elf" is still in the file name. Please remove it.
|6 ↗||(On Diff #329497)|
I'm agree, probably obj2yaml testing of the ELF flags is even redundant because it already implemented in llvm/test/Object/obj2yaml.test.
@jhenderson I suggest to implement "llvm/test/tools/llvm-readobj/ELF/xtensa-elf-flags.test" test instead of "llvm/test/Object/Xtensa/elf-header-flags.yaml". What do you think about this?
|1 ↗||(On Diff #333474)|
Rename this to xtensa-header-flags.test. This should hopefully make it clear that these are flags for the ELF header, without the redundant elf in the filename.
|3–7 ↗||(On Diff #333474)|
Use a different filename for the files. That way, if the test breaks, you can easily compare the two files. This is generally very useful for debugging.
I'd also drop ELF- from the prefixes, since they don't add any value.
I've suggested a possible name inline.
Are you referring to the yaml2obj line or the use of --docnum? All other files use pre-canned binaries, which are not ideal, as they are fixed and hard to inspect/understand/update. Using yaml2obj is generally preferable for creating objects at test time.
Oh you're right. Hm. --docnum=N for each architecture would start to get a bit silly though... I do wonder why we don't have one .test file for each architecture being tested, then you don't need to play --docnum or Inputs/foo.yaml games (and even for pre-canned binaries it keeps the tests nicely separated rather than having an 800-odd line file).
Still looks good to me, but someone with Xtensa knowledge needs to approve to confirm the flags etc are correct.
That would be reasonable. I think it's a legacy of how this test was written. In fact, having an obj2yaml test inside the Object library tests is a bit weird - it should either be in ObjectYAML or more likely tools\obj2yaml. Feel free to tidy things up!
(Apologies, I wrote this commenet ages ago, but forgot to submit it).