Re: [RFC] Add IFUNC support for MIPS

[Richard Sandiford <rdsandiford at googlemail dot com>]

[Sorry for mistyping your name in the previous message :-(]

Faraz Shahbazker <faraz.shahbazker@imgtec.com> writes:
>>>> +		/* 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.

I don't really see the benefit of that though.  It's possible for this
particular case because the specialness of the GOT entry is encoded in
the symbol type.  But the idea of the new GOT region was that we could
put any new GOT entry types there, even if the relocation is against a
normal function or data symbol (local or global).  We wouldn't have STT
to guide us then.

Even though it's possible to avoid putting global symbols in the new region
here, you seem to be jumping through some hoops to make it work.  And the
"best guess" worries me :-)  If we just treat the new GOT region as an
ordinary relocated region and allow it to refer to global symbols, we never
have to guess.  It's just a case of applying the relocation in the same way
as for any other part of the image.

There's already precendent for allowing global symbols not to be in the
directly-mapped GOT (for TLS), so it doesn't seem like that big a leap.

>> 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?

No, I don't think so.  We just handle relocations against unmapped symbols
in the same way as other targets do.  I.e. we use RESOLVE_MAP rather than
the directly-mapped GOT cache.

> 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?

I'd like to avoid another directly-mapped region at all costs :-)

The ABI GOT scheme, including direct mapping, works very well for the
situation it was designed for.  But I don't think it extends naturally
to ifuncs (or to TLS).  Normal relocations seem like a better fit.

>>>> @@ -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!

Heh.  I have no authority here. :-)  But IMO this is no different from DSOs
having to be careful about DT_INIT order.  Yes, it's bad if they get it wrong,
but we shouldn't second-guess the user.

At least a segfault at start-up is easier to debug than a symbol having
the wrong value.

That was the point of the example I sent on the binutils list.  The problem
isn't specific to MIPS and other targets call the resolver regardless.
I think we should be consistent here.

Thanks,
Richard