Re: [PATCH v2 1/4] misc: Optimize internal usage of __libc_single_threaded

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

> On 16 Jun 2022, at 00:15, Fangrui Song <maskray@google.com> wrote:
> 
> On 2022-06-10, Adhemerval Zanella via Libc-alpha wrote:
>> By adding an internal hidden_def alias to avoid the GOT indirection.
>> On some architecture, __libc_single_thread may be accessed through
>> copy relocations and thus it requires to update both copies.
>> 
>> To obtain the correct address of the __libc_single_thread,
>> __libc_dlsym is extended to support RTLD_DEFAULT. It searches
>> through all scope instead of default local ones.
>> 
>> Checked on x86_64-linux-gnu and i686-linux-gnu.
>> ---
>> elf/dl-libc.c | 20 ++++++++++++++++++--
>> elf/libc_early_init.c | 9 +++++++++
>> include/sys/single_threaded.h | 11 +++++++++++
>> misc/single_threaded.c | 2 ++
>> nptl/pthread_create.c | 6 +++++-
>> 5 files changed, 45 insertions(+), 3 deletions(-)
>> 
>> diff --git a/elf/dl-libc.c b/elf/dl-libc.c
>> index 266e068da6..e64f4b9910 100644
>> --- a/elf/dl-libc.c
>> +++ b/elf/dl-libc.c
>> @@ -16,6 +16,7 @@
>> License along with the GNU C Library; if not, see
>> <https://www.gnu.org/licenses/>. */
>> 
>> +#include <assert.h>
>> #include <dlfcn.h>
>> #include <stdlib.h>
>> #include <ldsodefs.h>
>> @@ -72,6 +73,7 @@ struct do_dlsym_args
>> /* Arguments to do_dlsym. */
>> struct link_map *map;
>> const char *name;
>> + const void *caller_dlsym;
>> 
>> /* Return values of do_dlsym. */
>> lookup_t loadbase;
>> @@ -102,8 +104,21 @@ do_dlsym (void *ptr)
>> {
>> struct do_dlsym_args *args = (struct do_dlsym_args *) ptr;
>> args->ref = NULL;
>> - args->loadbase = GLRO(dl_lookup_symbol_x) (args->name, args->map, &args->ref,
>> -					 args->map->l_local_scope, NULL, 0,
>> + struct link_map *match = args->map;
>> + struct r_scope_elem **scope;
>> + if (args->map == RTLD_DEFAULT)
>> + {
>> + ElfW(Addr) caller = (ElfW(Addr)) args->caller_dlsym;
>> + match = _dl_find_dso_for_object (caller);
>> + /* It is only used internally, so caller should be always recognized. */
>> + assert (match != NULL);
>> + scope = match->l_scope;
>> + }
>> + else
>> + scope = args->map->l_local_scope;
>> +
>> + args->loadbase = GLRO(dl_lookup_symbol_x) (args->name, match, &args->ref,
>> +					 scope, NULL, 0,
>> 					 DL_LOOKUP_RETURN_NEWEST, NULL);
>> }
>> 
>> @@ -182,6 +197,7 @@ __libc_dlsym (void *map, const char *name)
>> struct do_dlsym_args args;
>> args.map = map;
>> args.name = name;
>> + args.caller_dlsym = RETURN_ADDRESS (0);
>> 
>> #ifdef SHARED
>> if (GLRO (dl_dlfcn_hook) != NULL)
>> diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c
>> index 3c4a19cf6b..7cc2997122 100644
>> --- a/elf/libc_early_init.c
>> +++ b/elf/libc_early_init.c
>> @@ -16,7 +16,9 @@
>> License along with the GNU C Library; if not, see
>> <https://www.gnu.org/licenses/>. */
>> 
>> +#include <assert.h>
>> #include <ctype.h>
>> +#include <dlfcn.h>
>> #include <elision-conf.h>
>> #include <libc-early-init.h>
>> #include <libc-internal.h>
>> @@ -38,6 +40,13 @@ __libc_early_init (_Bool initial)
>> __libc_single_threaded = initial;
>> 
>> #ifdef SHARED
>> + /* __libc_single_threaded can be accessed through copy relocations, so it
>> + requires to update the external copy. */
>> + __libc_external_single_threaded = __libc_dlsym (RTLD_DEFAULT,
>> +						 "__libc_single_threaded");
>> + assert (__libc_external_single_threaded != NULL);
>> + *__libc_external_single_threaded = initial;
>> +
>> __libc_initial = initial;
>> #endif
> 
> I think this whole scheme can be greatly simplified.
> 
> Under the hood,
> 
> extern __typeof (__libc_single_threaded) __EI___libc_single_threaded __asm__("" "__libc_single_threaded"); extern __typeof (__libc_single_threaded) __EI___libc_single_threaded __attribute__((alias ("" "__GI___libc_single_threaded"))) __attribute__ ((__copy__ (__libc_single_threaded))); 
> * __libc_single_threaded is the STV_HIDDEN symbol with the asm name "__GI___libc_single_threaded"
> * __EI___libc_single_threaded is the STV_DEFAULT symbol with the asm name "__libc_single_threaded". It aliases __libc_single_threaded.

In fact libc_hidden_proto for SHARED will result in:

extern __typeof (__libc_single_threaded) __libc_single_threaded __asm__ ("" "__GI___libc_single_threaded") __attribute__ ((visibility ("hidden")));;

So using __libc_single_threaded will be the most simple way.

> 
> We can just access __EI___libc_single_threaded which will lead to a GOT
> indirection (R_*_GLOB_DAT). This can avoid a __libc_dlsym call.

The idea is exactly to avoid the GOT indirection.

> 
> I can see that accessing the __EI declaration is currently inconvenient
> because include/libc-symbols.h does not seem to provide a convenient
> macro, but that be added.
> 
>> diff --git a/include/sys/single_threaded.h b/include/sys/single_threaded.h
>> index 18f6972482..258b01e0b2 100644
>> --- a/include/sys/single_threaded.h
>> +++ b/include/sys/single_threaded.h
>> @@ -1 +1,12 @@
>> #include <misc/sys/single_threaded.h>
>> +
>> +#ifndef _ISOMAC
>> +
>> +libc_hidden_proto (__libc_single_threaded);
>> +
>> +# ifdef SHARED
>> +extern __typeof (__libc_single_threaded) *__libc_external_single_threaded
>> + attribute_hidden;
>> +# endif
>> +
>> +#endif
>> diff --git a/misc/single_threaded.c b/misc/single_threaded.c
>> index 96ada9137b..201d86a273 100644
>> --- a/misc/single_threaded.c
>> +++ b/misc/single_threaded.c
>> @@ -22,6 +22,8 @@
>> __libc_early_init (as false for inner libcs). */
>> #ifdef SHARED
>> char __libc_single_threaded;
>> +__typeof (__libc_single_threaded) *__libc_external_single_threaded;
>> #else
>> char __libc_single_threaded = 1;
>> #endif
>> +libc_hidden_data_def (__libc_single_threaded)
>> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
>> index e7a099acb7..5633d01c62 100644
>> --- a/nptl/pthread_create.c
>> +++ b/nptl/pthread_create.c
>> @@ -627,7 +627,11 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>> if (__libc_single_threaded)
>> {
>> late_init ();
>> - __libc_single_threaded = 0;
>> + __libc_single_threaded =
>> +#ifdef SHARED
>> + *__libc_external_single_threaded =
>> +#endif
>> +	0;
>> }
>> 
>> const struct pthread_attr *iattr = (struct pthread_attr *) attr;
>> -- 
>> 2.34.1