[PATCH] D115859: [LoongArch 2/5] Add ELF machine flag and relocs for upcoming LoongArch target

[Lu Weining via Phabricator via llvm-commits]

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