[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
> @@ -370,8 +370,14 @@
>  public:
>    RelocationBaseSection(StringRef Name, uint32_t Type, int32_t DynamicTag,
>                          int32_t SizeDynamicTag);
> +  // Add a dynamic relocation that will be handled by the the runtime linker
> +  // unchanged.

Not sure this comment adds much. If anything it makes it sound like the
dynamic linker will change the other relocations.

>    void addReloc(RelType DynType, InputSectionBase *IS, uint64_t OffsetInSec,
>                  Symbol *Sym);
> +  // 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 or --apply-dynamic-relocs.

I would reduce this to just:

Add a dynamic relocation that might need an addend. This takes care of
writing the addend to the output section if needed.

> 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 we are adding a relocation against the load address we need to write
> +    // the full virtual address as the addend, otherwise just the addend.

What is a "relocation against the load address"? Maybe just

If UseSymVA is true we have to include the symbol address, otherwise
just the addend.

> Index: ELF/Driver.cpp
> ===================================================================
> --- ELF/Driver.cpp
> +++ ELF/Driver.cpp
> @@ -837,6 +837,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).

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.

LGTM with the comments trimmed.

Cheers,
Rafael