[llvm-objcopy] Fix handling of MIPS64 little endian files

[atanasyan]

Summary

MIPS64 little endian target has a "special" encoding of r_info relocation record field. Instead of one 64-bit little endian number, it is a little endian 32-bit number followed by a 32-bit big endian number. For correct reading and writing such fields we must provide information about target machine into the corresponding routine. This patch does this for the llvm-objcopy tool and fix handling of MIPS64 little endian files.

The patch looks ugly but I could not figure out how to hide the nature of MIPS64 ABI better.

The bug was reported in the issue #52647.

[jhenderson]

Since the "Mips64EL" property is a propert of the input object, I wonder if we should just make it a member of the object, and then put a reference to that object in the relocation sections, so that we can query that, rather than needing to a) pass around the variable, and b) have to have the property in the relocation section? What do you think?

llvm/test/tools/llvm-objcopy/ELF/mips64el.test

5–7

I'd name the input and output objects differently for the two cases, so that we don't overwrite the first case with the second case. This is sometimes useful for test debugging.

11–14

We usually get rid of excessive spacing within a block, as per the suggested edit. Same below.

18–19

I'd be surprised if you need either of these fields.

20

I'd replace this with a Size field, if the size is important to the test case. If it isn't important, consider just getting rid of the field entirely.

24–25

These fields are redundant (either they're the default, or simply not needed).

37–42

Do you need this second symbol? It doesn't seem to be being used.

llvm/tools/llvm-objcopy/ELF/Object.h

799–800

[atanasyan]

Since the "Mips64EL" property is a property of the input object, I wonder if we should just make it a member of the object,
and then put a reference to that object in the relocation sections, so that we can query that, rather than needing
to a) pass around the variable, and b) have to have the property in the relocation section? What do you think?

Agreed.

[atanasyan]

• Reduce redundancy of the test case
• Add IsMips64 field to the Object class.
• Save reference to an Object in the RelocationSection class.

[nathanchance]

This resolves the Linux kernel build error, thank you for the quick fix!

[MaskRay]

LG

llvm/test/tools/llvm-objcopy/ELF/mips64.test

1 ↗

(On Diff #393936)

Add a comment like ## mips64el has a special encoding of the r_info relocation field. Test that we support both endianness.

[jhenderson]

Looks good, with @MaskRay's comment.

llvm/test/tools/llvm-objcopy/ELF/mips64.test

1 ↗

(On Diff #393936)

I assume that yaml2obj does actually generate the r_info correctly in this case? Easiest way to tell is to make sure the test fails without this fix, if you're not sure.

[atanasyan]

Thanks for review.

llvm/test/tools/llvm-objcopy/ELF/mips64el.test

+# RUN: yaml2obj %s -DENDIANNESS=ELFDATA2LSB -o %t
+# RUN: llvm-objcopy %t %t2
+# RUN: llvm-readobj --relocations %t2 | FileCheck %s
+
+# RUN: yaml2obj %s -DENDIANNESS=ELFDATA2MSB -o %t
+# RUN: llvm-objcopy %t %t2
+# RUN: llvm-readobj --relocations %t2 | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            [[ENDIANNESS]]
+  Type:            ET_REL
+  Machine:         EM_MIPS
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x10
+    Content:         0000000000000000
+  - Name:            .rela.text
+    Type:            SHT_RELA
+    Flags:           [ SHF_INFO_LINK ]
+    Link:            .symtab
+    AddressAlign:    0x8
+    Info:            .text
+    Relocations:
+      - Offset:          8
+        Symbol:          foo
+        Type:            R_MIPS_JALR
+Symbols:
+  - Name:            foo
+    Type:            STT_FUNC
+    Section:         .text
+    Binding:         STB_GLOBAL
+    Size:            8
+  - Name:            bar
+    Type:            STT_FUNC
+    Section:         .text
+    Binding:         STB_GLOBAL
+    Value:           8
+    Size:            8
+
+# CHECK:      Relocations [
+# CHECK-NEXT:   Section (2) .rela.text {
+# CHECK-NEXT:     0x8 R_MIPS_JALR/R_MIPS_NONE/R_MIPS_NONE foo 0x0
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]

llvm/tools/llvm-objcopy/ELF/Object.h

public:
   void finalize() override;
 };
 
 class RelocationSection
     : public RelocSectionWithSymtabBase<SymbolTableSection> {
   MAKE_SEC_WRITER_FRIEND
 
   std::vector<Relocation> Relocations;
+  bool IsObjMips64EL = false;
 
 public:
   void addRelocation(Relocation Rel) { Relocations.push_back(Rel); }
   Error accept(SectionVisitor &Visitor) const override;
   Error accept(MutableSectionVisitor &Visitor) override;
   Error removeSectionReferences(
       bool AllowBrokenLinks,
       function_ref<bool(const SectionBase *)> ToRemove) override;
   Error removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override;
   void markSymbols() override;
   void replaceSectionReferences(
       const DenseMap<SectionBase *, SectionBase *> &FromTo) override;
+  bool IsMips64EL() const { return IsObjMips64EL; }
+  void SetMips64EL(bool value) { IsObjMips64EL = value; }
 
   static bool classof(const SectionBase *S) {
     if (S->OriginalFlags & ELF::SHF_ALLOC)
       return false;
     return S->OriginalType == ELF::SHT_REL || S->OriginalType == ELF::SHT_RELA;
   }
 };
 

