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

Alexander Richardson via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 14 03:38:58 PST 2018


The attached patch does fix the issue in my test case but I believe
that it doesn't handle GOT/PLT relocations since those also sometimes
have UseSymVA = true.
I'll try to come up with a test case and update the patch.

On 13 February 2018 at 16:31, Rafael Avila de Espindola
<rafael.espindola at gmail.com> wrote:
> This patch is still a lot bigger than it needs to be.
>
> I took the liberty of committing refactoring bits in r325016 and r325017
> and I was able to reduce this patch to what is attached.
>
> Are you OK with the attached patch?
>
>
> diff --git a/ELF/InputSection.cpp b/ELF/InputSection.cpp
> index 6766c0091..c58cd20cf 100644
> --- a/ELF/InputSection.cpp
> +++ b/ELF/InputSection.cpp
> @@ -474,6 +474,8 @@ static uint64_t getRelocTargetVA(RelType Type, int64_t A, uint64_t P,
>    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:
> diff --git a/ELF/Relocations.cpp b/ELF/Relocations.cpp
> index e003b41df..45181796a 100644
> --- a/ELF/Relocations.cpp
> +++ b/ELF/Relocations.cpp
> @@ -780,8 +780,8 @@ static RelExpr processRelocAux(InputSectionBase &Sec, RelExpr Expr,
>                               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, R_ADDEND, Type);
>
>        // MIPS ABI turns using of GOT and dynamic relocations inside out.
>        // While regular ABI uses dynamic relocations to fill up GOT entries
> diff --git a/ELF/Relocations.h b/ELF/Relocations.h
> index 2cc8adfa5..7b142e15b 100644
> --- a/ELF/Relocations.h
> +++ b/ELF/Relocations.h
> @@ -32,6 +32,7 @@ typedef uint32_t RelType;
>  enum RelExpr {
>    R_INVALID,
>    R_ABS,
> +  R_ADDEND,
>    R_ARM_SBREL,
>    R_GOT,
>    R_GOTONLY_PC,
> diff --git a/ELF/SyntheticSections.cpp b/ELF/SyntheticSections.cpp
> index ae4fd7fed..9bfb9b57d 100644
> --- a/ELF/SyntheticSections.cpp
> +++ b/ELF/SyntheticSections.cpp
> @@ -1212,11 +1212,9 @@ void RelocationBaseSection::addReloc(RelType DynType,
>                                       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))
>      InputSec->Relocations.push_back({Expr, Type, OffsetInSec, Addend, Sym});
>    addReloc({DynType, InputSec, OffsetInSec, UseSymVA, Sym, Addend});
>  }
> diff --git a/test/ELF/rel-addend-with-rela-input.s b/test/ELF/rel-addend-with-rela-input.s
> new file mode 100644
> index 000000000..11eac97ed
> --- /dev/null
> +++ b/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
>
>
> Cheers,
> Rafael
>
> Alexander Richardson via Phabricator <reviews at reviews.llvm.org> writes:
>
>> arichardson updated this revision to Diff 133758.
>> arichardson marked 3 inline comments as done.
>> arichardson added a comment.
>>
>> - Split out an unrelated change to https://reviews.llvm.org/D43161
>> - made the diff smaller
>>
>>
>> Repository:
>>   rLLD LLVM Linker
>>
>> https://reviews.llvm.org/D42843
>>
>> Files:
>>   ELF/Arch/X86_64.cpp
>>   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
>> @@ -328,8 +328,6 @@
>>    int64_t getRelaAddend() const;
>>
>>    RelType Type;
>> -
>> -private:
>>    Symbol *Sym;
>>    const InputSectionBase *InputSec = nullptr;
>>    uint64_t OffsetInSec;
>> @@ -370,17 +368,32 @@
>>  public:
>>    RelocationBaseSection(StringRef Name, uint32_t Type, int32_t DynamicTag,
>>                          int32_t SizeDynamicTag);
>> -  void addReloc(uint32_t DynType, InputSectionBase *InputSec,
>> -                uint64_t OffsetInSec, bool UseSymVA, Symbol *Sym,
>> -                int64_t Addend, RelExpr Expr, RelType Type);
>> +  // TODO: We could merge these two functions if we can assume that the
>> +  // dynamic relocation and the static one touch the same bits. This does not
>> +  // always seem to be the case as e.g. on MIPS32 a R_MIPS_64 will be converted
>> +  // to a R_MIPS_REL32, but this may just be a bug in the MIPS code.
>> +
>> +  // Add a dynamic relocation that will be processed by the the runtime linker.
>> +  // If the dynamic relocation needs an addend or it is against the load address
>> +  // and not a symbol use addComplexReloc() instead since it can handle those
>> +  // cases.
>>    void addReloc(const DynamicReloc &Reloc);
>> +
>> +  // Add a dynamic relocation that may have been converted to a relocation
>> +  // against the load address. In that case the addend will be the virtual
>> +  // address of the symbol and must be written to the output if we are using
>> +  // REL output.
>> +  void addComplexReloc(const DynamicReloc &Reloc, RelType Type, RelExpr Expr);
>> +
>>    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:
>> +  // Add Reloc to the list of dynamic relocations and count NumRelativeRelocs
>> +  void addDynamicReloc(const DynamicReloc &Reloc);
>>    std::vector<DynamicReloc> Relocs;
>>    size_t NumRelativeRelocs = 0;
>>  };
>> Index: ELF/SyntheticSections.cpp
>> ===================================================================
>> --- ELF/SyntheticSections.cpp
>> +++ ELF/SyntheticSections.cpp
>> @@ -1202,21 +1202,33 @@
>>      : SyntheticSection(SHF_ALLOC, Type, Config->Wordsize, Name),
>>        DynamicTag(DynamicTag), SizeDynamicTag(SizeDynamicTag) {}
>>
>> -void RelocationBaseSection::addReloc(uint32_t 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)
>> -    InputSec->Relocations.push_back({Expr, Type, OffsetInSec, Addend, Sym});
>> -  addReloc({DynType, InputSec, OffsetInSec, UseSymVA, Sym, Addend});
>> +void RelocationBaseSection::addReloc(const DynamicReloc &Reloc) {
>> +  assert(!Reloc.UseSymVA &&
>> +         "For relocations against the load address use addComplexReloc()");
>> +  assert(Reloc.Addend == 0 &&
>> +         "For relocations with non-zero addends use addComplexReloc()");
>> +  addDynamicReloc(Reloc);
>>  }
>>
>> -void RelocationBaseSection::addReloc(const DynamicReloc &Reloc) {
>> +void RelocationBaseSection::addComplexReloc(const DynamicReloc &Reloc,
>> +                                            RelType Type, RelExpr Expr) {
>> +  addDynamicReloc(Reloc);
>> +
>> +  if (Config->WriteAddends) {
>> +    auto *IS = const_cast<InputSectionBase *>(Reloc.InputSec);
>> +    // If we are adding a relocation against the load address we need to write
>> +    // the full virtual address as the addend, otherwise just the addend
>> +    Expr = Reloc.UseSymVA ? Expr : R_ADDEND;
>> +    // Otherwise we only need to write the addend to the input file. To avoid
>> +    // unnecessary vector appends and calls to Target->relocateOne() we can skip
>> +    // it if the addend is zero since it will already be zero in the file.
>> +    if (Reloc.UseSymVA || Reloc.Addend != 0)
>> +      IS->Relocations.push_back(
>> +          {Expr, Type, Reloc.OffsetInSec, Reloc.Addend, Reloc.Sym});
>> +  }
>> +}
>> +
>> +void RelocationBaseSection::addDynamicReloc(const DynamicReloc &Reloc) {
>>    if (Reloc.Type == Target->RelativeRel)
>>      ++NumRelativeRelocs;
>>    Relocs.push_back(Reloc);
>> 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
>> @@ -192,8 +192,9 @@
>>        Config->Shared) {
>>      if (InX::Got->addDynTlsEntry(Sym)) {
>>        uint64_t Off = InX::Got->getGlobalDynOffset(Sym);
>> -      InX::RelaDyn->addReloc(
>> -          {Target->TlsDescRel, InX::Got, Off, !Sym.IsPreemptible, &Sym, 0});
>> +      InX::RelaDyn->addComplexReloc(
>> +          {Target->TlsDescRel, InX::Got, Off, !Sym.IsPreemptible, &Sym, 0},
>> +          Target->TlsDescRel, Expr);
>>      }
>>      if (Expr != R_TLSDESC_CALL)
>>        C.Relocations.push_back({Expr, Type, Offset, Addend, &Sym});
>> @@ -694,8 +695,9 @@
>>                          RelocationBaseSection *Rel, RelType Type, Symbol &Sym) {
>>    Plt->addEntry<ELFT>(Sym);
>>    GotPlt->addEntry(Sym);
>> -  Rel->addReloc(
>> -      {Type, GotPlt, Sym.getGotPltOffset(), !Sym.IsPreemptible, &Sym, 0});
>> +  Rel->addComplexReloc(
>> +      {Type, GotPlt, Sym.getGotPltOffset(), !Sym.IsPreemptible, &Sym, 0}, Type,
>> +      R_ABS);
>>  }
>>
>>  template <class ELFT> static void addGotEntry(Symbol &Sym) {
>> @@ -727,8 +729,11 @@
>>      Type = Target->RelativeRel;
>>    else
>>      Type = Target->GotRel;
>> -  InX::RelaDyn->addReloc(Type, InX::Got, Off, !Sym.IsPreemptible, &Sym, 0,
>> -                         R_ABS, Target->GotRel);
>> +
>> +  // If the symbol not preemptible, we can just write the virtual address to
>> +  // the GOT and add a relocation against the load address instead of using Sym
>> +  InX::RelaDyn->addComplexReloc(
>> +      {Type, InX::Got, Off, !Sym.IsPreemptible, &Sym, 0}, Type, R_ABS);
>>  }
>>
>>  // Return true if we can define a symbol in the executable that
>> @@ -778,12 +783,13 @@
>>      bool IsPreemptibleValue = Sym.IsPreemptible && Expr != R_GOT;
>>
>>      if (!IsPreemptibleValue) {
>> -      InX::RelaDyn->addReloc(Target->RelativeRel, &Sec, Offset, true, &Sym,
>> -                             Addend, Expr, Type);
>> +      InX::RelaDyn->addComplexReloc(
>> +          {Target->RelativeRel, &Sec, Offset, true, &Sym, Addend}, Type, Expr);
>>        return Expr;
>>      } else if (Target->isPicRel(Type)) {
>> -      InX::RelaDyn->addReloc(
>> -          {Target->getDynRel(Type), &Sec, Offset, false, &Sym, Addend});
>> +      InX::RelaDyn->addComplexReloc(
>> +          {Target->getDynRel(Type), &Sec, Offset, false, &Sym, Addend}, Type,
>> +          Expr);
>>
>>        // 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
>> @@ -463,6 +463,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
>> @@ -824,6 +824,13 @@
>>        (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 (for compatibility with GNU linkers).
>> +  // However, we can't do this by default since there is some system software
>> +  // such as the Bionic dynamic linker that uses the addend from the target
>> +  // location prior to dynamic relocation resolution (even when processing RELA
>> +  // relocations).
>>    Config->WriteAddends = Args.hasFlag(OPT_apply_dynamic_relocs,
>>                                        OPT_no_apply_dynamic_relocs, false) ||
>>                           !Config->IsRela;
>> Index: ELF/Arch/X86_64.cpp
>> ===================================================================
>> --- ELF/Arch/X86_64.cpp
>> +++ ELF/Arch/X86_64.cpp
>> @@ -318,6 +318,7 @@
>>    case R_X86_64_PC64:
>>    case R_X86_64_SIZE64:
>>    case R_X86_64_GOT64:
>> +  case R_X86_64_RELATIVE:
>>      write64le(Loc, Val);
>>      break;
>>    default:
>


More information about the llvm-commits mailing list