[PATCH] aarch64: simplify the DT_AARCH64_VARIANT_PCS handling code

[PATCH] aarch64: simplify the DT_AARCH64_VARIANT_PCS handling code

[Szabolcs Nagy]

Remove unnecessary variant_pcs field: the dynamic tag can be checked
directly.

I'll commit this tomorrow unless there are comments.
Then backport it with the variant pcs support to release branches
https://sourceware.org/ml/libc-alpha/2019-05/msg00523.html

2019-07-09  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	* sysdeps/aarch64/dl-machine.h (elf_machine_runtime_setup): Remove the
	DT_AARCH64_VARIANT_PCS check.
	(elf_machine_lazy_rel): Use l_info[DT_AARCH64 (VARIANT_PCS)].
	* sysdeps/aarch64/linkmap.h (struct link_map_machine): Remove
	variant_pcs.

Re: [PATCH] aarch64: simplify the DT_AARCH64_VARIANT_PCS handling code

[Szabolcs Nagy]

[-- Attachment #1: Type: text/plain, Size: 681 bytes --]

On 09/07/2019 13:55, Szabolcs Nagy wrote:
> Remove unnecessary variant_pcs field: the dynamic tag can be checked
> directly.
> 
> I'll commit this tomorrow unless there are comments.
> Then backport it with the variant pcs support to release branches
> https://sourceware.org/ml/libc-alpha/2019-05/msg00523.html
> 
> 2019-07-09  Szabolcs Nagy  <szabolcs.nagy@arm.com>
> 
> 	* sysdeps/aarch64/dl-machine.h (elf_machine_runtime_setup): Remove the
> 	DT_AARCH64_VARIANT_PCS check.
> 	(elf_machine_lazy_rel): Use l_info[DT_AARCH64 (VARIANT_PCS)].
> 	* sysdeps/aarch64/linkmap.h (struct link_map_machine): Remove
> 	variant_pcs.
> 

and now with the patch attached.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vpcs.diff --]
[-- Type: text/x-patch; name="vpcs.diff", Size: 1220 bytes --]

diff --git a/sysdeps/aarch64/dl-machine.h b/sysdeps/aarch64/dl-machine.h
index 4f27637b20..9b2e0ffdbf 100644
--- a/sysdeps/aarch64/dl-machine.h
+++ b/sysdeps/aarch64/dl-machine.h
@@ -105,10 +105,6 @@ elf_machine_runtime_setup (struct link_map *l, int lazy, int profile)
 	}
     }
 
-  /* Check if STO_AARCH64_VARIANT_PCS needs to be handled.  */
-  if (l->l_info[DT_AARCH64 (VARIANT_PCS)])
-    l->l_mach.variant_pcs = 1;
-
   return lazy;
 }
 
@@ -402,7 +398,7 @@ elf_machine_lazy_rel (struct link_map *map,
 	  return;
 	}
 
