Re: [PATCH] Fix lazy setting for PLTREL overlap cpus.

["Carlos O'Donell" <carlos at systemhalted dot org>]

On Wed, Apr 4, 2012 at 10:15 PM, David Miller <davem@davemloft.net> wrote:
> From: "Carlos O'Donell" <carlos@systemhalted.org>
> Date: Wed, 4 Apr 2012 00:39:13 -0400
>
>> Ah! I misread what you said. Yes, in that case, if we can *show* that
>> s390, power, and tile do the same then we can cleanup the code.
>
> Ok, here's what I came up with. ?I've tested this on all of
> x86-64, sparc32, and sparc64.
>
> 2012-04-04 ?David S. Miller ?<davem@davemloft.net>
>
> ? ? ? ?* elf/dynamic-linux.h (_ELF_DYNAMIC_DO_RELOC): Reduce down to one
> ? ? ? ?definition.
> ? ? ? ?* sysdeps/powerpc/powerpc32/dl-machine.h
> ? ? ? ?(ELF_MACHINE_PLTREL_OVERLAP): Delete.
> ? ? ? ?* sysdeps/s390/s390-32/dl-machine.h (ELF_MACHINE_PLTREL_OVERLAP):
> ? ? ? ?Likewise.
> ? ? ? ?* sysdeps/sparc/sparc32/dl-machine.h (ELF_MACHINE_PLTREL_OVERLAP):
> ? ? ? ?Likewise.
> ? ? ? ?* sysdeps/sparc/sparc64/dl-machine.h (ELF_MACHINE_PLTREL_OVERLAP):
> ? ? ? ?Likewise.

This is the right way forward, but your patch raises one more question.

> diff --git a/elf/dynamic-link.h b/elf/dynamic-link.h
> index aa71227..5f13913 100644
> --- a/elf/dynamic-link.h
> +++ b/elf/dynamic-link.h
> @@ -250,54 +250,9 @@ elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp)
> ? ?because we must be able to completely inline. ?*/
>
> ?/* On some machines, notably SPARC, DT_REL* includes DT_JMPREL in its
> - ? range. ?Note that according to the ELF spec, this is completely legal!
> - ? But conditionally define things so that on machines we know this will
> - ? not happen we do something more optimal. ?*/
> + ? range. ?Note that according to the ELF spec, this is completely legal! ?*/
>
> -# ifdef ELF_MACHINE_PLTREL_OVERLAP
> -# ?define _ELF_DYNAMIC_DO_RELOC(RELOC, reloc, map, do_lazy, skip_ifunc, test_rel) \
> - ?do { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> - ? ?struct { ElfW(Addr) start, size; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> - ? ? ? ? ? ?__typeof (((ElfW(Dyn) *) 0)->d_un.d_val) nrelative; int lazy; } ?\
> - ? ?ranges[3]; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> - ? ?int ranges_index; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> - ? ?ranges[0].lazy = ranges[2].lazy = 0; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> - ? ?ranges[1].lazy = 1; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> - ? ?ranges[0].size = ranges[1].size = ranges[2].size = 0; ? ? ? ? ? ? ? ? ? ?\
> - ? ?ranges[0].nrelative = ranges[1].nrelative = ranges[2].nrelative = 0; ? ? ?\
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> - ? ?if ((map)->l_info[DT_##RELOC]) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> - ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> - ? ? ? ranges[0].start = D_PTR ((map), l_info[DT_##RELOC]); ? ? ? ? ? ? ? ? ?\
> - ? ? ? ranges[0].size = (map)->l_info[DT_##RELOC##SZ]->d_un.d_val; ? ? ? ? ? \
> - ? ? ? if (map->l_info[VERSYMIDX (DT_##RELOC##COUNT)] != NULL) ? ? ? ? ? ? ? \
> - ? ? ? ? ranges[0].nrelative ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> - ? ? ? ? ? = MIN (map->l_info[VERSYMIDX (DT_##RELOC##COUNT)]->d_un.d_val, ? ?\
> - ? ? ? ? ? ? ? ? ?ranges[0].size / sizeof (ElfW(reloc))); ? ? ? ? ? ? ? ? ? ?\
> - ? ? ?} ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> - ? ?if ((do_lazy) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> - ? ? ? && (map)->l_info[DT_PLTREL] ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> - ? ? ? && (!test_rel || (map)->l_info[DT_PLTREL]->d_un.d_val == DT_##RELOC)) \
> - ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> - ? ? ? ranges[1].start = D_PTR ((map), l_info[DT_JMPREL]); ? ? ? ? ? ? ? ? ? \
> - ? ? ? ranges[1].size = (map)->l_info[DT_PLTRELSZ]->d_un.d_val; ? ? ? ? ? ? ?\
> - ? ? ? ranges[2].start = ranges[1].start + ranges[1].size; ? ? ? ? ? ? ? ? ? \
> - ? ? ? ranges[2].size = ranges[0].start + ranges[0].size - ranges[2].start; ?\
> - ? ? ? ranges[0].size = ranges[1].start - ranges[0].start; ? ? ? ? ? ? ? ? ? \
> - ? ? ?} ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> - ? ?for (ranges_index = 0; ranges_index < 3; ++ranges_index) ? ? ? ? ? ? ? ? \
> - ? ? ?elf_dynamic_do_##reloc ((map), ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ranges[ranges_index].start, ? ? ? ? ? ? ? ? ? ? \
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ranges[ranges_index].size, ? ? ? ? ? ? ? ? ? ? ?\
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ranges[ranges_index].nrelative, ? ? ? ? ? ? ? ? \
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ranges[ranges_index].lazy, ? ? ? ? ? ? ? ? ? ? ?\
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? skip_ifunc); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> - ?} while (0)
> -# else
> -# ?define _ELF_DYNAMIC_DO_RELOC(RELOC, reloc, map, do_lazy, skip_ifunc, test_rel) \
> +# define _ELF_DYNAMIC_DO_RELOC(RELOC, reloc, map, do_lazy, skip_ifunc, test_rel) \
> ? do { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> ? ? struct { ElfW(Addr) start, size; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> ? ? ? ? ? ? __typeof (((ElfW(Dyn) *) 0)->d_un.d_val) nrelative; int lazy; } ?\
> @@ -317,6 +272,8 @@ elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp)
> ? ? ? { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> ? ? ? ?ElfW(Addr) start = D_PTR ((map), l_info[DT_JMPREL]); ? ? ? ? ? ? ? ? ?\
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ? if (__builtin_expect (ranges[0].size, 1)) ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> + ? ? ? ? ranges[0].size = (start - ranges[0].start); ? ? ? ? ? ? ? ? ? ? ? ? \

The old code supported:

(a) |DT_REL|GAP|DT_JMPREL|

(b) |DT_JMPREL|GAP|DT_REL|

(c) |DT_JMPREL--------------------|
    |GAP1------|DT_JMPREL|GAP2----|

Where GAP, GAP1, and GAP2 can be of a size greater than or equal to zero.

After your changes we will support:

(a) |DT_REL|DT_JMPREL| (no GAP)

(c) |DT_JMPREL--------------------| (no GAP2)
    |GAP1---------------|DT_JMPREL|

The added assumption that you don't spell out is that DT_JMPREL
follows *immediately* after DT_REL.

Are we guaranteed that?

I asked Cary Coutant who is working on gold/incremental-linking about
this and he said the incremental linker always recreates the
relocation table so on an incremental link this should be true.

If we agree that (a) is true, that there is never any gap produced by
binutils, then I'm OK with this change as long as you add a comment
stating the new assumption.

Cheers,
Carlos.