llvm/tools/llvm-objcopy/ELF/Object.cpp

 static void setAddend(Elf_Rel_Impl<ELFT, false> &, uint64_t) {}
 
 template <class ELFT>
 static void setAddend(Elf_Rel_Impl<ELFT, true> &Rela, uint64_t Addend) {
   Rela.r_addend = Addend;
 }
 
 template <class RelRange, class T>
-static void writeRel(const RelRange &Relocations, T *Buf) {
+static void writeRel(const RelRange &Relocations, T *Buf, bool IsMips64EL) {
   for (const auto &Reloc : Relocations) {
     Buf->r_offset = Reloc.Offset;
     setAddend(*Buf, Reloc.Addend);
     Buf->setSymbolAndType(Reloc.RelocSymbol ? Reloc.RelocSymbol->Index : 0,
-                          Reloc.Type, false);
+                          Reloc.Type, IsMips64EL);
     ++Buf;
   }
 }
 
 template <class ELFT>
 Error ELFSectionWriter<ELFT>::visit(const RelocationSection &Sec) {
   uint8_t *Buf = reinterpret_cast<uint8_t *>(Out.getBufferStart()) + Sec.Offset;
   if (Sec.Type == SHT_REL)
-    writeRel(Sec.Relocations, reinterpret_cast<Elf_Rel *>(Buf));
+    writeRel(Sec.Relocations, reinterpret_cast<Elf_Rel *>(Buf),
+             Sec.IsMips64EL());
   else
-    writeRel(Sec.Relocations, reinterpret_cast<Elf_Rela *>(Buf));
+    writeRel(Sec.Relocations, reinterpret_cast<Elf_Rela *>(Buf),
+             Sec.IsMips64EL());
   return Error::success();
 }
 
 Error RelocationSection::accept(SectionVisitor &Visitor) const {
   return Visitor.visit(*this);
 }
 
 Error RelocationSection::accept(MutableSectionVisitor &Visitor) {

 
 template <class ELFT>
 static void getAddend(uint64_t &ToSet, const Elf_Rel_Impl<ELFT, true> &Rela) {
   ToSet = Rela.r_addend;
 }
 
 template <class T>
 static Error initRelocations(RelocationSection *Relocs,
-                             SymbolTableSection *SymbolTable, T RelRange) {
+                             SymbolTableSection *SymbolTable, T RelRange,
+                             bool IsMips64EL) {
+  Relocs->SetMips64EL(IsMips64EL);
   for (const auto &Rel : RelRange) {
     Relocation ToAdd;
     ToAdd.Offset = Rel.r_offset;
     getAddend(ToAdd.Addend, Rel);
-    ToAdd.Type = Rel.getType(false);
+    ToAdd.Type = Rel.getType(IsMips64EL);
 
-    if (uint32_t Sym = Rel.getSymbol(false)) {
+    if (uint32_t Sym = Rel.getSymbol(IsMips64EL)) {
       if (!SymbolTable)
         return createStringError(
             errc::invalid_argument,
             "'" + Relocs->Name + "': relocation references symbol with index " +
                 Twine(Sym) + ", but there is no symbol table");
       Expected<Symbol *> SymByIndex = SymbolTable->getSymbolByIndex(Sym);
       if (!SymByIndex)
         return SymByIndex.takeError();
if (auto RelSec = dyn_cast<RelocationSection>(&Sec)) {
       const typename ELFFile<ELFT>::Elf_Shdr *Shdr =
           Sections->begin() + RelSec->Index;
       if (RelSec->Type == SHT_REL) {
         Expected<typename ELFFile<ELFT>::Elf_Rel_Range> Rels =
             ElfFile.rels(*Shdr);
         if (!Rels)
           return Rels.takeError();
 
-        if (Error Err = initRelocations(RelSec, Obj.SymbolTable, *Rels))
+        if (Error Err = initRelocations(RelSec, Obj.SymbolTable, *Rels,
+                                        ElfFile.isMips64EL()))
           return Err;
       } else {
         Expected<typename ELFFile<ELFT>::Elf_Rela_Range> Relas =
             ElfFile.relas(*Shdr);
         if (!Relas)
           return Relas.takeError();
 
-        if (Error Err = initRelocations(RelSec, Obj.SymbolTable, *Relas))
+        if (Error Err = initRelocations(RelSec, Obj.SymbolTable, *Relas,
+                                        ElfFile.isMips64EL()))
           return Err;
       }
     } else if (auto GroupSec = dyn_cast<GroupSection>(&Sec)) {
       if (Error Err = initGroupSection(GroupSec))
         return Err;
     }
   }