[PATCH] D41613: [ELF] - Do not use HeaderSize for conditions in PltSection.

[Rafael Avila de Espindola via llvm-commits]

LGTM

George Rimar via Phabricator <reviews at reviews.llvm.org> writes:

> grimar updated this revision to Diff 128300.
> grimar added a comment.
>
> - Addressed review comment (IsPlt->IsIplt).
>
>
> https://reviews.llvm.org/D41613
>
> Files:
>   ELF/SyntheticSections.cpp
>   ELF/SyntheticSections.h
>   ELF/Writer.cpp
>   test/ELF/ppc64-ifunc.s
>
> Index: test/ELF/ppc64-ifunc.s
> ===================================================================
> --- test/ELF/ppc64-ifunc.s
> +++ test/ELF/ppc64-ifunc.s
> @@ -0,0 +1,41 @@
> +# REQUIRES: ppc
> +# RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-linux %s -o %t.o
> +# RUN: llvm-mc -filetype=obj -triple=powerpc64-unknown-linux %p/Inputs/shared-ppc64.s -o %t2.o
> +# RUN: ld.lld -shared %t2.o -o %t2.so
> +# RUN: ld.lld %t.o %t2.so -o %t
> +# RUN: llvm-objdump -d %t | FileCheck %s
> +
> +# CHECK:      _start:
> +# CHECK-NEXT:  10010004: {{.*}} bl .+12
> +# CHECK-NEXT:  10010008: {{.*}} bl .+40
> +
> +# 0x10010004 + 12 = 0x10010010 (PLT entry 0)
> +# 0x10010008 + 40 = 0x10010030 (PLT entry 1)
> +
> +# CHECK:     Disassembly of section .plt:
> +# CHECK:       10010010: {{.*}} std 2, 40(1)
> +# CHECK-NEXT:  10010014: {{.*}} addis 11, 2, 4098
> +# CHECK-NEXT:  10010018: {{.*}} ld 12, -32744(11)
> +# CHECK-NEXT:  1001001c: {{.*}} ld 11, 0(12)
> +# CHECK-NEXT:  10010020: {{.*}} mtctr 11
> +# CHECK-NEXT:  10010024: {{.*}} ld 2, 8(12)
> +# CHECK-NEXT:  10010028: {{.*}} ld 11, 16(12)
> +# CHECK-NEXT:  1001002c: {{.*}} bctr
> +# CHECK-NEXT:  10010030: {{.*}} std 2, 40(1)
> +# CHECK-NEXT:  10010034: {{.*}} addis 11, 2, 4098
> +# CHECK-NEXT:  10010038: {{.*}} ld 12, -32736(11)
> +# CHECK-NEXT:  1001003c: {{.*}} ld 11, 0(12)
> +# CHECK-NEXT:  10010040: {{.*}} mtctr 11
> +# CHECK-NEXT:  10010044: {{.*}} ld 2, 8(12)
> +# CHECK-NEXT:  10010048: {{.*}} ld 11, 16(12)
> +# CHECK-NEXT:  1001004c: {{.*}} bctr
> +
> +.type ifunc STT_GNU_IFUNC
> +.globl ifunc
> +ifunc:
> + nop
> +
> +.global _start
> +_start:
> +  bl bar
> +  bl ifunc
> Index: ELF/Writer.cpp
> ===================================================================
> --- ELF/Writer.cpp
> +++ ELF/Writer.cpp
> @@ -369,9 +369,9 @@
>        false /*Sort*/);
>    Add(InX::RelaIplt);
>  
> -  InX::Plt = make<PltSection>(Target->PltHeaderSize);
> +  InX::Plt = make<PltSection>(Target->PltHeaderSize, false /*IsIplt*/);
>    Add(InX::Plt);
> -  InX::Iplt = make<PltSection>(0);
> +  InX::Iplt = make<PltSection>(0, true /*IsIplt*/);
>    Add(InX::Iplt);
>  
>    if (!Config->Relocatable) {
> Index: ELF/SyntheticSections.h
> ===================================================================
> --- ELF/SyntheticSections.h
> +++ ELF/SyntheticSections.h
> @@ -484,13 +484,13 @@
>    size_t Size = 0;
>  };
>  
> -// The PltSection is used for both the Plt and Iplt. The former always has a
> +// The PltSection is used for both the Plt and Iplt. The former usually has a
>  // header as its first entry that is used at run-time to resolve lazy binding.
>  // The latter is used for GNU Ifunc symbols, that will be subject to a
>  // Target->IRelativeRel.
>  class PltSection : public SyntheticSection {
>  public:
> -  PltSection(size_t HeaderSize);
> +  PltSection(size_t HeaderSize, bool IsIplt);
>    void writeTo(uint8_t *Buf) override;
>    size_t getSize() const override;
>    bool empty() const override { return Entries.empty(); }
> @@ -501,8 +501,10 @@
>  private:
>    unsigned getPltRelocOff() const;
>    std::vector<std::pair<const Symbol *, unsigned>> Entries;
> -  // Iplt always has HeaderSize of 0, the Plt HeaderSize is always non-zero
>    size_t HeaderSize;
> +
> +  // True if section is Iplt.
> +  bool IsIplt;
>  };
>  
>  // GdbIndexChunk is created for each .debug_info section and contains
> Index: ELF/SyntheticSections.cpp
> ===================================================================
> --- ELF/SyntheticSections.cpp
> +++ ELF/SyntheticSections.cpp
> @@ -1836,9 +1836,9 @@
>    }
>  }
>  
> -PltSection::PltSection(size_t S)
> +PltSection::PltSection(size_t S, bool IsIplt)
>      : SyntheticSection(SHF_ALLOC | SHF_EXECINSTR, SHT_PROGBITS, 16, ".plt"),
> -      HeaderSize(S) {
> +      HeaderSize(S), IsIplt(IsIplt) {
>    // The PLT needs to be writable on SPARC as the dynamic linker will
>    // modify the instructions in the PLT entries.
>    if (Config->EMachine == EM_SPARCV9)
> @@ -1867,7 +1867,7 @@
>  template <class ELFT> void PltSection::addEntry(Symbol &Sym) {
>    Sym.PltIndex = Entries.size();
>    RelocationBaseSection *PltRelocSection = InX::RelaPlt;
> -  if (HeaderSize == 0) {
> +  if (IsIplt) {
>      PltRelocSection = InX::RelaIplt;
>      Sym.IsInIplt = true;
>    }
> @@ -1894,7 +1894,7 @@
>  }
>  
>  unsigned PltSection::getPltRelocOff() const {
> -  return (HeaderSize == 0) ? InX::Plt->getSize() : 0;
> +  return IsIplt ? InX::Plt->getSize() : 0;
>  }
>  
>  // The string hash function for .gdb_index.