[ELF] Move the outSecOff addend from relocAlloc/relocNonAlloc/... to InputSectionBase::relocate

[MaskRay]

Summary

For an InputSection, the buf argument of InputSectionBase::relocate points
to the content of the containing OutputSection, instead of the content of the
InputSection itself, so outSecOff needs to be added in its callees. This is
counter-intuitive and leads to many - outSecOff and + outSecOff.

This patch makes InputSection::writeTo call InputSectionBase::relocate with
outSecOff added. relocAlloc/relocNonAlloc/relocateNonAllocForRelocatable can
thus be simplified now.

Updated test:

• non-abs-reloc.s: A minor offset bug is fixed for a diagnostic in relocateNonAlloc

[psmith]

Simplification looks good to me. I'm not sure about the test changes as I would have expected the change to be NFC. If these are test improvements it will be worth altering separately.

The comment in InputSections.h says

// Each section knows how to relocate itself. These functions apply
// relocations, assuming that Buf points to this section's copy in
// the mmap'ed output buffer.
template <class ELFT> void relocate(uint8_t *buf, uint8_t *bufEnd);
void relocateAlloc(uint8_t *buf, uint8_t *bufEnd);

May be worth changing "Buf points to this section's copy in the mmap'ed output buffer" to "Buf points to the start of the copy of the InputSection's data in the mmap'ed output buffer."

[grimar]

In D85618#2205936, @psmith wrote:

Simplification looks good to me.

+1.

I'm not sure about the test changes as I would have expected the change to be NFC. If these are test improvements it will be worth altering separately.

+1

[MaskRay]

Address comments

[MaskRay]

lld/test/ELF/non-abs-reloc.s

5–6

This is the only behavior difference (a bug fix).

[psmith]

Thanks for the update, I'm happy when George is.

[grimar]

LGTM too.

