Page MenuHomePhabricator

[ELF] Add a new e_machine value EM_CSKY and add some CSKY relocation types
ClosedPublic

Authored by zixuan-wu on Wed, Aug 26, 4:30 AM.

Details

Summary

This is the split part of D86269, which add a new ELF machine flag called EM_CSKY and related relocations.
Some target-specific flags and tests for csky can be added in follow-up patches later.

Diff Detail

Event Timeline

zixuan-wu created this revision.Wed, Aug 26, 4:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Aug 26, 4:30 AM
zixuan-wu requested review of this revision.Wed, Aug 26, 4:30 AM
arsenm added inline comments.Wed, Aug 26, 12:00 PM
llvm/include/llvm/BinaryFormat/ELF.h
315

This looks good to me, but I guess the target may need an official approval (from LLVM Foundation Board of Directors?) first.
From very few reports on c-sky I can find on the Internet, the v1 and v2 of the chip csky uses the "c-sky" architecture. v3 uses RISC-V. I am a bit worrisome whether this organization backing the target will commit to maintain for a sufficiently long time in LLVM. (If you look at existing lib/Target/* targets, many have been there for 10 years. For the younger ones, I think there are commitments from the companies).

llvm/include/llvm/BinaryFormat/ELF.h
315

See the thread "Machine Request - 65816/65c816" in the generic-abi google group.
binutils has the definition
https://sourceware.org/legacy-ml/binutils/2018-05/msg00242.html
and the value is known by several folks on generic-abi.

Unfortunately the website has been unmaintained for a few years now.

llvm/test/Object/CSKY/elf-flags.yaml
1 ↗(On Diff #287915)

llvm/unittests/Object/ELFObjectFileTest.cpp may be more suitable. We don't need to teach llvm-readobj about every EM_* value.

zixuan-wu added a comment.EditedWed, Aug 26, 8:37 PM
llvm/include/llvm/BinaryFormat/ELF.h
315

I find the page https://www.sco.com/developers/gabi/latest/ch4.eheader.html is out-of-date and have been unmaintained. You can check https://groups.google.com/g/generic-abi/c/qaPzp2lRzDA?pli=1.

And the binutils have contained this EM_CSKY.

llvm/test/Object/CSKY/elf-flags.yaml
1 ↗(On Diff #287915)

Good, thank you. I will add a unittest at ELFObjectFileTest. And I think I can keep elf-flags.yaml for target flags check later.

This looks good to me, but I guess the target may need an official approval (from LLVM Foundation Board of Directors?) first.

I don't think it's necessary to hold up this patch on anything like that. It would be reasonable to define the reserved enum values independently of the target

This looks good to me, but I guess the target may need an official approval (from LLVM Foundation Board of Directors?) first.

I don't think it's necessary to hold up this patch on anything like that. It would be reasonable to define the reserved enum values independently of the target

The test ELFObjectFileTest.cpp depends on the Triple::csky so it would be follow-up of triple patch if that patch is representing the target acceptance.

MaskRay added inline comments.Tue, Sep 1, 9:25 PM
llvm/include/llvm/BinaryFormat/ELFRelocs/CSKY.def
7

For relocation type changes, we have tests in test/tools/llvm-readobj/ELF/reloc-types-elf-*.test. I don't know replicating every value is useful (you'll probably just copy and paste the value anyway), but having a few should not hurt.

[ELF] Add new EM called EM_CSKY

Add a new e_machine value EM_CSKY and add some CSKY relocation types

zixuan-wu retitled this revision from [ELF] Add new EM called EM_CSKY to [ELF] Add a new e_machine value EM_CSKY and add some CSKY relocation types.Tue, Sep 1, 11:22 PM
grimar added inline comments.Wed, Sep 2, 1:56 AM
llvm/include/llvm/BinaryFormat/ELFRelocs/CSKY.def
10

Please be consistent. All other *.def files have a different style.
E.g:

ELF_RELOC(R_390_NONE,          0)
ELF_RELOC(R_390_8,             1)
ELF_RELOC(R_390_12,            2)
ELF_RELOC(R_390_16,            3)
ELF_RELOC(R_390_32,            4)
...
llvm/include/llvm/Object/ELFObjectFile.h
1116

This value should be tested in
llvm\unittests\Object\ELFObjectFileTest.cpp

llvm/lib/Object/ELF.cpp
205

I think we do not have unit tests for getRelativeRelocationType,
but you probably can add the first one (for EM_CSKY) to ELFObjectFileTest.cpp to test this value.

zixuan-wu updated this revision to Diff 289387.Wed, Sep 2, 3:33 AM

Address comments.

zixuan-wu marked 3 inline comments as done.Wed, Sep 2, 3:34 AM
zixuan-wu added inline comments.Wed, Sep 2, 7:30 PM
llvm/include/llvm/BinaryFormat/ELFRelocs/CSKY.def
7

Sorry to know what your meaning less. Should I add test under test/tools/llvm-readobj/ELF/reloc-types-elf-*.test?

grimar added inline comments.Thu, Sep 3, 12:48 AM
llvm/include/llvm/BinaryFormat/ELFRelocs/CSKY.def
75

No newline at end of file

This should be fixed (the same below).

llvm/test/Object/CSKY/elf-flags.yaml
1 ↗(On Diff #287915)

I think you can remove this test case, it doesn't do any useful job right now.

aside:

  1. it calls obj2yaml, but test/Object/CSKY/ is not the right place for obj2yaml testing.
  2. EM_AMDGPU flags printed by llvm-readobj are tested in llvm-readobj\ELF\amdgpu-elf-headers.test, though perhaps we should test flags in ELFObjectFileTest.cpp instead.
llvm/unittests/Object/ELFObjectFileTest.cpp
296

No full stop at the end of comment.

297

I'd make it generic. I.e. remove "ForCSKY". Then we can add other EM_* types here.

zixuan-wu updated this revision to Diff 289653.Thu, Sep 3, 1:10 AM

Address comments.

zixuan-wu marked 4 inline comments as done.Thu, Sep 3, 1:16 AM
grimar accepted this revision.Thu, Sep 3, 1:37 AM

I have no more comment, thanks! Lets see what others think.

This revision is now accepted and ready to land.Thu, Sep 3, 1:37 AM

I have no more comment, thanks! Lets see what others think.

Even if this had high chance to be accepted, I think it needs an official approval (even for this binary format patch). For an unknown arch, supporting EM_* is fine, but not R_*_RELATIVE and its list of relocation types.

There is no need to revert, though.