[ELF][MIPS] Factor out SumVA adjustments into a couple of separate functions. NFC.

[atanasyan]

Summary

The patch does not reduce the size of the code but makes InputSectionBase::relocate cleaner a bit.

[grimar]

Generally this looks good to me, have few notes though.

ELF/InputSection.cpp

219 ↗

(On Diff #50524)

Is that correct ? Originally addend was added:

 SymVA = Out<ELFT>::Got->getMipsLocalFullAddr(Body) + A;
 }
}
else {
 SymVA = Body.getGotVA<ELFT>() + A;

290 ↗

(On Diff #50524)

I don't like this place because it has "if (Config->EMachine == EM_MIPS)"
I would do next:

1. rename static uintX_t getMipsGotVA -> static uintX_t getGotVA
2. remove uintX_t SymVA argument, but add uintX_t A

and do:

template <class ELFT, class uintX_t>
static uintX_t getGotVA(const SymbolBody &Body, uintX_t A,
                            uint8_t *BufLoc, uint8_t *PairedLoc) {

if (Config->EMachine != EM_MIPS)
 return Body.getGotVA<ELFT>() + A;
...

else if (Target->needsGot(Type, Body)) {
  SymVA = getGotVA<ELFT>(Body, A, BufLoc, PairedLoc);
...

[ruiu]

Since this is a refactoring patch to split the function, LGTM.

But I really want you and all other people who are working on MIPS to not only add code to the support current MIPS ABI but also make efforts to fix the ABI. Some parts of the ABI are very odd and sometimes even broken (e.g. the mixed endian header field), and supporting it sometimes really complicates stuff (like the paired relocations.)

ELF/InputSection.cpp

290 ↗

(On Diff #50524)

I prefer the other way. This function would not have been written if MIPS ABI were not so weird. I want to keep the code in the form that you can ignore all function calls including "mips" if you are not interested in MIPS. If you move that code into the function, you have to jump to that function and read that function only to know that that was basically separated for MIPS oddity.

[atanasyan]

Thanks for review.

The oddity of MIPS ABI is obvious to everybody. Probably the ABI will be changed gradually or by release completely new ABI but anyway it is a long process.

ELF/InputSection.cpp

219 ↗

(On Diff #50524)

It is correct for 32-bit MIPS because in that case we use REL format and have to extract addend from the buffer insider the relocateOne routine. The A is always zero. But I will return + A expressions before commit the patch to escape unnecessary changes.

lld/trunk/ELF/InputSection.cpp

for (const RelTy *RI = Rel; RI != End; ++RI) {
     uintX_t Offset = getOffset(RI->r_offset);
     if (Offset == (uintX_t)-1)
       return nullptr;
     return Buf + Offset;
   }
   return nullptr;
 }
 
+template <class ELFT, class uintX_t>
+static uintX_t adjustMipsSymVA(uint32_t Type, const ObjectFile<ELFT> &File,
+                               const SymbolBody &Body, uintX_t AddrLoc,
+                               uintX_t SymVA) {
+  if (Type == R_MIPS_HI16 && &Body == Config->MipsGpDisp)
+    return getMipsGpAddr<ELFT>() - AddrLoc;
+  if (Type == R_MIPS_LO16 && &Body == Config->MipsGpDisp)
+    return getMipsGpAddr<ELFT>() - AddrLoc + 4;
+  if (&Body == Config->MipsLocalGp)
+    return getMipsGpAddr<ELFT>();
+  if (Body.isLocal() && (Type == R_MIPS_GPREL16 || Type == R_MIPS_GPREL32))
+    // We need to adjust SymVA value in case of R_MIPS_GPREL16/32
+    // relocations because they use the following expression to calculate
+    // the relocation's result for local symbol: S + A + GP0 - G.
+    return SymVA + File.getMipsGp0();
+  return SymVA;
+}
+
+template <class ELFT, class uintX_t>
+static uintX_t getMipsGotVA(const SymbolBody &Body, uintX_t SymVA,
+                            uint8_t *BufLoc, uint8_t *PairedLoc) {
+  if (Body.isLocal()) {
+    // If relocation against MIPS local symbol requires GOT entry, this entry
+    // should be initialized by 'page address'. This address is high 16-bits
+    // of sum the symbol's value and the addend. The addend in that case is
+    // calculated using addends from R_MIPS_GOT16 and paired R_MIPS_LO16
+    // relocations.
+    const endianness E = ELFT::TargetEndianness;
+    uint64_t AHL = read32<E>(BufLoc) << 16;
+    if (PairedLoc)
+      AHL += SignExtend64<16>(read32<E>(PairedLoc));
+    return Out<ELFT>::Got->getMipsLocalPageAddr(SymVA + AHL);
+  }
+  if (!canBePreempted(Body))
+    // For non-local symbols GOT entries should contain their full
+    // addresses. But if such symbol cannot be preempted, we do not
+    // have to put them into the "global" part of GOT and use dynamic
+    // linker to determine their actual addresses. That is why we
+    // create GOT entries for them in the "local" part of GOT.
+    return Out<ELFT>::Got->getMipsLocalFullAddr(Body);
+  return Body.getGotVA<ELFT>();
+}
+
 template <class ELFT>
 template <class RelTy>
 void InputSectionBase<ELFT>::relocate(uint8_t *Buf, uint8_t *BufEnd,
                                       iterator_range<const RelTy *> Rels) {
   size_t Num = Rels.end() - Rels.begin();
   for (size_t I = 0; I < Num; ++I) {
     const RelTy &RI = *(Rels.begin() + I);
     uintX_t Offset = getOffset(RI.r_offset);
for (size_t I = 0; I < Num; ++I) {
     bool CBP = canBePreempted(Body);
     uint8_t *PairedLoc = nullptr;
     if (Config->EMachine == EM_MIPS)
       PairedLoc = findMipsPairedReloc(Buf, &RI, Rels.end());
 
     if (Target->needsPlt<ELFT>(Type, Body)) {
       SymVA = Body.getPltVA<ELFT>() + A;
     } else if (Target->needsGot(Type, Body)) {
-      if (Config->EMachine == EM_MIPS && !CBP) {
-        if (Body.isLocal()) {
-          // R_MIPS_GOT16 relocation against local symbol requires index of
-          // a local GOT entry which contains page address corresponds
-          // to sum of the symbol address and addend. The addend in that case
-          // is calculated using addends from R_MIPS_GOT16 and paired
-          // R_MIPS_LO16 relocations.
-          const endianness E = ELFT::TargetEndianness;
-          uint64_t AHL = read32<E>(BufLoc) << 16;
-          if (PairedLoc)
-            AHL += SignExtend64<16>(read32<E>(PairedLoc));
-          SymVA = Out<ELFT>::Got->getMipsLocalPageAddr(SymVA + AHL);
-        } else {
-          // For non-local symbols GOT entries should contain their full
-          // addresses. But if such symbol cannot be preempted, we do not
-          // have to put them into the "global" part of GOT and use dynamic
-          // linker to determine their actual addresses. That is why we
-          // create GOT entries for them in the "local" part of GOT.
-          SymVA = Out<ELFT>::Got->getMipsLocalFullAddr(Body) + A;
-        }
-      } else {
+      if (Config->EMachine == EM_MIPS)
+        SymVA = getMipsGotVA<ELFT>(Body, SymVA, BufLoc, PairedLoc) + A;
+      else
         SymVA = Body.getGotVA<ELFT>() + A;
-      }
       if (Body.IsTls)
         Type = Target->getTlsGotRel(Type);
     } else if (Target->isSizeRel(Type) && CBP) {
       // A SIZE relocation is supposed to set a symbol size, but if a symbol
       // can be preempted, the size at runtime may be different than link time.
       // If that's the case, we leave the field alone rather than filling it
       // with a possibly incorrect value.
       continue;
     } else if (Config->EMachine == EM_MIPS) {
-      if (Type == R_MIPS_HI16 && &Body == Config->MipsGpDisp) {
-        SymVA = getMipsGpAddr<ELFT>() - AddrLoc + A;
-      } else if (Type == R_MIPS_LO16 && &Body == Config->MipsGpDisp) {
-        SymVA = getMipsGpAddr<ELFT>() - AddrLoc + 4 + A;
-      } else if (&Body == Config->MipsLocalGp) {
-        SymVA = getMipsGpAddr<ELFT>() + A;
-      } else if (Type == R_MIPS_GPREL16 || Type == R_MIPS_GPREL32) {
-        // We need to adjust SymVA value in case of R_MIPS_GPREL16/32
-        // relocations because they use the following expression to calculate
-        // the relocation's result for local symbol: S + A + GP0 - G.
-        SymVA += File->getMipsGp0();
-      }
+      SymVA = adjustMipsSymVA<ELFT>(Type, *File, Body, AddrLoc, SymVA) + A;
     } else if (!Target->needsCopyRel<ELFT>(Type, Body) && CBP) {
       continue;
     }
     uintX_t Size = Body.getSize<ELFT>();
     Target->relocateOne(BufLoc, BufEnd, Type, AddrLoc, SymVA, Size + A,
                         PairedLoc);
   }
 }