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

Tue May 26 01:02:44 PDT 2020

jhenderson added a comment.

Sorry for the delay in responding - I ran out of time on Friday, and then it was a public holiday yesterday, so I wasn't working. Mostly, this looks fine. I just think the ELFObjectFIle unit tests still has some issues, which are described inline and below. Once you've resolve those, I think this is good to go.

I'm not a fan of writing out the ELF data byte-by-byte as you have done in the ELFObjectFile test - I think it can be simplified somewhat, and made more flexible/less prone to being broken by endianness issues etc. ELF already has the Ehdr structures, which you can use to populate the data below, rather than individually assigning each byte. Here's what I would recommend to solve these issues (note - code is untested, so don't blindly copy, but should give you a good indication of what I think you should do):

  // You might want to cosider storing Encoding in the struct, to make using this function easier.
  template <typename T> T swap(T Value, uint8_t Encoding) {
    assert(Encoding == ELF::ELFDATA2MSB || Encoding == ELF::ELFDATA2LSB);
    bool IsLittleEndian == ELF::ELFDATA2LSB;
    if (IsLittleEndianHost != IsLittleEndian)
      return sys::swapByteOrder(Value);
    return Value;
  }

  DataForTest(uint8_t Class, uint8_t Encoding, uint16_t Machine) {
    if (Class == ELF::ELFCLASS64)
      Data = makeElfData<ELF::Elf64_Ehdr>(Class, Encoding, Machine);
    else {
      assert(Class == ELF::ELFCLASS32);
      Data = makeElfData<ELF::Elf64_Ehdr>(Class, Encoding, Machine);
    }
  }

  template <typename T>
  std::vector<uint8_t> makeElfData(uint8_t Class, uint8_t Encoding, uint16_t Machine) {
    T Ehdr {}; // Zero-initialise the header.
    Ehdr.e_ident[ELF::EI_MAG0] = 0x7f;
    Ehdr.e_ident[ELF::EI_MAG1] = 'E';
    Ehdr.e_ident[ELF::EI_MAG2] = 'L';
    Ehdr.e_ident[ELF::EI_MAG3] = 'F';
    Ehdr.e_ident[ELF::EI_CLASS] = Class;
    Ehdr.e_ident[ELF::EI_DATA] = Encoding;
    Ehdr.e_ident[ELF::EI_VERSION] = 1;
    Ehdr.e_type = swap(ELF::ET_REL, Encoding);
    Ehdr.e_machine = swap(Machine, Encoding);
    Ehdr.e_version = swap(uint16_t(1), Encoding);
    Ehdr.e_ehsize = swap(uint16_t(sizeof(T)), Encoding);
    uint8_t *EhdrBytes = reinterpret_cast<uint8_t *>(&Ehdr);
    std::vector<uint8_t> Bytes;
    std::copy(EhdrBytes, EhdrBytes + sizeof(Ehdr), std::back_inserter(Bytes));
    return Bytes;
  }

================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1464
   ENUM_ENT(EM_BPF,           "EM_BPF"),
+  ENUM_ENT(EM_VE,            "NEC SX-Aurora Vector Engine"),
 };
----------------
kaz7 wrote:
> kaz7 wrote:
> > jhenderson wrote:
> > > kaz7 wrote:
> > > > jhenderson wrote:
> > > > > 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]]
> > > > > ```
> > > > > 
> > > > > 
> > > > > 
> > > > I've made a test and found a problem that EM_ECOG1 and EM_ECOG1X uses identical number 168.  Let me know what I should do for this.
> > > Just test what is printed for one of them. It might be a bug in LLVM, or a gABI bug or something similar. Speaking of the gABI, have you been officially assigned a value for `EM_VE` using the ELF gABI mailing list?
> > Ok.  I test onl EM_ECOG1.  Tests for ECOG1X is commented out in above test with FIXME tag.
> > 
> > Regarding to gABI, I don't know.  I guess EM_VE is officially assigned since EM_VE is used in NEC's official compiler systems like below.  I tried to check it out, but I didn't find the way to do so unfortunately.
> > 
> > https://github.com/veos-sxarr-NEC/musl-libc-ve/blob/master/include/elf.h#L220
> FYI, I've checked registry status internally.  And I heard that it is registered through registry at sco.com.
Great, good to hear. When I was referring to clang-formatting things, I was specifically referring to the new files you've added, rather than this enum, which I'm not convinced we should reformat. Could you undo the formatting changes to this bit, please?

================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:27
+
+  DataForTest(uint8_t Class, uint8_t Encoding, uint16_t Machine) : Data() {
+    // Swap byte order of Machine
----------------
No need to explicitly initialise `Data`.

================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:30
+    if (Encoding == ELF::ELFDATA2MSB)
+      sys::swapByteOrder(Machine);
+
----------------
If I'm reading the code correctly, this will unconditionally swap bytes for big endian inputs, regardless of the host endianness. If the host machine is big endian, you don't want to swap. There are build bots out there which are big endian, so you need to handle that case, or the test will fail on those.

See my out-of-line comment for a possible solution.

================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:36
+    if (Class == ELF::ELFCLASS64) {
+      Data.assign({
+          0x7f,     'E',      'L', 'F',             // Magic
----------------
I don't think assigning each byte individually is a good way to go. See my out-of-line comment for an alternative.

================
Comment at: llvm/unittests/Object/ELFTest.cpp:57
+}

----------------
No need for an additional blank line here at the end of the file.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79545