Ensure that Elf_Rel addends are always written for dynamic relocations

[arichardson]

Summary

This follows up on r321889 where writing of Elf_Rel addends was partially
moved to RelocationBaseSection. This patch ensures that the addends are
always written to the output section when a input section uses RELA but the
output is REL.

[arichardson]

test/ELF/convert-rel-rela-addends.s

103 ↗

(On Diff #132582)

This ugly hack is needed because LLD will reject R_X86_64_32 relocations in shared objects but 16 and 64 bit is fine. Not sure why this is the case as the dynamic linker can just write the correct value to data section.

[grimar]

test/ELF/convert-rel-rela-addends.s

103 ↗

(On Diff #132582)

We do not allow 32 bits relocations because they might overflow in runtime.

May be I am missing something, but I do not understand why we allow 16 bit relocations in dso. I think it is a bug.
Though bfd allows them too, gold rejects them and that is what I would expect.

Usually we don't mix i386 and x64 test cases in a single test, so I would split it probably.

[arichardson]

test/ELF/convert-rel-rela-addends.s

103 ↗

(On Diff #132582)

Ah that makes sense, thanks. I can split it into two tests, one checking REL-> RELA and the other checking that RELA->REL is also correct.

[ruiu]

I don't think I fully understand the code. Can you write more comments so that first-time readers of your code can understand how your code is supposed to work?

ELF/SyntheticSections.h

331

Basically we should avoid friend whenever possible. If you need to access something that is currently private, please make it public.

372

Just like above, please avoid writing functions with default parameters (and function overloading) as well. I prefer not to hide arguments.

[arichardson]

• Removed fried and renamed a method with the same name as the member but completely different semantics
• Removed default parameters
• Added comments

[ruiu]

ELF/SyntheticSections.cpp

1215

Do I understand it correctly that, when you call the second addReloc (by the way it is better to give the two functions different name), if (!Config->IsRela) part won't be executed? If so, can this function just be

if (Reloc.Type == Target->RelativeRel)
  ++NumRelativeRelocs;
Relocs.push_back(Reloc);

?

1233–1235

I agree with you that this is a nice optimization. Please remove this TODO and just say that we skip addend 0 to reduce the number of dynamic relocations.

ELF/SyntheticSections.h

319–320

nit: add blank lines before and after this block so that it is obvious that this comment describes getRelaAddend.

331–332

This TODO should be a new code review request rather than a TODO in source code.

371

input -> output?

test/ELF/convert-rel-rela-addends.s

110 ↗

(On Diff #132734)

Please make sure the last byte of a text file is '\n'.

[arichardson]

• rebase
• address comments
• use mips for tests to avoid depending on llvm-mc changes

ELF/SyntheticSections.cpp

1215

Since the UseSymVA flag is always zero and and the addend is zero it will not be doing anything. I avoid calling this function now to skip unnecessary checks.

[arichardson]

• Split out an unrelated change to D43161
• made the diff smaller

[arichardson]

Shrink patch even more to address only the issue in the test case

[arichardson]

• Updated comments to Rafael's suggestions

ELF/Driver.cpp

static void setConfigs(opt::InputArgList &Args) {
   Config->IsLE = (Kind == ELF32LEKind || Kind == ELF64LEKind);
   Config->Endianness =
       Config->IsLE ? support::endianness::little : support::endianness::big;
   Config->IsMips64EL = (Kind == ELF64LEKind && Machine == EM_MIPS);
   Config->IsRela =
       (Config->Is64 || IsX32 || Machine == EM_PPC) && Machine != EM_MIPS;
   Config->Pic = Config->Pie || Config->Shared;
   Config->Wordsize = Config->Is64 ? 8 : 4;
+  // If the output uses REL relocations we must store the dynamic relocation
+  // addends to the output sections. We also store addends for RELA relocations
+  // if --apply-dynamic-relocs is used.
+  // We default to not writing the addends when using RELA relocations since
+  // any standard conforming tool can find it in r_addend.
   Config->WriteAddends = Args.hasFlag(OPT_apply_dynamic_relocs,
                                       OPT_no_apply_dynamic_relocs, false) ||
                          !Config->IsRela;
 }
 
 // Returns a value of "-format" option.
 static bool getBinaryOption(StringRef S) {
   if (S == "binary")

ELF/InputSection.cpp

 static uint64_t getRelocTargetVA(RelType Type, int64_t A, uint64_t P,
                                  const Symbol &Sym, RelExpr Expr) {
   switch (Expr) {
   case R_INVALID:
     return 0;
   case R_ABS:
   case R_RELAX_GOT_PC_NOPIC:
     return Sym.getVA(A);
+  case R_ADDEND:
+    return A;
   case R_ARM_SBREL:
     return Sym.getVA(A) - getARMStaticBase(Sym);
   case R_GOT:
   case R_RELAX_TLS_GD_TO_IE_ABS:
     return Sym.getGotVA() + A;
   case R_GOTONLY_PC:
     return InX::Got->getVA() + A - P;
   case R_GOTONLY_PC_FROM_END:

ELF/Relocations.h

 typedef uint32_t RelType;
 
 // List of target-independent relocation types. Relocations read
 // from files are converted to these types so that the main code
 // doesn't have to know about architecture-specific details.
 enum RelExpr {
   R_INVALID,
   R_ABS,
+  R_ADDEND,
   R_ARM_SBREL,
   R_GOT,
   R_GOTONLY_PC,
   R_GOTONLY_PC_FROM_END,
   R_GOTREL,
   R_GOTREL_FROM_END,
   R_GOT_FROM_END,
   R_GOT_OFF,

ELF/Relocations.cpp

if (CanWrite) {
     // R_GOT refers to a position in the got, even if the symbol is preemptible.
     bool IsPreemptibleValue = Sym.IsPreemptible && Expr != R_GOT;
 
     if (!IsPreemptibleValue) {
       InX::RelaDyn->addReloc(Target->RelativeRel, &Sec, Offset, true, &Sym,
                              Addend, Expr, Type);
       return Expr;
     } else if (Target->isPicRel(Type)) {
-      InX::RelaDyn->addReloc(
-          {Target->getDynRel(Type), &Sec, Offset, false, &Sym, Addend});
+      InX::RelaDyn->addReloc(Target->getDynRel(Type), &Sec, Offset, false, &Sym,
+                             Addend, Expr, Type);
 
       // MIPS ABI turns using of GOT and dynamic relocations inside out.
       // While regular ABI uses dynamic relocations to fill up GOT entries
       // MIPS ABI requires dynamic linker to fills up GOT entries using
       // specially sorted dynamic symbol table. This affects even dynamic
       // relocations against symbols which do not require GOT entries
       // creation explicitly, i.e. do not have any GOT-relocations. So if
       // a preemptible symbol has a dynamic relocation we anyway have

ELF/SyntheticSections.h

 class DynamicReloc {
 public:
   DynamicReloc(RelType Type, const InputSectionBase *InputSec,
                uint64_t OffsetInSec, bool UseSymVA, Symbol *Sym, int64_t Addend)
       : Type(Type), Sym(Sym), InputSec(InputSec), OffsetInSec(OffsetInSec),
         UseSymVA(UseSymVA), Addend(Addend) {}
 
   uint64_t getOffset() const;
   int64_t getAddend() const;
   uint32_t getSymIndex() const;
   const InputSectionBase *getInputSec() const { return InputSec; }
 
   RelType Type;
 
 private:
   Symbol *Sym;
   const InputSectionBase *InputSec = nullptr;
   uint64_t OffsetInSec;
   bool UseSymVA;
   int64_t Addend;
 };
 
 template <class ELFT> class DynamicSection final : public SyntheticSection {
   typedef typename ELFT::Dyn Elf_Dyn;
   typedef typename ELFT::Rel Elf_Rel;
   typedef typename ELFT::Rela Elf_Rela;
   typedef typename ELFT::Shdr Elf_Shdr;
   typedef typename ELFT::Sym Elf_Sym;
 
   // finalizeContents() fills this vector with the section contents.

 };
 
 class RelocationBaseSection : public SyntheticSection {
 public:
   RelocationBaseSection(StringRef Name, uint32_t Type, int32_t DynamicTag,
                         int32_t SizeDynamicTag);
   void addReloc(RelType DynType, InputSectionBase *IS, uint64_t OffsetInSec,
                 Symbol *Sym);
+  // Add a dynamic relocation that might need an addend. This takes care of
+  // writing the addend to the output section if needed.
   void addReloc(RelType DynType, InputSectionBase *InputSec,
                 uint64_t OffsetInSec, bool UseSymVA, Symbol *Sym,
                 int64_t Addend, RelExpr Expr, RelType Type);
   void addReloc(const DynamicReloc &Reloc);
   bool empty() const override { return Relocs.empty(); }
   size_t getSize() const override { return Relocs.size() * this->Entsize; }
   size_t getRelativeRelocCount() const { return NumRelativeRelocs; }
   void finalizeContents() override;
   int32_t DynamicTag, SizeDynamicTag;
 
 protected:
   std::vector<DynamicReloc> Relocs;
   size_t NumRelativeRelocs = 0;

ELF/SyntheticSections.cpp

void RelocationBaseSection::addReloc(RelType DynType, InputSectionBase *IS,
   addReloc({DynType, IS, OffsetInSec, false, Sym, 0});
 }
 
 void RelocationBaseSection::addReloc(RelType DynType,
                                      InputSectionBase *InputSec,
                                      uint64_t OffsetInSec, bool UseSymVA,
                                      Symbol *Sym, int64_t Addend, RelExpr Expr,
                                      RelType Type) {
-  // We store the addends for dynamic relocations for both REL and RELA
-  // relocations for compatibility with GNU Linkers. There is some system
-  // software such as the Bionic dynamic linker that uses the addend prior
-  // to dynamic relocation resolution.
-  if (Config->WriteAddends && UseSymVA)
+  // Write the addends to the relocated address if required. We skip
+  // it if the written value would be zero.
+  if (Config->WriteAddends && (UseSymVA || Addend != 0)) {
+    // If UseSymVA is true we have to write the symbol address, otherwise just
+    // the addend.
+    Expr = UseSymVA ? Expr : R_ADDEND;
     InputSec->Relocations.push_back({Expr, Type, OffsetInSec, Addend, Sym});
+  }
   addReloc({DynType, InputSec, OffsetInSec, UseSymVA, Sym, Addend});
 }
 
 void RelocationBaseSection::addReloc(const DynamicReloc &Reloc) {
   if (Reloc.Type == Target->RelativeRel)
     ++NumRelativeRelocs;
   Relocs.push_back(Reloc);
 }
 
 void RelocationBaseSection::finalizeContents() {
   // If all relocations are R_*_RELATIVE they don't refer to any
   // dynamic symbol and we don't need a dynamic symbol table. If that
   // is the case, just use 0 as the link.
   Link = InX::DynSymTab ? InX::DynSymTab->getParent()->SectionIndex : 0;
 
   // Set required output section properties.
   getParent()->Link = Link;
 }
 
 template <class ELFT>
 static void encodeDynamicReloc(typename ELFT::Rela *P,

test/ELF/rel-addend-with-rela-input.s

+# REQUIRES: mips
+# Check that we correctly write addends if the output use Elf_Rel but the input
+# uses Elf_Rela
+
+# RUN: llvm-mc -filetype=obj -triple=mips64-unknown-linux %s -o %t-rela.o
+# RUN: llvm-readobj -h -s -section-data -relocations %t-rela.o | FileCheck -check-prefix INPUT-RELA %s
+# INPUT-RELA:  ElfHeader {
+# INPUT-RELA:     Class: 64-bit
+# INPUT-RELA:     DataEncoding: BigEndian
+# INPUT-RELA:   Section {
+# INPUT-RELA:       Name: .data
+# INPUT-RELA:       SectionData (
+# INPUT-RELA-NEXT:    0000: 00000000 00000000 ABCDEF00 12345678 |.............4Vx|
+#                              ^--- No addend here since it uses RELA
+# INPUT-RELA:     Relocations [
+# INPUT-RELA-NEXT:  Section ({{.+}}) .rela.data {
+# INPUT-RELA-NEXT:     0x0 R_MIPS_64/R_MIPS_NONE/R_MIPS_NONE foo 0x5544
+# INPUT-RELA-NEXT:  }
+# INPUT-RELA-NEXT: ]
+
+# Previously the addend to the dynamic relocation in the .data section was not copied if
+# the input file used RELA and the output uses REL. Check that it works now:
+# RUN: ld.lld -shared -o %t.so %t-rela.o  -verbose
+# RUN: llvm-readobj -h -s -section-data -relocations %t.so | FileCheck -check-prefix RELA-TO-REL %s
+# RELA-TO-REL:  ElfHeader {
+# RELA-TO-REL:    Class: 64-bit
+# RELA-TO-REL:    DataEncoding: BigEndian
+# RELA-TO-REL:  Section {
+# RELA-TO-REL:       Name: .data
+# RELA-TO-REL:       SectionData (
+# RELA-TO-REL-NEXT:    0000: 00000000 00005544 ABCDEF00 12345678 |......UD.....4Vx|
+#                                        ^--- Addend for relocation in .rel.dyn
+# RELA-TO-REL:     Relocations [
+# RELA-TO-REL-NEXT:  Section ({{.+}}) .rel.dyn {
+# RELA-TO-REL-NEXT:     0x10000 R_MIPS_REL32/R_MIPS_64/R_MIPS_NONE foo 0x0
+# RELA-TO-REL-NEXT:  }
+# RELA-TO-REL-NEXT: ]
+
+.extern foo
+
+.data
+.quad foo + 0x5544
+.quad 0xabcdef0012345678