Re: [RFC] Add IFUNC support for MIPS

[Faraz Shahbazker <faraz dot shahbazker at imgtec dot com>]

I fudged the text formatting on my mail client in the previous message. Hopefully, 
this version will read better:

On 08/02/2015 07:03 AM, Richard Sandiford wrote:
> I'm not sure we should consider the new tag to be unlikely.  If the
> intention is to use ifuncs in libc then it would eventually become
> pretty common.
Agreed.

>>> +		/* Symbol pre-empted, use value from GOT.
>>> +		 2nd part of condition is redundant, explicit for clarity.  */
>>> +		if (__glibc_unlikely (rmap->l_relocated)
>>> +		    && __glibc_unlikely (rmap != map))
>>> +		  {
>>> +		    const ElfW(Addr) *got
>>> +		      = (const ElfW(Addr) *) D_PTR (rmap, l_info[DT_PLTGOT]);
>>> +		    const ElfW(Word) local_gotno
>>> +		      = (const ElfW(Word))
>>> +		      rmap->l_info[DT_MIPS (LOCAL_GOTNO)]->d_un.d_val;
>>> +		    symidx = (sym
>>> +			      - (ElfW(Sym) *) D_PTR(rmap, l_info[DT_SYMTAB]))
>>> +		      / sizeof (sym);
>>> +		    reloc_value = got[symidx + local_gotno - gotsym];
>>> +		  }
>>> +		/* Symbol pre-empted by not yet relocated, use best guess.  */
>>> +		else if (__glibc_unlikely (rmap != map))
>>> +		  reloc_value = sym->st_value + rmap->l_addr;
>>> +		  /* Resolve IFUNC in this link unit.  */
>>> +		else if (__glibc_likely (ELFW(ST_TYPE) (sym->st_info)
>>> +					 == STT_GNU_IFUNC))
>>> +		  reloc_value = elf_ifunc_invoke (sym->st_value + map->l_addr);
>>> +		else
>>> +		  reloc_value += sym->st_value + map->l_addr;
>>> +	      }
> 
> Does this mean that an STT_GNU_IFUNC can have two GOT entries,
> one in the new explicitly-relocated region and one in the normal
> global GOT?  

No. My intention is to keep global IFUNCs in normal global GOT only, so that
the ABI method of mapping symbol index to GOT entry always works. The obvious
anomaly is that the ABI global GOT now contains entries that are explicitly
relocated. OTOH, by virtue of information present in the dynamic symbol table,
these GOT entries are in no danger of being relocated twice.

> The way I'd imagined it working is that STT_GNU_IFUNC
> would be an exception to the usual ABI rule that every symbol involved
> in a relocation needs a GOT entry.  Instead STT_GNU_IFUNCs would be
> kept below the DT_MIPS_GOTSYM threshold and the:
> 
> 	    if ((ElfW(Word))symidx < gotsym)
> 	      {
> 	      }
> 
> block would then handle global symbols too.  The IFUNC handling would be
> very similar to other targets.

We would still need to be able to map symbol index to GOT entry for global
symbols, right? So if we have a set of global GOT entries within the new
explicit GOT region for all global preemptible IFUNC symbols, we would need
them sorted to be contiguous in the dynamic symbol table and add a new dynamic
tag(DT_MIPS_GENERAL_GOTSYM) to mark the first index in this range. All
instances of global GOT lookup would then be modified to check whether the
symbol is IFUNC or normal. Is that what you are nudging me towards?

>>> @@ -795,8 +844,18 @@ elf_machine_got_rel (struct link_map *map, int lazy)
>>>        const struct r_found_version *version __attribute__ ((unused)) \
>>>  	= vernum ? &map->l_versions[vernum[sym_index] & 0x7fff] : NULL;	  \
>>>        struct link_map *sym_map;					  \
>>> +      ElfW(Addr) value = 0;					  \
>>>        sym_map = RESOLVE_MAP (&ref, version, reloc);		  \
>>> -      ref ? sym_map->l_addr + ref->st_value : 0;			  \
>>> +      if (__glibc_likely(ref != NULL))				  \
>>> +	{								  \
>>> +	  value = sym_map->l_addr + ref->st_value;			  \
>>> +	  if (__glibc_unlikely (ELFW(ST_TYPE) (ref->st_info)		  \
>>> +				== STT_GNU_IFUNC			  \
>>> +				&& sym_map != map			  \
>>> +				&& sym_map->l_relocated))		  \
>>> +	      value = elf_ifunc_invoke (value);				  \
>>> +	}								  \
>>> +      value;							  \
>>>      })
> 
> With that change, I think we should either:
> (b) call the resolver unconditionally, without the sym_map-based checks.
>     We shouldn't resolve an IFUNC without calling the resolver.
> 
> It's a question of whether it's more defensive to support IFUNCs in the
> ABI global GOT in the obvious way (b), in case we find a use for it later,
> or whether it's more defensive to rule it out until we know it has
> a use case (a).
> 
> Anything that did want to put IFUNCs in the ABI global GOT would need
> to be sure that the resolver doesn't rely on unrelocated data.  But in
> cases where that holds, traditional global entries might be (slightly)
> more efficient.

My understanding is: anything that wants to _invoke_ an IFUNC resolver needs
to be sure that the resolver doesn't rely on unrelocated data. I'd rather have
those sym_map checks in place and allow the program to at least load, with the
unresolved IFUNC, without causing a crash. There is the possibility that an
IFUNC which was not resolved at load time, does not get invoked at all during
execution. Allowing a crash in ld.so due to the way the application is written
makes me uncomfortable. I know the sym_map-check won't handle the most general
case(mutual dependency, etc). If you deem it unnecessary, I will gladly defer
to your judgement on this!

I did earlier consider replacing the elf_ifunc_invoke() in this case with a
GOT lookup:

+      if (__glibc_likely(ref != NULL))					       \
+	{								       \
+	  value = sym_map->l_addr + ref->st_value;			       \
+	  if (__glibc_unlikely (ELFW(ST_TYPE) (ref->st_info)		       \
+				== STT_GNU_IFUNC			       \
+				&& sym_map != map			       \
+				&& sym_map->l_relocated))		       \
+	  {								       \
+	    const ElfW(Addr) *got					       \
+		= (const ElfW(Addr) *) D_PTR (sym_map, l_info[DT_PLTGOT]);     \
+	    const ElfW(Word) local_gotno				       \
+	        = (const ElfW(Word))					       \
+                  sym_map->l_info[DT_MIPS (LOCAL_GOTNO)]->d_un.d_val;	       \
+	    symidx = (ref - (ElfW(Sym) *) D_PTR(sym_map, l_info[DT_SYMTAB]))   \
+		  / sizeof (ref);					       \
+	    reloc_value = got[symidx + local_gotno - gotsym];		       \
+	  }								       \
+	}								       \
+      value;						                       \

But couldn't make up my mind on which code would be more efficient.

To be clear, ^that^ particular bit of code is only relevant for an undefined
global symbol that resolves to an IFUNC in another ELF. IFUNCs in ABI global
GOT are already excluded/deferred by the following hunk, as they are each
assured to have an explicit R_MIPS_REL32 relocations.

@@ -866,5 +928,5 @@ elf_machine_got_rel (struct link_map *map, int lazy)
 	  if (sym->st_other == 0)
 	    *got += map->l_addr;
 	}
-      else
+      else if (ELFW(ST_TYPE) (sym->st_info) != STT_GNU_IFUNC)
 	*got = RESOLVE_GOTSYM (sym, vernum, symidx, R_MIPS_32);

Regards,
Faraz Shahbazker