Re: [RFC 1/2] Add IFUNC support for MIPS (v5)

[Richard Sandiford <rdsandiford at googlemail dot com>]

Faraz Shahbazker <faraz.shahbazker@imgtec.com> writes:
> -		/* This wouldn't work for a symbol imported from other
> -		   libraries for which there's no GOT entry, but MIPS
> -		   requires every symbol referenced in a dynamic
> -		   relocation to have a GOT entry in the primary GOT,
> -		   so we only get here for locally-defined symbols.
> -		   For section symbols, we should *NOT* be adding
> -		   sym->st_value (per the definition of the meaning of
> -		   S in reloc expressions in the ELF64 MIPS ABI),
> -		   since it should have already been added to
> -		   reloc_value by the linker, but older versions of
> -		   GNU ld didn't add it, and newer versions don't emit
> -		   useless relocations to section symbols any more, so
> -		   it is safe to keep on adding sym->st_value, even
> -		   though it's not ABI compliant.  Some day we should
> -		   bite the bullet and stop doing this.  */
> +		if (ELFW(ST_BIND) (sym->st_info) == STB_LOCAL)
> +		  {
> +		    /* For section symbols, we should *NOT* be adding
> +		       sym->st_value (per the definition of the meaning of S
> +		       in reloc expressions in the ELF64 MIPS ABI), since it
> +		       should have already been added to reloc_value by the
> +		       linker, but older versions of GNU ld didn't add it, and
> +		       newer versions don't emit useless relocations to
> +		       section symbols any more, so it is safe to keep on
> +		       adding sym->st_value, even though it's not ABI
> +		       compliant.  */
>  #ifndef RTLD_BOOTSTRAP
> -		if (map != &GL(dl_rtld_map))
> +		    if (map != &GL(dl_rtld_map))
> +#endif
> +			reloc_value += sym->st_value + map->l_addr;
> +		  }
> +#ifndef RTLD_BOOTSTRAP
> +		/* The original MIPS ABI required every global symbol used in
> +		   a relocation to be in the global GOT.  We would then only
> +		   expect to get here for local symbols.  This restriction is
> +		   removed for objects that use DT_MIPS_GENERAL_GOTNO, since
> +		   newer relocations and symbol types do not fit easily in the
> +		   original ABI scheme.  Relocations against symbols below
> +		   DT_MIPS_GOTSYM bind in just the same way as relocations
> +		   against symbols in the global GOT; the only difference is
> +		   that we are not able to use the global GOT as a
> +		   directly-indexed lookup cache.  Symbols below
> +		   DT_MIPS_GOTSYM might be in the general GOT region or might
> +		   not have a GOT entry at all.  */
> +		else if (__glibc_unlikely (map->l_info[DT_MIPS (GENERAL_GOTNO)]
> +					   == NULL))
> +		  {
> +		    const char *strtab;
> +		    strtab = (const void *) D_PTR (map, l_info[DT_STRTAB]);
> +
> +		    _dl_error_printf ("\
> +%s: Explicitly relocated symbol `%s' requires dynamic tag MIPS_GENERAL_GOTNO\n",
> +				      RTLD_PROGNAME, strtab + sym->st_name);
> +		  }
> +		else
> +		  {
> +		    struct link_map *rmap = RESOLVE_MAP (&sym, version, r_type);
> +		    if ((ELFW(ST_TYPE) (sym->st_info) == STT_GNU_IFUNC))
> +		      reloc_value = elf_ifunc_invoke (sym->st_value
> +						      + rmap->l_addr);
> +		    else
> +		      /* The behavior for section symbols described above
> +			 is now so firmly established that it is explicitly
> +			 adopted by objects with DT_MIPS_GLOBAL_GOTNO.
> +			 We therefore don't have a special case for
> +			 section symbols.  */
> +		      reloc_value = sym->st_value + rmap->l_addr;
> +		  }
>  #endif
> -		  reloc_value += sym->st_value + map->l_addr;

Don't shoot me :-) but I meant that the structure ought to be:

  if (__glibc_unlikely (map->l_info[DT_MIPS (GENERAL_GOTNO)] == NULL))
    {
      if (ELFW(ST_BIND) (sym->st_info) == STB_LOCAL)
        {
          /* For section symbols, we should *NOT* be adding
          ...
        }
      else
        {
          const char *strtab;
          ...
        }
    }
  else
    {
      struct link_map *rmap = RESOLVE_MAP (&sym, version, r_type);
      ...
    }

I.e. in the DT_MIPS_GENERAL_GOTNO != NULL case we treat the relocation
exactly like a normal relocation on other targets.  That just feels more
future proof to me, rather than than keeping the legacy special case
even for the new objects.

This also justifies the "The behavior for section symbols described above
is now so firmly established..." comment.  With the structure in the patch
we'd never actually reach that point for section symbols so the comment
doesn't say much.  With the revised structure we'd reach that point for
section symbols but not do anything special for them.

Aside from that shuffling around, this looks great to me.

Thanks,
Richard