[PATCH] D42843: Ensure that Elf_Rel addends are always written for dynamic relocations

[Rafael Avila de Espindola via llvm-commits]

LGTM

Thanks,
Rafael

Alexander Richardson via Phabricator <reviews at reviews.llvm.org> writes:

> arichardson updated this revision to Diff 134524.
> arichardson added a comment.
>
> - Updated comments to Rafael's suggestions
>
>
> Repository:
>   rLLD LLVM Linker
>
> https://reviews.llvm.org/D42843
>
> Files:
>   ELF/Driver.cpp
>   ELF/InputSection.cpp
>   ELF/Relocations.cpp
>   ELF/Relocations.h
>   ELF/SyntheticSections.cpp
>   ELF/SyntheticSections.h
>   test/ELF/rel-addend-with-rela-input.s
>
> Index: test/ELF/rel-addend-with-rela-input.s
> ===================================================================
> --- /dev/null
> +++ test/ELF/rel-addend-with-rela-input.s
> @@ -0,0 +1,43 @@
> +# 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
> Index: ELF/SyntheticSections.h
> ===================================================================
> --- ELF/SyntheticSections.h
> +++ ELF/SyntheticSections.h
> @@ -372,6 +372,8 @@
>                          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);
> Index: ELF/SyntheticSections.cpp
> ===================================================================
> --- ELF/SyntheticSections.cpp
> +++ ELF/SyntheticSections.cpp
> @@ -1212,12 +1212,14 @@
>                                       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});
>  }
>  
> Index: ELF/Relocations.h
> ===================================================================
> --- ELF/Relocations.h
> +++ ELF/Relocations.h
> @@ -32,6 +32,7 @@
>  enum RelExpr {
>    R_INVALID,
>    R_ABS,
> +  R_ADDEND,
>    R_ARM_SBREL,
>    R_GOT,
>    R_GOTONLY_PC,
> Index: ELF/Relocations.cpp
> ===================================================================
> --- ELF/Relocations.cpp
> +++ ELF/Relocations.cpp
> @@ -780,8 +780,8 @@
>                               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
> Index: ELF/InputSection.cpp
> ===================================================================
> --- ELF/InputSection.cpp
> +++ ELF/InputSection.cpp
> @@ -474,6 +474,8 @@
>    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:
> Index: ELF/Driver.cpp
> ===================================================================
> --- ELF/Driver.cpp
> +++ ELF/Driver.cpp
> @@ -842,6 +842,11 @@
>        (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;