Re: [PATCH glibc 2/2] aarch64: handle STO_AARCH64_VARIANT_PCS

[Szabolcs Nagy <Szabolcs.Nagy@arm.com>]

On 23/05/2019 13:31, Szabolcs Nagy wrote:
> On 23/05/2019 12:35, Florian Weimer wrote:
>> * Szabolcs Nagy:
>>
>>>    /* Check for unexpected PLT reloc type.  */
>>>    if (__builtin_expect (r_type == AARCH64_R(JUMP_SLOT), 1))
>>>      {
>>> -      if (__builtin_expect (map->l_mach.plt, 0) == 0)
>>> -	*reloc_addr += l_addr;
>>> -      else
>>> -	*reloc_addr = map->l_mach.plt;
>>> +      if (map->l_mach.plt == 0)
>>> +	{
>>> +	  /* Prelinking.  */
>>> +	  *reloc_addr += l_addr;
>>> +	  return;
>>> +	}
>>> +
>>> +      if (__glibc_unlikely (map->l_mach.variant_pcs))
>>> +	{
>>> +	  /* Check the symbol table for variant PCS symbols.  */
>>> +	  const Elf_Symndx symndx = ELFW (R_SYM) (reloc->r_info);
>>> +	  const ElfW (Sym) *symtab =
>>> +	    (const void *)D_PTR (map, l_info[DT_SYMTAB]);
>>> +	  const ElfW (Sym) *sym = &symtab[symndx];
>>> +	  if (__glibc_unlikely (sym->st_other & STO_AARCH64_VARIANT_PCS))
>>> +	    {
>>> +	      /* Avoid lazy resolution of variant PCS symbols.  */
>>> +	      const struct r_found_version *version = NULL;
>>> +	      if (map->l_info[VERSYMIDX (DT_VERSYM)] != NULL)
>>> +		{
>>> +		  const ElfW (Half) *vernum =
>>> +		    (const void *)D_PTR (map, l_info[VERSYMIDX (DT_VERSYM)]);
>>> +		  version = &map->l_versions[vernum[symndx] & 0x7fff];
>>> +		}
>>> +	      elf_machine_rela (map, reloc, sym, version, reloc_addr,
>>> +				skip_ifunc);
>>> +	      return;
>>> +	    }
>>> +	}
>>> +
>>> +      *reloc_addr = map->l_mach.plt;
>>
>> Looking at this, I wonder if it would be more natural to handle this as
>> a new relocation type.  This way, you will get an error from old dynamic
>> linkers.
> 
> yes, but that's a more intrusive change.

to expand a bit: i want old dynamic linkers to
continue to work if they do not use lazy binding.

currently only glibc supports lazy binding as far as
i can tell and even there you can force it to be off
(in which case a new binary with variant pcs calls works
with an old dyn linker).

unfortunately i don't see an easy way to error out in
an old dynamic linker just in case of lazy binding.

a new dyn reloc seems to be more in spirit of the elf
spec and means nicer code (the reloc type is enough
instead of using the reloc sym index to check the symtab),
but i think that is not good enough argument to break
bw compat.

i think the abi can be still changed if it is important to
error out when variant pcs calls are not supported, but
that means it's impossible to run new code on an old system
even with adequate workarounds in place (-Wl,-z,now linking
or no lazy binding support in libc).

do you see other problems with not using a separate dyn reloc?

> 
> R_*_JUMP_SLOT is handled specially at various places
> which require changes if we introduce a new dynamic
> reloc.
> 
> the symbol marking is still required: static linker
> needs to know which symbols are special (could be
> done with static relocations, but then almost all
> relocations would need a 'variant pcs' alternative
> which is a lot of changes in static linkers)
> 
> so it seemed that DT_ tag + st_other marking is
> minimally intrusive and sufficient, but i agree
> that and R_* dynamic reloc + st_other marking
> would be another feasible option (i didnt prototype
> that so i cant tell how much more work that is).
>