[ELF] Update addends in non-allocatable sections for REL targets when creating a relocatable output.

[ikudrin]

Summary

This fixes PR37735.

As addends were not updated in that case, the debug information in the relocatable output became corrupted.

[grimar]

ELF/InputSection.cpp

751

Should instead of all other changes in this patch this line just be a

(Flags & SHF_ALLOC || Config->Relocatable) ?

With the corresponding renaming/commenting of things may be.

We have relocateAlloc/relocateNonAlloc splitted mostly for speedup of
processing debug relocations I think.

I do not feel it is very critical for -r. It seems better to reuse the existent
code than to add new.

Rui, what do you think?

851

This function is a bit overloaded with excessive variables.
It could be shorter:

void InputSectionBase::relocateNonAllocForRelocatable(uint8_t *Buf, uint8_t *BufEnd) {
  for (const Relocation &Rel : Relocations) {
    // InputSection::copyRelocations() adds only R_ABS relocations.
    assert(Rel.Expr == R_ABS);
    uint8_t *BufLoc = Rel.Offset + cast<InputSection>(this)->OutSecOff + Offset;
    uint64_t TargetVA =
        SignExtend64(Rel.Sym->getVA(Rel.Addend), Config->Wordsize * 8);
    Target->relocateOne(BufLoc, Rel.Type, TargetVA);
  }
}

And then probably inlined. (If will be decided to not to reuse the code).

[ikudrin]

ELF/InputSection.cpp

751

Yes, that's possible, and, actually, the first variant of the fix was made this way. I switched to the current version because I found it more straightforward.

With the corresponding renaming/commenting of things may be.

What renaming are you suggesting?

[grimar]

ELF/InputSection.cpp

751

Maybe something like relocateAlloc->relocateAll?

[ruiu]

ELF/InputSection.cpp

834

Can this be a non-member function? I mean if you explicitly pass cast<InputSection>(this) as an argument for this function, can't you make this a non-member, static function? Non-member functions are generally preferred over member functions if they don't depend on non-public part of a class.

835

You are not using BufEnd, so please remove.

836

I believe const unsigned Bits = Config->Is64 ? 64 : 32 would produce slightly better code because by doing that, a compiler knows Bits is either 32 or 64 and not any other value.

[ikudrin]

• Shortened relocateNonAllocForRelocatable();
• Made it static;
• Removed an unused argument;
• Changed an expression for Bits.

[ikudrin]

Ping.

[grimar]

This LGTM. The minor suggestion is below.

ELF/InputSection.cpp

746

Maybe I would make it even shorter:

const unsigned Bits = Config->Is64 ? 64 : 32;
for (const Relocation &Rel : Sec->Relocations) {
  assert(Rel.Expr == R_ABS);
  Target->relocateOne(Buf + Rel.Offset + Sec->OutSecOff, Rel.Type,
                      SignExtend64(Rel.Sym->getVA(Rel.Addend), Bits));
}

[ruiu]

LGTM

[ikudrin]

ELF/InputSection.cpp

746

Thanks for the suggestion, but I prefer to stay with the original version. I believe that the shortened variant is more complicated while giving distinct names to different things improves readability.

[MaskRay]

Note that the interaction with --compress-debug-sections={zlib,zstd} is not handled in this patch. For relocatable linking of .rel.debug_* sections, the implicit addends are not applied to the output.
I am trying to figure out a solution.

Writer<ELFT>::run
  maybeCompress
    OutputSection::writeTo
      InputSection::writeTo
        relocate  # relocations is empty
  writeSections
    OutputSection::writeTo
      InputSection::writeTo
        if SHT_REL copyRelocations
          addReloc R_ABS
        elif non-compress relocate
          relocateNonAllocForRelocatable handle R_ABS

[MaskRay]

In D48929#4647755, @MaskRay wrote:

Note that the interaction with --compress-debug-sections={zlib,zstd} is not handled in this patch. For relocatable linking of .rel.debug_* sections, the implicit addends are not applied to the output.
I am trying to figure out a solution.

Writer<ELFT>::run
  maybeCompress
    OutputSection::writeTo
      InputSection::writeTo
        relocate  # relocations is empty
  writeSections
    OutputSection::writeTo
      InputSection::writeTo
        if SHT_REL copyRelocations
          addReloc R_ABS
        elif non-compress relocate
          relocateNonAllocForRelocatable handle R_ABS

Filed https://github.com/llvm/llvm-project/issues/66738 and pushed 345f532f3fe9bd4b6d55a490683455ee542d90d9 to add a test demonstrating the issue.

ELF/InputSection.cpp

for (const RelTy &Rel : Rels) {
 
     if (Sym.isTls() && !Out::TlsPhdr)
       Target->relocateOne(BufLoc, Type, 0);
     else
       Target->relocateOne(BufLoc, Type, SignExtend64<Bits>(Sym.getVA(Addend)));
   }
 }
 
+// This is used when '-r' is given.
+// For REL targets, InputSection::copyRelocations() may store artificial
+// relocations aimed to update addends. They are handled in relocateAlloc()
+// 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;
+    uint64_t TargetVA = SignExtend64(Rel.Sym->getVA(Rel.Addend), Bits);
+    Target->relocateOne(BufLoc, Rel.Type, TargetVA);
+  }
+}
+
 template <class ELFT>
 void InputSectionBase::relocate(uint8_t *Buf, uint8_t *BufEnd) {
   if (Flags & SHF_ALLOC) {
     relocateAlloc(Buf, BufEnd);
     return;
   }
 
   auto *Sec = cast<InputSection>(this);
-  if (Sec->AreRelocsRela)
+  if (Config->Relocatable)
+    relocateNonAllocForRelocatable(Sec, Buf);
+  else if (Sec->AreRelocsRela)
     Sec->relocateNonAlloc<ELFT>(Buf, Sec->template relas<ELFT>());
   else
     Sec->relocateNonAlloc<ELFT>(Buf, Sec->template rels<ELFT>());
 }
 
 void InputSectionBase::relocateAlloc(uint8_t *Buf, uint8_t *BufEnd) {
   assert(Flags & SHF_ALLOC);
   const unsigned Bits = Config->Wordsize * 8;
void InputSectionBase::relocateAlloc(uint8_t *Buf, uint8_t *BufEnd) {
   }
 }
 
 template <class ELFT> void InputSection::writeTo(uint8_t *Buf) {
   if (Type == SHT_NOBITS)
     return;
 
   if (auto *S = dyn_cast<SyntheticSection>(this)) {
     S->writeTo(Buf + OutSecOff);
     return;
   }
 
   // If -r or --emit-relocs is given, then an InputSection
   // may be a relocation section.
   if (Type == SHT_RELA) {
     copyRelocations<ELFT>(Buf + OutSecOff, getDataAs<typename ELFT::Rela>());
     return;
   }
   if (Type == SHT_REL) {
     copyRelocations<ELFT>(Buf + OutSecOff, getDataAs<typename ELFT::Rel>());
     return;
   }
 
   // If -r is given, we may have a SHT_GROUP section.
   if (Type == SHT_GROUP) {
     copyShtGroup<ELFT>(Buf + OutSecOff);
     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);

test/ELF/pr37735.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=i386-pc-linux-gnu %s -o %t.o
+# RUN: ld.lld -r %t.o %t.o -o %t1.o
+# RUN: llvm-objdump -s -section=.bar %t1.o | FileCheck %s
+
+.section .foo
+  .byte 0
+
+# CHECK:      Contents of section .bar:
+# CHECK-NEXT:  0000 00000000 01000000
+.section .bar
+  .dc.a .foo