Page MenuHomePhabricator

[Xtensa 2/10] Add Xtensa ELF definitions.
AcceptedPublic

Authored by andreisfr on Jul 16 2019, 3:18 PM.

Details

Summary

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Patch is updated according to latest upstream version. The E_XTENSA_MACH_BASE symbol changed to EF_XTENSA_MACH_BASE.

andreisfr updated this revision to Diff 243466.Feb 9 2020, 4:46 PM

Add Xtensa ELF relocations test. Move ELF flags test from "llvm/test/Object/Xtensa/" directory to test/Object/obj2yaml.test.

andreisfr marked 3 inline comments as done and 2 inline comments as done.Feb 9 2020, 4:51 PM
andreisfr added inline comments.
llvm/include/llvm/BinaryFormat/ELF.h
775

Changed E_XTENSA_MACH to EF_XTENSA_MACH_BASE

andreisfr marked 5 inline comments as done.Feb 9 2020, 4:52 PM
jhenderson added inline comments.Feb 10 2020, 2:18 AM
llvm/test/Object/obj2yaml.test
734 ↗(On Diff #243466)

Rather than adding a pre-canned binary, use yaml2obj to generate a file.

llvm/test/tools/llvm-readobj/ELF/reloc-types-elf-xtensa.test
1 ↗(On Diff #243466)

No need for "elf" to be in the file name, since it's in the ELF subdirectory.

183 ↗(On Diff #243466)

Delete extra blank line here.

andreisfr updated this revision to Diff 244128.Feb 12 2020, 4:09 AM

Correction of the llvm/test/tools/ llvm-readobj/ELF/reloc-types-elf-xtensa.test file (renaming to reloc-types-xtensa.test, and also delete the empty line at the end of file).

andreisfr added inline comments.Feb 12 2020, 4:26 AM
llvm/test/Object/obj2yaml.test
734 ↗(On Diff #243466)

You suggest to create separate obj2yaml test for Xtensa?
Or add Xtensa ELF header yaml description in current file? I'm bother that there is already X86 ELF yaml description in this file, so yaml2obj could produce wrong result while processing obj2yaml.test. As I'm understand that is a reason why tests foir all other architectures in this file use pre-canned binaries.

jhenderson added inline comments.Feb 12 2020, 4:31 AM
llvm/test/Object/obj2yaml.test
734 ↗(On Diff #243466)

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.

Correction of the llvm/test/Object/obj2yaml.test, now Xtensa test uses yaml2obj instead of pre-built binary trivial-object-test.elf-xtensa.

andreisfr marked an inline comment as done.Feb 12 2020, 12:49 PM
andreisfr added inline comments.
llvm/test/Object/obj2yaml.test
734 ↗(On Diff #243466)

Now Xtensa test uses yaml2obj instead of pre-built binary

andreisfr marked an inline comment as not done.Feb 12 2020, 1:04 PM

There should be an llvm-readobj test for the xtensa file header (machine type + flags).

llvm/include/llvm/BinaryFormat/ELFRelocs/Xtensa.def
2

(Not specific to your change, but I don't understand why .def files don't seem to have any licence information)

llvm/test/Object/obj2yaml.test
667 ↗(On Diff #244250)

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.

675 ↗(On Diff #244250)

This should specify all EF_XTENSA flags, to show that they all work.

andreisfr updated this revision to Diff 244900.Feb 16 2020, 3:05 PM

Change EF_XTENSA_MACH_BASE to EF_XTENSA_MACH_NONE, it seems this is closer to ELF flag naming style used for AMDGPU and MIPS.

andreisfr updated this revision to Diff 244903.Feb 16 2020, 3:50 PM

Correct Xtensa ELF flag test

andreisfr added inline comments.Feb 16 2020, 4:12 PM
llvm/include/llvm/BinaryFormat/ELFRelocs/Xtensa.def
2

I also noticed that and I thought that this is some kind of coding style and decided to follow it.

llvm/test/Object/obj2yaml.test
667 ↗(On Diff #244250)

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.

675 ↗(On Diff #244250)

Added rest Xtensa ELF flags

jhenderson added inline comments.Feb 18 2020, 2:39 AM
llvm/include/llvm/BinaryFormat/ELF.h
773

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").

llvm/include/llvm/BinaryFormat/ELFRelocs/Xtensa.def
11

Is there an xtensa specification anywhere?

llvm/test/Object/obj2yaml.test
667 ↗(On Diff #244250)

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."

andreisfr updated this revision to Diff 245366.Feb 19 2020, 3:20 AM

Correction of comments and Xtensa ELF flags test.

andreisfr added inline comments.Feb 19 2020, 3:44 AM
llvm/include/llvm/BinaryFormat/ELF.h
773

Comment is corrected

llvm/include/llvm/BinaryFormat/ELFRelocs/Xtensa.def
11

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.

llvm/test/Object/obj2yaml.test
667 ↗(On Diff #244250)

The EF_XTENSA_MACH_NONE removed from input flags and added comment which explains why EF_XTENSA_MACH_NONE is printed by obj2yaml.

jhenderson added inline comments.Feb 24 2020, 7:58 AM
llvm/test/Object/obj2yaml.test
667 ↗(On Diff #244250)

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.

llvm/test/tools/llvm-readobj/ELF/reloc-types-xtensa.test
2 ↗(On Diff #245366)

for the Xtensa

5 ↗(On Diff #245366)

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.

andreisfr updated this revision to Diff 247387.Feb 28 2020, 4:04 PM

Correct relocations test for the Xtensa target

andreisfr marked 3 inline comments as done.Feb 28 2020, 4:04 PM
andreisfr added inline comments.
llvm/test/tools/llvm-readobj/ELF/reloc-types-xtensa.test
5 ↗(On Diff #245366)

Corrected

andreisfr marked an inline comment as done.Feb 28 2020, 4:25 PM
andreisfr added inline comments.
llvm/test/Object/obj2yaml.test
667 ↗(On Diff #244250)

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.

MaskRay added inline comments.Feb 28 2020, 6:01 PM
llvm/test/tools/llvm-readobj/ELF/reloc-types-xtensa.test
5 ↗(On Diff #245366)

Delete --expand-relocs and you will be able to use CHECK-NEXT: instead of CHECK:.

jhenderson added inline comments.Mar 2 2020, 12:54 AM
llvm/test/Object/obj2yaml.test
667 ↗(On Diff #244250)

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

andreisfr updated this revision to Diff 328692.Mar 5 2021, 4:39 PM

Patch is updated according to latest upstream version. Added llvm-readobj test.

andreisfr marked an inline comment as done.Mar 5 2021, 4:45 PM
andreisfr added inline comments.
llvm/test/Object/obj2yaml.test
667 ↗(On Diff #244250)

Added Xtensa ELF flags to llvm-readobj, also implemented test

llvm/test/tools/llvm-readobj/ELF/reloc-types-elf-xtensa.test
183 ↗(On Diff #243466)

Corrected

andreisfr updated this revision to Diff 329497.Mar 9 2021, 4:48 PM

Patch is updated according to LLVM upstream version and latest Xtensa backend version.

andreisfr marked 2 inline comments as done.Mar 9 2021, 4:57 PM
andreisfr added inline comments.Mar 9 2021, 5:01 PM
llvm/test/Object/Xtensa/lit.local.cfg
3

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?

jhenderson added inline comments.Mar 19 2021, 2:54 AM
llvm/include/llvm/Object/ELFObjectFile.h
1079–1080

I don't see any testing for this here?

llvm/test/Object/Xtensa/elf-header-flags.yaml
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.

llvm/test/Object/Xtensa/lit.local.cfg
3

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.

llvm/test/tools/llvm-readobj/ELF/reloc-types-elf-xtensa.test
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.

andreisfr updated this revision to Diff 333474.Mar 25 2021, 5:34 PM

Corrected according to review comments

andreisfr marked 2 inline comments as done.Mar 25 2021, 5:40 PM
andreisfr added inline comments.
llvm/test/tools/llvm-readobj/ELF/reloc-types-elf-xtensa.test
1 ↗(On Diff #243466)

I changed file name to reloc-types-xtensa.test

andreisfr marked an inline comment as done.Mar 25 2021, 5:41 PM
andreisfr added inline comments.
llvm/include/llvm/Object/ELFObjectFile.h
1079–1080

I added test to llvm/unittests/Object/ELFObjectFileTest.cpp

andreisfr added inline comments.Mar 25 2021, 5:47 PM
llvm/test/Object/Xtensa/elf-header-flags.yaml
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?

jhenderson added inline comments.Mar 26 2021, 1:35 AM
llvm/test/tools/llvm-readobj/ELF/xtensa-elf-flags.test
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.

andreisfr updated this revision to Diff 333612.Mar 26 2021, 1:06 PM

Corrected llvm-readobj test

andreisfr added inline comments.Mar 26 2021, 1:12 PM
llvm/test/tools/llvm-readobj/ELF/xtensa-elf-flags.test
1 ↗(On Diff #333474)

File name is renamed to xtensa-header-flags.test.

3–7 ↗(On Diff #333474)

Thank you for suggestions, I corrected this code in xtensa-header-flags.test

jhenderson accepted this revision.Mar 29 2021, 12:10 AM

LGTM, with one test nit. You might want to give it a couple of days to see if any of the other reviewers have further comments to make.

llvm/test/tools/llvm-readobj/ELF/xtensa-header-flags.test
9–12 ↗(On Diff #333612)

No need for the extra indentation.

This revision is now accepted and ready to land.Mar 29 2021, 12:10 AM
MaskRay accepted this revision.Mar 29 2021, 12:15 AM
MaskRay added inline comments.
llvm/include/llvm/BinaryFormat/ELFRelocs/Xtensa.def
6

Don't use a space for macro calls.

andreisfr updated this revision to Diff 334835.Apr 1 2021, 3:15 PM

Removed spaces in macro calls in Xtensa.def. Also removed extra indentation in xtensa-header-flags.test.

andreisfr added inline comments.Apr 1 2021, 3:16 PM
llvm/include/llvm/BinaryFormat/ELFRelocs/Xtensa.def
6

Corrected, also aligned right parentheses

llvm/test/tools/llvm-readobj/ELF/xtensa-header-flags.test
9–12 ↗(On Diff #333612)

Corrected