[PATCH] D79545: [VE] Implements minimum MC layer for VE (3/4)

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/ELFRelocs/VE.def:36
+ELF_RELOC(R_VE_DTPOFF64,     23)
+// ELF_RELOC(R_VE_TPOFF64,     24)
+ELF_RELOC(R_VE_TLS_GD_HI32,  25)
----------------
Why are some commented out?


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:1142-1143
       return "elf64-bpf";
+    case ELF::EM_VE:
+      return "elf64-ve";
     default:
----------------
Test coverage?


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:1222-1223
 
+  case ELF::EM_VE:
+    return Triple::ve;
+
----------------
Test coverage?


================
Comment at: llvm/lib/Object/ELF.cpp:148-154
+  case ELF::EM_VE:
+    switch (Type) {
+#include "llvm/BinaryFormat/ELFRelocs/VE.def"
+    default:
+      break;
+    }
+    break;
----------------
Please add an llvm-readobj test "reloc-types-elf-ve.test" to test/tools/llvm-readobj/ELF similar to the existing "reloc-types-elf-*.test" files for other machines.


================
Comment at: llvm/lib/Object/ELF.cpp:201-202
     break;
+  case ELF::EM_VE:
+    return ELF::R_VE_RELATIVE;
   default:
----------------
Test coverage?


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:666-668
+  case ELF::EM_VE:
+#include "llvm/BinaryFormat/ELFRelocs/VE.def"
+    break;
----------------
There should be some yaml2obj/obj2yaml testing to show that VE relocation enums are supported in those tools.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1464
   ENUM_ENT(EM_BPF,           "EM_BPF"),
+  ENUM_ENT(EM_VE,            "NEC SX-Aurora Vector Engine"),
 };
----------------
It looks like this enum is currently essentially untested. Could you add a simple test case to llvm-readobj's tests to cover file header printing of the different machine types, please?

Here's a rough example of what I'm thinking of:
```
# RUN: yaml2obj %s -o %t.none -D MACHINE=EM_NONE
# RUN: llvm-readelf --file-headers %t.none | FileCheck %s -D MACHINE=None --check-prefix=GNU

# RUN: yaml2obj %s -o %t.m32 -D MACHINE=EM_M32
# RUN: llvm-readelf --file-headers %t.m32 | FileCheck %s -D MACHINE=WE32100

<repeat for each value>

# CHECK: Machine: [[MACHINE]]

--- !ELF
FileHeader:
  Class:   ELFCLASS32
  Data:    ELFDATA2LSB
  Type:    ET_REL
  Machine: [[MACHINE]]
```





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79545