diff mbox series

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

Message ID 20220608164941.3325089-2-adhemerval.zanella@linaro.org
State Superseded
Headers
Series Simplify internal single-threaded usage |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Adhemerval Zanella June 8, 2022, 4:49 p.m. UTC 
  To avoid a GOT indirection for internal usages.  On some architecture,
__libc_single_thread can be accessed through copy relocations so it
requires to update both copies, which is done through finding the
new with dlsym.

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 dlfcn/dlsym.c                 |  1 +
 elf/libc_early_init.c         |  9 +++++++++
 include/dlfcn.h               |  4 ++++
 include/sys/single_threaded.h | 11 +++++++++++
 misc/single_threaded.c        |  2 ++
 nptl/pthread_create.c         |  6 +++++-
 6 files changed, 32 insertions(+), 1 deletion(-)

Comments

Florian Weimer June 8, 2022, 5:44 p.m. UTC | #1 
* Adhemerval Zanella via Libc-alpha:

> diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c
> index 3c4a19cf6b..18966900c4 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_thread can be accessed through copy relocations, so it
> +     requires to update the external copy.  */
> +  __libc_external_single_threaded = ___dlsym (RTLD_DEFAULT,
> +					      "__libc_single_threaded");
> +  assert (__libc_external_single_threaded != NULL);
> +  *__libc_external_single_threaded = initial;
> +
>    __libc_initial = initial;
>  #endif

Typo in the comment: [_]_libc_single_thread.

You must use __libc_dlsym, to avoid clobbering dlerror.  No need to add
___dlsym.

Is it necessary to cache the address?

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

I think you can call __libc_dlsym here.

Thanks,
Florian
Andreas Schwab June 8, 2022, 6:03 p.m. UTC | #2 
On Jun 08 2022, Florian Weimer via Libc-alpha wrote:

> * Adhemerval Zanella via Libc-alpha:
>
>> diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c
>> index 3c4a19cf6b..18966900c4 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_thread can be accessed through copy relocations, so it
>> +     requires to update the external copy.  */
>> +  __libc_external_single_threaded = ___dlsym (RTLD_DEFAULT,
>> +					      "__libc_single_threaded");
>> +  assert (__libc_external_single_threaded != NULL);
>> +  *__libc_external_single_threaded = initial;
>> +
>>    __libc_initial = initial;
>>  #endif
>
> Typo in the comment: [_]_libc_single_thread.

__libc_single_threaded
Adhemerval Zanella June 8, 2022, 6:14 p.m. UTC | #3 
On 08/06/2022 14:44, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c
>> index 3c4a19cf6b..18966900c4 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_thread can be accessed through copy relocations, so it
>> +     requires to update the external copy.  */
>> +  __libc_external_single_threaded = ___dlsym (RTLD_DEFAULT,
>> +					      "__libc_single_threaded");
>> +  assert (__libc_external_single_threaded != NULL);
>> +  *__libc_external_single_threaded = initial;
>> +
>>    __libc_initial = initial;
>>  #endif
> 
> Typo in the comment: [_]_libc_single_thread.

Ack.

> 
> You must use __libc_dlsym, to avoid clobbering dlerror.  No need to add
> ___dlsym.

In fact we do need to use ___dlsym so we can use RTLD_DEFAULT, __libc_dlsym
does not support it and I am not sure how easy it would be do to it (I am
checking, but running in some issues).

> 
> Is it necessary to cache the address?

Not really, but it a duplicated effort when pthread_create is called.

> 
>> 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;
> 
> I think you can call __libc_dlsym here.
> 
> Thanks,
> Florian
>
Adhemerval Zanella June 8, 2022, 7 p.m. UTC | #4 
On 08/06/2022 15:14, Adhemerval Zanella wrote:
> 
> 
> On 08/06/2022 14:44, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c
>>> index 3c4a19cf6b..18966900c4 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_thread can be accessed through copy relocations, so it
>>> +     requires to update the external copy.  */
>>> +  __libc_external_single_threaded = ___dlsym (RTLD_DEFAULT,
>>> +					      "__libc_single_threaded");
>>> +  assert (__libc_external_single_threaded != NULL);
>>> +  *__libc_external_single_threaded = initial;
>>> +
>>>    __libc_initial = initial;
>>>  #endif
>>
>> Typo in the comment: [_]_libc_single_thread.
> 
> Ack.
> 
>>
>> You must use __libc_dlsym, to avoid clobbering dlerror.  No need to add
>> ___dlsym.
> 
> In fact we do need to use ___dlsym so we can use RTLD_DEFAULT, __libc_dlsym
> does not support it and I am not sure how easy it would be do to it (I am
> checking, but running in some issues).
> 

Ok, I could make __libc_dlsym work with RTLD_DEFAULT, the trick is we need to
use global cope instead of local one.

>>
>> Is it necessary to cache the address?
> 
> Not really, but it a duplicated effort when pthread_create is called.
> 
>>
>>> 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;
>>
>> I think you can call __libc_dlsym here.
>>
>> Thanks,
>> Florian
>>
Florian Weimer June 8, 2022, 7:41 p.m. UTC | #5 
* Adhemerval Zanella:

> Ok, I could make __libc_dlsym work with RTLD_DEFAULT, the trick is we need to
> use global cope instead of local one.

Doesn't L(dl_ns)[LM_ID_BASE]._ns_loaded work as the handle/link map?

If it is about multiple namespaces, I think we can make pthread_create
fail with ENOSYS in secondary namespaces.  It is currently broken
anyway.

Thanks,
Florian
diff mbox series

Patch

diff --git a/dlfcn/dlsym.c b/dlfcn/dlsym.c
index 2e9ff98e79..43c7ee8c4d 100644
--- a/dlfcn/dlsym.c
+++ b/dlfcn/dlsym.c
@@ -88,3 +88,4 @@  ___dlsym (void *handle, const char *name)
 }
 weak_alias (___dlsym, dlsym)
 #endif /* !SHARED */
+libc_hidden_def (___dlsym)
diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c
index 3c4a19cf6b..18966900c4 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_thread can be accessed through copy relocations, so it
+     requires to update the external copy.  */
+  __libc_external_single_threaded = ___dlsym (RTLD_DEFAULT,
+					      "__libc_single_threaded");
+  assert (__libc_external_single_threaded != NULL);
+  *__libc_external_single_threaded = initial;
+
   __libc_initial = initial;
 #endif
 
diff --git a/include/dlfcn.h b/include/dlfcn.h
index ae25f05303..95b8756770 100644
--- a/include/dlfcn.h
+++ b/include/dlfcn.h
@@ -135,5 +135,9 @@  extern int __dladdr1 (const void *address, Dl_info *info,
 extern int __dlinfo (void *handle, int request, void *arg);
 extern char *__dlerror (void);
 
+/* Internal interfaces to avoid intra-PLT calls.  */
+extern __typeof (dlsym) ___dlsym;
+libc_hidden_proto (___dlsym);
+
 #endif
 #endif
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;