[PATCH] D86610: [ELF] Add a new e_machine value EM_CSKY and add some CSKY relocation types

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/ELFRelocs/CSKY.def:75
+ELF_RELOC(R_CKCORE_PCREL_VLRW_IMM12_8,         68)
\ No newline at end of file

----------------
> No newline at end of file

This should be fixed (the same below).


================
Comment at: llvm/test/Object/CSKY/elf-flags.yaml:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj --file-headers %t | FileCheck -check-prefix=OBJ %s
----------------
zixuan-wu wrote:
> MaskRay wrote:
> > `llvm/unittests/Object/ELFObjectFileTest.cpp` may be more suitable. We don't need to teach llvm-readobj about every EM_* value.
> 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 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.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:296
+
+// ELF relative relocation type test
+TEST(ELFObjectFileTest, RelativeRelocationTypeTestForCSKY) {
----------------
No full stop at the end of comment.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:297
+// ELF relative relocation type test
+TEST(ELFObjectFileTest, RelativeRelocationTypeTestForCSKY) {
+  EXPECT_EQ(ELF::R_CKCORE_RELATIVE, getELFRelativeRelocationType(ELF::EM_CSKY));
----------------
I'd make it generic. I.e. remove "ForCSKY". Then we can add other EM_* types here.


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

https://reviews.llvm.org/D86610