[PATCH] D115859: [LoongArch 2/5] Add ELF machine flag and relocs for upcoming LoongArch target
Lu Weining via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 20 03:32:59 PST 2021
SixWeining added a comment.
Thanks for your comments @xen0n and I will update the revision to address them.
================
Comment at: llvm/include/llvm/BinaryFormat/ELFRelocs/LoongArch.def:5
+
+// Based on the psABI: https://loongson.github.io/LoongArch-Documentation/LoongArch-ELF-ABI-EN.html
+// Adoption in binutils: include/elf/loongarch.h
----------------
xen0n wrote:
> It may be better to point to the documentation source code with exact commit hash, because unfortunately there seems to be no versioning in the rendered website, so future revisions to the documentation could render this comment out-dated.
>
> Also minor nit: it should probably say "from the LoongArch ELF psABI", both because the values are not modified here downstream, and to fully spell out the documentation title.
Thanks. That's reasonable and I will change it.
================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:292
+TEST(ELFObjectFileTest, MachineTestForLOONGARCH) {
+ std::array<StringRef, 4> Formats = {"elf32-loongarch", "elf32-loongarch",
----------------
xen0n wrote:
> `MachineTestForLoongArch` for proper casing?
OK, I will change it.
================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:293
+TEST(ELFObjectFileTest, MachineTestForLOONGARCH) {
+ std::array<StringRef, 4> Formats = {"elf32-loongarch", "elf32-loongarch",
+ "elf64-loongarch", "elf64-loongarch"};
----------------
xen0n wrote:
> From my cursory look, it seems the 2nd and 4th elements are for verifying big-endian cases, yet LoongArch is little-endian-only according to documentation. So do you want to ignore big-endian cases and fix here to say `elfNN-unknown` instead?
Yes, LoongArch is little-endian only. But I think it doesn't mean LoongArch should only support identifing little-endian ELF files. For example, on little endian x86, the llvm-readelf tool can even recognize big-endian ELF file.
```
$ arch
x86_64
$ cat a.yaml
--- !ELF
FileHeader:
Class: ELFCLASS64
Data: ELFDATA2MSB
Type: ET_REL
Machine: EM_X86_64
$ yaml2obj a.yaml -o a.o
$ llvm-readelf -h a.o
ELF Header:
Magic: 7f 45 4c 46 02 02 01 00 00 00 00 00 00 00 00 00
Class: ELF64
Data: 2's complement, big endian
Version: 1 (current)
OS/ABI: UNIX - System V
ABI Version: 0
Type: REL (Relocatable file)
Machine: Advanced Micro Devices X86-64
Version: 0x1
...
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115859/new/
https://reviews.llvm.org/D115859
More information about the llvm-commits
mailing list