Re: [RFC] Add IFUNC support for MIPS (v3)

[Richard Sandiford <rdsandiford at googlemail dot com>]

Faraz Shahbazker <faraz.shahbazker@imgtec.com> writes:
> Hi Richard,
>
> I am still confused :(
> Lets back-up a bit to your earlier comment.
>
> On 08/16/2015 12:17 PM, Richard Sandiford wrote:
>> Faraz Shahbazker <faraz.shahbazker@imgtec.com> writes:
>>> @@ -579,21 +584,37 @@ elf_machine_reloc (struct link_map *map, ElfW(Addr) r_info,
>>>  
>>>  	    if ((ElfW(Word))symidx < gotsym)
>>>  	      {
>>> -		/* 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.  */
>>> +#ifndef RTLD_BOOTSTRAP
>>> +		/* Resolve IFUNC symbols with pre-emption.  */
>>> +		if (sym && __glibc_unlikely (ELFW(ST_TYPE) (sym->st_info)
>>> +					     == STT_GNU_IFUNC) && !skip_ifunc)
>> 
>> The idea of the DT_MIPS_GENERAL_GOTNO tag is to add a new region of the
>> GOT that is relocated normally.  So like I mentioned in my previous email,
>> I think we should handle all global symbols here if the tag is defined,
>> rather than restrict it to just ifuncs.
>
> We check whether the function is an IFUNC here because we are about to 
> attempting preemption for a symbol which would otherwise not be preemptible,
> as a consequence of (symidx < gotsym).
>
> If I understand correctly, we want to attempt preemption for all global
> symbols that use the explicit GOT region, not just for IFUNCs. However,
> this (symidx < gotsym) case is a pre-existing code block and I see nothing that
> precludes control from reaching here for another locally-defined global symbol
> (which should not be preempted). We have no means to distinguish the
> preemptible GENERAL_GOT symbols from the non-preemptible ones. The best we 
> can do is check if (PLTGOT < reloc_offset < PLTGOT+GENERAL_GOTNO) in order
> to decide which symbols we should attempt to preempt.
>
> The caveat is that there could be other REL32 relocations for these same
> symbols that modify a data section (eg: function pointer to an IFUNC).
> The  condition (reloc_offset < PLTGOT + GENERAL_GOTNO) does not hold for 
> such relocations, but we still need to attempt preemption. At best, 
> we would end up with code like this, where the IFUNC condition is 
> present, but not compulsory:
>
> if (symidx < gotsym)
> {
> 	const ElfW(Addr) *got = (const ElfW(Addr) *) D_PTR (map, l_info[DT_PLTGOT]);
> 	const ElfW(Word) general_gotno = 0;
>
> 	if (map->l_info[DT_MIPS (GENERAL_GOTNO)])
> 		general_gotno = const ElfW(Word)) map->l_info[DT_MIPS (GENERAL_GOTNO)]->d_un.d_val;
>
> 	if (general_gotno != 0 
> 		&& ((reloc_addr > got && reloc_addr < got + general_gotno)
> 			|| (ELFW(ST_TYPE) (sym->st_info) == STT_GNU_IFUNC)))
> 	{
> 		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
> 			reloc_value = sym->st_value + rmap->l_addr;
> 	}
> 	else if (general_gotno == 0 && ELFW(ST_TYPE) (sym->st_info) == STT_GNU_IFUNC))
> 		_dl_error_printf ("%s: Global IFUNC symbol `%s' has relocation but "
> 			"no explicitly relocated GOT entry.\n",
> 			RTLD_PROGNAME, strtab + sym->st_name);
> 	else /* pre-existing block for locally-defined global symbols */
> #ifndef RTLD_BOOTSTRAP
> 		if (map != &GL(dl_rtld_map))
> #endif
> 			reloc_value += sym->st_value + map->l_addr;
> }
>
> The only likeliness we should assign in this code is to the error condition 
> being unlikely. Only the presence of GLOBAL_GOTNO is checked and no specific value/range 
> check is imposed up on it.

This isn't necessary.  The linker shouldn't be emitting a relocation
against a symbol that under the normal gABI rules would be preemptible
if the linker wants the symbol to bind locally for that relocation.

I agree that the original psABI probably lets you get away with doing
that in some cases, but I can't really believe it was intentionally
allowed.  Certainly:

    DT_MIPS_GOTSYM

    This member holds the index of the first dynamic symbol table
    entry that corresponds to an entry in the global offset table.
    See "Global Offset Table" in this chapter.

implies that it wasn't the intention to have globally-binding dynamic
symbol table entries less than DT_MIPS_GOTSYM in the GOT, but binding
locally.  If the linker did use local GOT entries in such cases,
then why wouldn't it also emit relocations against local symbols,
such as section symbols or symbol 0?

If you're worried that (IMO broken) objects might have been trying to
get local binding for preemptible symbols, then that's the kind of thing
that the DT_MIPS_GENERAL_GOTNO check would guard against.

(AIUI this is why, when IRIX added new visibility types, they also
defined DT_MIPS_HIDDEN_GOTIDX and DT_MIPS_PROTECTED_GOTIDX.  Fortunately
that was never adopted for the GNU ABI :-)  We just use local GOT entries
and relocations against local symbols for things that should bind locally.
But the point is that even those new regions had symbols marked with the
appropriate ST_VISIBILITY.  I don't think the intention was ever to
subvert the normal gABI binding rules.)

Note that GNU ld already ensures that all "local" R_MIPS_REL32s are
against symbol 0.  AFAIK it has never intentionally emitted relocations
against global dynamic symbols < DT_MIPS_GOTSYM.

We shouldn't have a situation where the binding rules are different for
ifuncs vs. non-ifuncs, or for GOT relocations vs. non-GOT relocations.

Thanks,
Richard