-      if (__glibc_unlikely (map->l_mach.variant_pcs))
+      if (__glibc_unlikely (map->l_info[DT_AARCH64 (VARIANT_PCS)] != NULL))
 	{
 	  /* Check the symbol table for variant PCS symbols.  */
 	  const Elf_Symndx symndx = ELFW (R_SYM) (reloc->r_info);
diff --git a/sysdeps/aarch64/linkmap.h b/sysdeps/aarch64/linkmap.h
index 7f801b14db..ba74fe10e1 100644
--- a/sysdeps/aarch64/linkmap.h
+++ b/sysdeps/aarch64/linkmap.h
@@ -20,5 +20,4 @@ struct link_map_machine
 {
   ElfW(Addr) plt;	  /* Address of .plt */
   void *tlsdesc_table;	  /* Address of TLS descriptor hash table.  */
-  int variant_pcs;	  /* If set, PLT calls may follow a variant PCS.  */
 };

Re: [PATCH] aarch64: simplify the DT_AARCH64_VARIANT_PCS handling code

[Szabolcs Nagy]

On 09/07/2019 13:55, Szabolcs Nagy wrote:
> Remove unnecessary variant_pcs field: the dynamic tag can be checked
> directly.
> 
> I'll commit this tomorrow unless there are comments.
> Then backport it with the variant pcs support to release branches
> https://sourceware.org/ml/libc-alpha/2019-05/msg00523.html

i was about to back port this but i wonder if
glibc has policy that a release branch cannot
change libc internal abi between dynamic linker
and lib*.so to allow reliable updates?

this patch changes the size of map->l_info array.

> 
> 2019-07-09  Szabolcs Nagy  <szabolcs.nagy@arm.com>
> 
> 	* sysdeps/aarch64/dl-machine.h (elf_machine_runtime_setup): Remove the
> 	DT_AARCH64_VARIANT_PCS check.
> 	(elf_machine_lazy_rel): Use l_info[DT_AARCH64 (VARIANT_PCS)].
> 	* sysdeps/aarch64/linkmap.h (struct link_map_machine): Remove
> 	variant_pcs.
> 

Re: [PATCH] aarch64: simplify the DT_AARCH64_VARIANT_PCS handling code

[Carlos O'Donell]

On 7/10/19 1:21 PM, Szabolcs Nagy wrote:
> On 09/07/2019 13:55, Szabolcs Nagy wrote:
>> Remove unnecessary variant_pcs field: the dynamic tag can be checked
>> directly.
>>
>> I'll commit this tomorrow unless there are comments.
>> Then backport it with the variant pcs support to release branches
>> https://sourceware.org/ml/libc-alpha/2019-05/msg00523.html
> 
> i was about to back port this but i wonder if
> glibc has policy that a release branch cannot
> change libc internal abi between dynamic linker
> and lib*.so to allow reliable updates?

Correct.

If possible it's something I would like to avoid.

I have some ideas for narrowing the window in Fedora (using a symlink
tree and flipping a prent symlink), but it's still a window.

How strong is your need to backport this?

> this patch changes the size of map->l_info array.
> 
>>
>> 2019-07-09  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>>
>> 	* sysdeps/aarch64/dl-machine.h (elf_machine_runtime_setup): Remove the
>> 	DT_AARCH64_VARIANT_PCS check.
>> 	(elf_machine_lazy_rel): Use l_info[DT_AARCH64 (VARIANT_PCS)].
>> 	* sysdeps/aarch64/linkmap.h (struct link_map_machine): Remove
>> 	variant_pcs.
>>
> 


-- 
Cheers,
Carlos.

Re: [PATCH] aarch64: simplify the DT_AARCH64_VARIANT_PCS handling code

[Florian Weimer]

* Szabolcs Nagy:

> On 09/07/2019 13:55, Szabolcs Nagy wrote:
>> Remove unnecessary variant_pcs field: the dynamic tag can be checked
>> directly.
>> 
>> I'll commit this tomorrow unless there are comments.
>> Then backport it with the variant pcs support to release branches
>> https://sourceware.org/ml/libc-alpha/2019-05/msg00523.html
>
> i was about to back port this but i wonder if
> glibc has policy that a release branch cannot
> change libc internal abi between dynamic linker
> and lib*.so to allow reliable updates?
>
> this patch changes the size of map->l_info array.

The usual issue involves changing the size of _rtld_global and
_rtld_global_ro.

I don't know to what extent libc and libdl access link maps directly,
without going through ld.so.  I don't recall seeing anything like
that, but that doesn't mean much.

Re: [PATCH] aarch64: simplify the DT_AARCH64_VARIANT_PCS handling code

[Szabolcs Nagy]

On 10/07/2019 20:46, Florian Weimer wrote:
> * Szabolcs Nagy:
> 
>> On 09/07/2019 13:55, Szabolcs Nagy wrote:
>>> Remove unnecessary variant_pcs field: the dynamic tag can be checked
>>> directly.
>>>
>>> I'll commit this tomorrow unless there are comments.
>>> Then backport it with the variant pcs support to release branches
>>> https://sourceware.org/ml/libc-alpha/2019-05/msg00523.html
>>
>> i was about to back port this but i wonder if
>> glibc has policy that a release branch cannot
>> change libc internal abi between dynamic linker
>> and lib*.so to allow reliable updates?
>>
>> this patch changes the size of map->l_info array.
> 
> The usual issue involves changing the size of _rtld_global and
> _rtld_global_ro.
> 
> I don't know to what extent libc and libdl access link maps directly,
> without going through ld.so.  I don't recall seeing anything like
> that, but that doesn't mean much.

i'll try to do some analysis, but e.g. l_tls_* is after l_info
in struct link_map and i'd expect those to be accessed by
libpthreads and ld.so as well.

Re: [PATCH] aarch64: simplify the DT_AARCH64_VARIANT_PCS handling code

[Szabolcs Nagy]

On 10/07/2019 20:29, Carlos O'Donell wrote:
> On 7/10/19 1:21 PM, Szabolcs Nagy wrote:
>> On 09/07/2019 13:55, Szabolcs Nagy wrote:
>>> Remove unnecessary variant_pcs field: the dynamic tag can be checked
>>> directly.
>>>
>>> I'll commit this tomorrow unless there are comments.
>>> Then backport it with the variant pcs support to release branches
>>> https://sourceware.org/ml/libc-alpha/2019-05/msg00523.html
>>
>> i was about to back port this but i wonder if
>> glibc has policy that a release branch cannot
>> change libc internal abi between dynamic linker
>> and lib*.so to allow reliable updates?
> 
> Correct.
> 
> If possible it's something I would like to avoid.
> 
> I have some ideas for narrowing the window in Fedora (using a symlink
> tree and flipping a prent symlink), but it's still a window.
> 
> How strong is your need to backport this?

i really want to make older glibc work with sve calls.

currently it's a hard to debug failure whenever a new
compiler+linker is used on an existing distro.

in principle there is no need to change the link_map
struct, DT_AARCH64_VARIANT_PCS is an optimization for
load time reloc processing: symbol table check can be
avoided for untagged binaries (they don't do sve calls).

so one solutions is to just do the symbol table check
unconditionally (slight load time regression with lazy
binding).

(alternatively it is also possible to store the one
bit information somewhere else in link_map without
changing its layout, but that would be a huge hack).

> 
>> this patch changes the size of map->l_info array.
>>
>>>
>>> 2019-07-09  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>>>
>>>     * sysdeps/aarch64/dl-machine.h (elf_machine_runtime_setup): Remove the
>>>     DT_AARCH64_VARIANT_PCS check.
>>>     (elf_machine_lazy_rel): Use l_info[DT_AARCH64 (VARIANT_PCS)].
>>>     * sysdeps/aarch64/linkmap.h (struct link_map_machine): Remove
>>>     variant_pcs.
>>>
>>
> 
> 

Re: [PATCH] aarch64: simplify the DT_AARCH64_VARIANT_PCS handling code

[Carlos O'Donell]

On 7/11/19 5:22 AM, Szabolcs Nagy wrote:
> On 10/07/2019 20:29, Carlos O'Donell wrote:
>> On 7/10/19 1:21 PM, Szabolcs Nagy wrote:
>>> On 09/07/2019 13:55, Szabolcs Nagy wrote:
>>>> Remove unnecessary variant_pcs field: the dynamic tag can be checked
>>>> directly.
>>>>
>>>> I'll commit this tomorrow unless there are comments.
>>>> Then backport it with the variant pcs support to release branches
>>>> https://sourceware.org/ml/libc-alpha/2019-05/msg00523.html
>>>
>>> i was about to back port this but i wonder if
>>> glibc has policy that a release branch cannot
>>> change libc internal abi between dynamic linker
>>> and lib*.so to allow reliable updates?
>>
>> Correct.
>>
>> If possible it's something I would like to avoid.
>>
>> I have some ideas for narrowing the window in Fedora (using a symlink
>> tree and flipping a prent symlink), but it's still a window.
>>
>> How strong is your need to backport this?
> 
> i really want to make older glibc work with sve calls.
> 
> currently it's a hard to debug failure whenever a new
> compiler+linker is used on an existing distro.

That sounds like a strong need.

> in principle there is no need to change the link_map
> struct, DT_AARCH64_VARIANT_PCS is an optimization for
> load time reloc processing: symbol table check can be
> avoided for untagged binaries (they don't do sve calls).
> 
> so one solutions is to just do the symbol table check
> unconditionally (slight load time regression with lazy
> binding).

That sounds like a good compromise. I would be OK with
that kind of change on a stable branch.

> (alternatively it is also possible to store the one
> bit information somewhere else in link_map without
> changing its layout, but that would be a huge hack).

Yeah, we should avoid that :-)

>>
>>> this patch changes the size of map->l_info array.
>>>
>>>>
>>>> 2019-07-09  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>>>>
>>>>      * sysdeps/aarch64/dl-machine.h (elf_machine_runtime_setup): Remove the
>>>>      DT_AARCH64_VARIANT_PCS check.
>>>>      (elf_machine_lazy_rel): Use l_info[DT_AARCH64 (VARIANT_PCS)].
>>>>      * sysdeps/aarch64/linkmap.h (struct link_map_machine): Remove
>>>>      variant_pcs.
>>>>
>>>
>>
>>
> 


-- 
Cheers,
Carlos.

Re: [PATCH] aarch64: simplify the DT_AARCH64_VARIANT_PCS handling code

[Szabolcs Nagy]

On 11/07/2019 10:07, Szabolcs Nagy wrote:
> On 10/07/2019 20:46, Florian Weimer wrote:
>> * Szabolcs Nagy:
>>
>>> On 09/07/2019 13:55, Szabolcs Nagy wrote:
>>>> Remove unnecessary variant_pcs field: the dynamic tag can be checked
>>>> directly.
>>>>
>>>> I'll commit this tomorrow unless there are comments.
>>>> Then backport it with the variant pcs support to release branches
>>>> https://sourceware.org/ml/libc-alpha/2019-05/msg00523.html
>>>
>>> i was about to back port this but i wonder if
>>> glibc has policy that a release branch cannot
>>> change libc internal abi between dynamic linker
>>> and lib*.so to allow reliable updates?
>>>
>>> this patch changes the size of map->l_info array.
>>
>> The usual issue involves changing the size of _rtld_global and
>> _rtld_global_ro.
>>
>> I don't know to what extent libc and libdl access link maps directly,
>> without going through ld.so.  I don't recall seeing anything like
>> that, but that doesn't mean much.
> 
> i'll try to do some analysis, but e.g. l_tls_* is after l_info
> in struct link_map and i'd expect those to be accessed by
> libpthreads and ld.so as well.
> 

i checked what happens with old ld.so but new lib*.so and
libc.so can call

  EXTERN void (*_dl_rtld_lock_recursive) (void *);

member of the rtld_global struct in ld.so during a
__GI__dl_addr(ptmalloc_init,...) call, but there is a

  EXTERN struct link_map _dl_rtld_map;

member before, so inconsistent link_map size across
libc and ld.so breaks this.

i suspect rtld_global._dl_tls_* and link_map->l_tls*
access would be broken too if execution could get there.