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

[Rafael Avila de Espindola via llvm-commits]

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

> Index: ELF/SyntheticSections.h
> ===================================================================
> --- ELF/SyntheticSections.h
> +++ ELF/SyntheticSections.h
> @@ -310,22 +310,29 @@
>  
>  class DynamicReloc {
>  public:
> -  DynamicReloc(uint32_t Type, const InputSectionBase *InputSec,
> +  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) {}

Nice cleanup, but please commit it fist since it is not related to the
rest of patch.

>    uint64_t getOffset() const;
> -  int64_t getAddend() const;
>    uint32_t getSymIndex() const;
>    const InputSectionBase *getInputSec() const { return InputSec; }
>  
> -  uint32_t Type;
> +  // Return the value that should be written to the Elf_Rela r_addend field.
> +  // This is not the same as the addend used when creating this dynamic
> +  // relocation since we may have converted a static relocation against a
> +  // non-preemptible symbol into a relocation against the load address. In that
> +  // case we write the virtual address of the symbol plus the addend into the
> +  // r_addend field.
> +  int64_t getRelaAddend() const;
>  
> -private:
> +  RelType Type;
>    Symbol *Sym;
>    const InputSectionBase *InputSec = nullptr;
>    uint64_t OffsetInSec;
> +  // If this member is true we will add the dynamic relocation against the load
> +  // address and use the symbol virtual address as the addend

Similar for the extra comments and method rename. Please commit a NFC
patch with those first.

> +void RelocationBaseSection::addComplexReloc(const DynamicReloc &Reloc,
> +                                            RelType Type, RelExpr Expr) {
> +  addDynamicReloc(Reloc);
> +
> +  // 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).

This comment seems out of place. Maybe move it to where WriteAddends is
set? It can also go in the initial cleanup patch.

I am taking a look at the rest of the patch, but feel free to commit the
NFC cleanups and rebase.

Cheers,
Rafael