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.
Details
Diff Detail
Event Timeline
llvm/include/llvm/BinaryFormat/ELF.h | ||
---|---|---|
315 | This doesn't look registered? https://www.sco.com/developers/gabi/latest/ch4.eheader.html |
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. Unfortunately the website has been unmaintained for a few years now. | |
llvm/test/Object/CSKY/elf-flags.yaml | ||
2 | llvm/unittests/Object/ELFObjectFileTest.cpp may be more suitable. We don't need to teach llvm-readobj about every EM_* value. |
I think http://lists.llvm.org/pipermail/llvm-dev/2020-August/144587.html may answer your concern. @MaskRay
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 | ||
2 | Good, thank you. I will add a unittest at ELFObjectFileTest. And I think I can keep elf-flags.yaml for target flags check later. |
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.
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
llvm/include/llvm/BinaryFormat/ELFRelocs/CSKY.def | ||
---|---|---|
10 | Please be consistent. All other *.def files have a different style. 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/lib/Object/ELF.cpp | ||
205 | I think we do not have unit tests for getRelativeRelocationType, |
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? |
llvm/include/llvm/BinaryFormat/ELFRelocs/CSKY.def | ||
---|---|---|
75 |
This should be fixed (the same below). | |
llvm/test/Object/CSKY/elf-flags.yaml | ||
2 | I think you can remove this test case, it doesn't do any useful job right now. aside:
| |
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. |
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.
This doesn't look registered? https://www.sco.com/developers/gabi/latest/ch4.eheader.html