lld/ELF/AArch64ErrataFix.cpp

 }
 
 void Patch843419Section::writeTo(uint8_t *buf) {
   // Copy the instruction that we will be replacing with a branch in the
   // patchee Section.
   write32le(buf, read32le(patchee->data().begin() + patcheeOffset));
 
   // Apply any relocation transferred from the original patchee section.
-  // For a SyntheticSection Buf already has outSecOff added, but relocateAlloc
-  // also adds outSecOff so we need to subtract to avoid double counting.
-  this->relocateAlloc(buf - outSecOff, buf - outSecOff + getSize());
+  relocateAlloc(buf, buf + getSize());
 
   // Return address is the next instruction after the one we have just copied.
   uint64_t s = getLDSTAddr() + 4;
   uint64_t p = patchSym->getVA() + 4;
   target->relocateNoSym(buf + 4, R_AARCH64_JUMP26, s - p);
 }
 
 void AArch64Err843419Patcher::init() {

lld/ELF/ARMErrataFix.cpp

 }
 
 void Patch657417Section::writeTo(uint8_t *buf) {
   // The base instruction of the patch is always a 32-bit unconditional branch.
   if (isARM)
     write32le(buf, 0xea000000);
   else
     write32le(buf, 0x9000f000);
-  // If we have a relocation then apply it. For a SyntheticSection buf already
-  // has outSecOff added, but relocateAlloc also adds outSecOff so we need to
-  // subtract to avoid double counting.
+  // If we have a relocation then apply it.
   if (!relocations.empty()) {
-    relocateAlloc(buf - outSecOff, buf - outSecOff + getSize());
+    relocateAlloc(buf, buf + getSize());
     return;
   }
 
   // If we don't have a relocation then we must calculate and write the offset
   // ourselves.
   // Get the destination offset from the addend in the branch instruction.
   // We cannot use the instruction in the patchee section as this will have
   // been altered to point to us!

lld/ELF/InputSection.cpp

for (const RelTy &rel : rels) {
 
     // GCC 8.0 or earlier have a bug that they emit R_386_GOTPC relocations
     // against _GLOBAL_OFFSET_TABLE_ for .debug_info. The bug has been fixed
     // in 2017 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82630), but we
     // need to keep this bug-compatible code for a while.
     if (config->emachine == EM_386 && type == R_386_GOTPC)
       continue;
 
-    uint64_t offset = getOffset(rel.r_offset);
+    uint64_t offset = rel.r_offset;
     uint8_t *bufLoc = buf + offset;
     int64_t addend = getAddend<ELFT>(rel);
     if (!RelTy::IsRela)
       addend += target->getImplicitAddend(bufLoc, type);
 
     Symbol &sym = getFile<ELFT>()->getRelocTargetSym(rel);
     RelExpr expr = target->getRelExpr(type, sym, bufLoc);
     if (expr == R_NONE)
if (expr != R_ABS && expr != R_DTPREL && expr != R_RISCV_ADD) {
       // If the control reaches here, we found a PC-relative relocation in a
       // non-ALLOC section. Since non-ALLOC section is not loaded into memory
       // at runtime, the notion of PC-relative doesn't make sense here. So,
       // this is a usage error. However, GNU linkers historically accept such
       // relocations without any errors and relocate them as if they were at
       // address 0. For bug-compatibilty, we accept them with warnings. We
       // know Steel Bank Common Lisp as of 2018 have this bug.
       warn(msg);
-      target->relocateNoSym(bufLoc, type,
-                            SignExtend64<bits>(sym.getVA(addend - offset)));
+      target->relocateNoSym(
+          bufLoc, type,
+          SignExtend64<bits>(sym.getVA(addend - offset - outSecOff)));
       continue;
     }
 
     if (tombstone ||
         (isDebug && (type == target->symbolicRel || expr == R_DTPREL))) {
       // Resolve relocations in .debug_* referencing (discarded symbols or ICF
       // folded section symbols) to a tombstone value. Resolving to addend is
       // unsatisfactory because the result address range may collide with a

 // for allocatable sections, and this function does the same for
 // non-allocatable sections, such as sections with debug information.
 static void relocateNonAllocForRelocatable(InputSection *sec, uint8_t *buf) {
   const unsigned bits = config->is64 ? 64 : 32;
 
   for (const Relocation &rel : sec->relocations) {
     // InputSection::copyRelocations() adds only R_ABS relocations.
     assert(rel.expr == R_ABS);
-    uint8_t *bufLoc = buf + rel.offset + sec->outSecOff;
+    uint8_t *bufLoc = buf + rel.offset;
     uint64_t targetVA = SignExtend64(rel.sym->getVA(rel.addend), bits);
     target->relocate(bufLoc, rel, targetVA);
   }
 }
 
 template <class ELFT>
 void InputSectionBase::relocate(uint8_t *buf, uint8_t *bufEnd) {
   if (flags & SHF_EXECINSTR)

 void InputSectionBase::relocateAlloc(uint8_t *buf, uint8_t *bufEnd) {
   assert(flags & SHF_ALLOC);
   const unsigned bits = config->wordsize * 8;
 
   for (const Relocation &rel : relocations) {
     if (rel.expr == R_NONE)
       continue;
     uint64_t offset = rel.offset;
-    if (auto *sec = dyn_cast<InputSection>(this))
-      offset += sec->outSecOff;
     uint8_t *bufLoc = buf + offset;
     RelType type = rel.type;
 
     uint64_t addrLoc = getOutputSection()->addr + offset;
+    if (auto *sec = dyn_cast<InputSection>(this))
+      addrLoc += sec->outSecOff;
     RelExpr expr = rel.expr;
     uint64_t targetVA = SignExtend64(
         getRelocTargetVA(file, type, rel.addend, addrLoc, *rel.sym, expr),
         bits);
 
     switch (expr) {
     case R_RELAX_GOT_PC:
     case R_RELAX_GOT_PC_NOPIC:
void InputSectionBase::relocateAlloc(uint8_t *buf, uint8_t *bufEnd) {
   }
 
   // Apply jumpInstrMods.  jumpInstrMods are created when the opcode of
   // a jmp insn must be modified to shrink the jmp insn or to flip the jmp
   // insn.  This is primarily used to relax and optimize jumps created with
   // basic block sections.
   if (auto *sec = dyn_cast<InputSection>(this)) {
     for (const JumpInstrMod &jumpMod : jumpInstrMods) {
-      uint64_t offset = jumpMod.offset + sec->outSecOff;
+      uint64_t offset = jumpMod.offset;
       uint8_t *bufLoc = buf + offset;
       target->applyJumpInstrMod(bufLoc, jumpMod.original, jumpMod.size);
     }
   }
 }
 
 // For each function-defining prologue, find any calls to __morestack,
 // and replace them with calls to __morestack_non_split.
if (Defined *d = dyn_cast<Defined>(rel.sym))
         if (!isec || !isec->getFile<ELFT>() || isec->getFile<ELFT>()->splitStack)
           continue;
 
     if (enclosingPrologueAttempted(rel.offset, prologues))
       continue;
 
     if (Defined *f = getEnclosingFunction<ELFT>(rel.offset)) {
       prologues.insert(f);
-      if (target->adjustPrologueForCrossSplitStack(buf + getOffset(f->value),
-                                                   end, f->stOther))
+      if (target->adjustPrologueForCrossSplitStack(buf + f->value, end,
+                                                   f->stOther))
         continue;
       if (!getFile<ELFT>()->someNoSplitStack)
         error(lld::toString(this) + ": " + f->getName() +
               " (with -fsplit-stack) calls " + rel.sym->getName() +
               " (without -fsplit-stack), but couldn't adjust its prologue");
     }
   }
 
template <class ELFT> void InputSection::writeTo(uint8_t *buf) {
   // to the buffer.
   if (uncompressedSize >= 0) {
     size_t size = uncompressedSize;
     if (Error e = zlib::uncompress(toStringRef(rawData),
                                    (char *)(buf + outSecOff), size))
       fatal(toString(this) +
             ": uncompress failed: " + llvm::toString(std::move(e)));
     uint8_t *bufEnd = buf + outSecOff + size;
-    relocate<ELFT>(buf, bufEnd);
+    relocate<ELFT>(buf + outSecOff, bufEnd);
     return;
   }
 
   // Copy section contents from source object file to output file
   // and then apply relocations.
   memcpy(buf + outSecOff, data().data(), data().size());
   uint8_t *bufEnd = buf + outSecOff + data().size();
-  relocate<ELFT>(buf, bufEnd);
+  relocate<ELFT>(buf + outSecOff, bufEnd);
 }
 
 void InputSection::replace(InputSection *other) {
   alignment = std::max(alignment, other->alignment);
 
   // When a section is replaced with another section that was allocated to
   // another partition, the replacement section (and its associated sections)
   // need to be placed in the main partition so that both partitions will be

lld/ELF/SyntheticSections.cpp

 
 bool GotSection::isNeeded() const {
   // We need to emit a GOT even if it's empty if there's a relocation that is
   // relative to GOT(such as GOTOFFREL).
   return numEntries || hasGotOffRel;
 }
 
 void GotSection::writeTo(uint8_t *buf) {
-  // Buf points to the start of this section's buffer,
-  // whereas InputSectionBase::relocateAlloc() expects its argument
-  // to point to the start of the output section.
   target->writeGotHeader(buf);
-  relocateAlloc(buf - outSecOff, buf - outSecOff + size);
+  relocateAlloc(buf, buf + size);
 }
 
 static uint64_t getMipsPageAddr(uint64_t addr) {
   return (addr + 0x8000) & ~0xffff;
 }
 
 static uint64_t getMipsPageCount(uint64_t size) {
   return (size + 0xfffe) / 0xffff + 1;
void ARMExidxSyntheticSection::writeTo(uint8_t *buf) {
   const uint8_t cantUnwindData[8] = {0, 0, 0, 0,  // PREL31 to target
                                      1, 0, 0, 0}; // EXIDX_CANTUNWIND
 
   uint64_t offset = 0;
   for (InputSection *isec : executableSections) {
     assert(isec->getParent() != nullptr);
     if (InputSection *d = findExidxSection(isec)) {
       memcpy(buf + offset, d->data().data(), d->data().size());
-      d->relocateAlloc(buf, buf + d->getSize());
+      d->relocateAlloc(buf + d->outSecOff, buf + d->outSecOff + d->getSize());
       offset += d->getSize();
     } else {
       // A Linker generated CANTUNWIND section.
       memcpy(buf + offset, cantUnwindData, sizeof(cantUnwindData));
       uint64_t s = isec->getVA();
       uint64_t p = getVA() + offset;
       target->relocateNoSym(buf + offset, R_ARM_PREL31, s - p);
       offset += 8;

lld/test/ELF/non-abs-reloc.s

 // REQUIRES: x86
 // RUN: split-file %s %t
 // RUN: llvm-mc -filetype=obj -triple=x86_64 %t/asm -o %t.o
 // RUN: ld.lld -T %t/lds %t.o -o %t.exe 2>&1 | FileCheck %s
-// CHECK:      warning: {{.*}}.o:(.nonalloc1+0x2): has non-ABS relocation R_X86_64_PC32 against symbol '_start'
-// CHECK-NEXT: warning: {{.*}}.o:(.nonalloc1+0x7): has non-ABS relocation R_X86_64_PC32 against symbol '_start'
+// CHECK:      warning: {{.*}}.o:(.nonalloc1+0x1): has non-ABS relocation R_X86_64_PC32 against symbol '_start'
+// CHECK-NEXT: warning: {{.*}}.o:(.nonalloc1+0x6): has non-ABS relocation R_X86_64_PC32 against symbol '_start'
 
 // RUN: llvm-objdump -D --no-show-raw-insn %t.exe | FileCheck --check-prefix=DISASM %s
 // DISASM:      Disassembly of section .nonalloc:
 // DISASM-EMPTY:
 // DISASM-NEXT: <.nonalloc>:
 // DISASM-NEXT:   0: nop
 // DISASM-NEXT:   1: callq 0x0
 // DISASM-NEXT:   6: callq 0x0