Re: [PATCH] Fix dladdr on hppa

["Carlos O'Donell" <carlos at redhat dot com>]

On 01/02/2016 09:48 AM, John David Anglin wrote:
> 2016-01-02  John David Anglin  <danglin@gcc.gnu.org>
> 
> 	[BZ #19415]
> 	* sysdeps/hppa/dl-fptr.c (_dl_fixup): Declare.
> 	(elf_machine_resolve): New.  Return address of _dl_runtime_resolve.
> 	(_dl_lookup_address): Rewrite using function resolver trampoline.
> 	* sysdeps/hppa/dl-lookupcfg.h (DL_LOOKUP_ADDRESS): Don't clear bottom
> 	two bits in address.

Looks good to me.

The existing code was not ia64 specific, but it did assume that all of
the function descriptors were resolved before being compared, and that's
not a very good assumption. There were similar problems on i386 and
x86_64 until the compiler started loading the final symbol address from
the GOT and passing that to dladdr, otherwise it passed the PLT entry
address and that obviously causes the same problems hppa had.

That's the reason there is probably no test case for this because I bet
it also doesn't work reliable well on all the targets we support. For
example dlfcn/tst-dladdr.c just makes sure the various entries are
non-NULL in the returned struct.

Mike could you hlep check this in?
 
> diff --git a/sysdeps/hppa/dl-fptr.c b/sysdeps/hppa/dl-fptr.c
> index bb12ab2..ddb9db7 100644
> --- a/sysdeps/hppa/dl-fptr.c
> +++ b/sysdeps/hppa/dl-fptr.c
> @@ -315,23 +315,54 @@ _dl_unmap (struct link_map *map)
>    map->l_mach.fptr_table = NULL;
>  }
>  
> +extern ElfW(Addr) _dl_fixup (struct link_map *, ElfW(Word)) attribute_hidden;
>  
> -ElfW(Addr)
> -_dl_lookup_address (const void *address)
> +static inline Elf32_Addr
> +elf_machine_resolve (void)
>  {
> -  ElfW(Addr) addr = (ElfW(Addr)) address;
> -  struct fdesc_table *t;
> -  unsigned long int i;
> +  Elf32_Addr addr;
>  
> -  for (t = local.root; t != NULL; t = t->next)
> -    {
> -      i = (struct fdesc *) addr - &t->fdesc[0];
> -      if (i < t->first_unused && addr == (ElfW(Addr)) &t->fdesc[i])
> -	{
> -	  addr = t->fdesc[i].ip;
> -	  break;
> -	}
> -    }
> +  asm ("b,l     1f,%0\n"
> +"	depi	0,31,2,%0\n"
> +"1:	addil	L'_dl_runtime_resolve - ($PIC_pcrel$0 - 8),%0\n"
> +"	ldo	R'_dl_runtime_resolve - ($PIC_pcrel$0 - 12)(%%r1),%0\n"
> +       : "=r" (addr) : : "r1");
>  
>    return addr;
>  }
> +
> +ElfW(Addr)
> +_dl_lookup_address (const void *address)
> +{
> +  ElfW(Addr) addr = (ElfW(Addr)) address;
> +  unsigned int *desc, *gptr;
> +
> +  /* Check for special cases.  */

When might we have -1 and 0-4096?

They are obviously not real fdescs, but when are they used?

> +  if ((int) addr == -1
> +      || (unsigned int) addr < 4096
> +      || !((unsigned int) addr & 2))
> +    return addr;

OK.

> +
> +  /* Clear least-significant two bits from descriptor address.  */
> +  desc = (unsigned int *) ((unsigned int) addr & ~3);

OK.

> +
> +  /* Check if descriptor requires resolution.  The following trampoline is
> +     used in each global offset table for function resolution:
> +
> +		ldw 0(r20),r22
> +		bv r0(r22)
> +		ldw 4(r20),r21
> +     tramp:	b,l .-12,r20
> +		depwi 0,31,2,r20
> +		.word _dl_runtime_resolve
> +		.word "_dl_runtime_resolve ltp"
> +     got:	.word _DYNAMIC
> +		.word "struct link map address" */
> +  gptr = (unsigned int *) desc[0];
> +  if (gptr[0] == 0xea9f1fdd			/* b,l .-12,r20     */
> +      && gptr[1] == 0xd6801c1e			/* depwi 0,31,2,r20 */
> +      && (ElfW(Addr)) gptr[2] == elf_machine_resolve ())

OK.

> +    _dl_fixup ((struct link_map *) gptr[5], (ElfW(Word)) desc[1]);

OK.

> +
> +  return (ElfW(Addr)) desc[0];
> +}
> diff --git a/sysdeps/hppa/dl-lookupcfg.h b/sysdeps/hppa/dl-lookupcfg.h
> index c36928c..0b4dc45 100644
> --- a/sysdeps/hppa/dl-lookupcfg.h
> +++ b/sysdeps/hppa/dl-lookupcfg.h
> @@ -31,9 +31,7 @@ rtld_hidden_proto (_dl_symbol_address)
>  
>  Elf32_Addr _dl_lookup_address (const void *address);
>  
> -/* Clear the bottom two bits so generic code can find the fdesc entry */
> -#define DL_LOOKUP_ADDRESS(addr) \
> -  (_dl_lookup_address ((void *)((unsigned long)addr & ~3)))
> +#define DL_LOOKUP_ADDRESS(addr) _dl_lookup_address ((const void *) addr)

OK.

>  
>  void attribute_hidden _dl_unmap (struct link_map *map);
>  

Cheers,
Carlos.