diff mbox series

nptl: fix __builtin_thread_pointer detection on LoongArch

Message ID 20241104023256.2983322-2-caiyinyu@loongson.cn
State New
Headers
Series nptl: fix __builtin_thread_pointer detection on LoongArch |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed

Commit Message

caiyinyu Nov. 4, 2024, 2:32 a.m. UTC 
  This is the version 2, disscussed in https://sourceware.org/pipermail/libc-alpha/2024-November/161208.html
v1: https://sourceware.org/pipermail/libc-alpha/2024-November/161185.html
---
 sysdeps/loongarch/nptl/thread_pointer.h | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

Comments

Florian Weimer Nov. 4, 2024, 6:28 a.m. UTC | #1 
> This is the version 2, disscussed in https://sourceware.org/pipermail/libc-alpha/2024-November/161208.html
> v1: https://sourceware.org/pipermail/libc-alpha/2024-November/161185.html
> ---
>  sysdeps/loongarch/nptl/thread_pointer.h | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/sysdeps/loongarch/nptl/thread_pointer.h b/sysdeps/loongarch/nptl/thread_pointer.h
> index 5dec2ef4c6..96ac47ecbf 100644
> --- a/sysdeps/loongarch/nptl/thread_pointer.h
> +++ b/sysdeps/loongarch/nptl/thread_pointer.h
> @@ -19,18 +19,12 @@
>  #ifndef _SYS_THREAD_POINTER_H
>  #define _SYS_THREAD_POINTER_H
>  
> -#include <sys/cdefs.h>
> +register void *__thread_register asm ("$tp");
>  
>  static inline void *
>  __thread_pointer (void)
>  {
> -#if __glibc_has_builtin (__builtin_thread_pointer)
> -  return __builtin_thread_pointer ();
> -#else
> -  void *__thread_register;
> -  __asm__ ("move %0, $tp" : "=r" (__thread_register));
>    return __thread_register;
> -#endif
>  }
>  
>  #endif /* _SYS_THREAD_POINTER_H */

This may result in:

warning: register of ‘tp’ used for multiple global register variables

with GCC if the application already uses a similar construct.  I don't
know if the warning is just cosmetic, or results in code generation
problems.

Thanks,
Florian
Xi Ruoyao Nov. 4, 2024, 6:45 a.m. UTC | #2 
On Mon, 2024-11-04 at 07:28 +0100, Florian Weimer wrote:
> > This is the version 2, disscussed in https://sourceware.org/pipermail/libc-alpha/2024-November/161208.html
> > v1: https://sourceware.org/pipermail/libc-alpha/2024-November/161185.html
> > ---
> >  sysdeps/loongarch/nptl/thread_pointer.h | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> > 
> > diff --git a/sysdeps/loongarch/nptl/thread_pointer.h b/sysdeps/loongarch/nptl/thread_pointer.h
> > index 5dec2ef4c6..96ac47ecbf 100644
> > --- a/sysdeps/loongarch/nptl/thread_pointer.h
> > +++ b/sysdeps/loongarch/nptl/thread_pointer.h
> > @@ -19,18 +19,12 @@
> >  #ifndef _SYS_THREAD_POINTER_H
> >  #define _SYS_THREAD_POINTER_H
> >  
> > -#include <sys/cdefs.h>
> > +register void *__thread_register asm ("$tp");
> >  
> >  static inline void *
> >  __thread_pointer (void)
> >  {
> > -#if __glibc_has_builtin (__builtin_thread_pointer)
> > -  return __builtin_thread_pointer ();
> > -#else
> > -  void *__thread_register;
> > -  __asm__ ("move %0, $tp" : "=r" (__thread_register));
> >    return __thread_register;
> > -#endif
> >  }
> >  
> >  #endif /* _SYS_THREAD_POINTER_H */
> 
> This may result in:
> 
> warning: register of ‘tp’ used for multiple global register variables
> 
> with GCC if the application already uses a similar construct.  I don't
> know if the warning is just cosmetic, or results in code generation
> problems.

This file isn't installed, so as long as the warning does not alarm when
we build Glibc it should be fine.
Michael Jeanson Nov. 4, 2024, 4:15 p.m. UTC | #3 
On 2024-11-04 01:45, Xi Ruoyao wrote:
> 
> This file isn't installed, so as long as the warning does not alarm when
> we build Glibc it should be fine.

For the record the global 'register' variable was my first approach but it
resulted in build failures on other architectures where the same register
was used in another header. There was also some talk of eventually
promoting 'thread_pointer.h' to an installed header.

That being said, any working solution is fine with me, I just want to use
__thread_pointer () in a future rseq related patch :).

Thanks,

Michael
caiyinyu Nov. 5, 2024, 2:28 a.m. UTC | #4 
在 2024/11/4 下午2:28, Florian Weimer 写道:
>> This is the version 2, disscussed in https://sourceware.org/pipermail/libc-alpha/2024-November/161208.html
>> v1: https://sourceware.org/pipermail/libc-alpha/2024-November/161185.html
>> ---
>>   sysdeps/loongarch/nptl/thread_pointer.h | 8 +-------
>>   1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/sysdeps/loongarch/nptl/thread_pointer.h b/sysdeps/loongarch/nptl/thread_pointer.h
>> index 5dec2ef4c6..96ac47ecbf 100644
>> --- a/sysdeps/loongarch/nptl/thread_pointer.h
>> +++ b/sysdeps/loongarch/nptl/thread_pointer.h
>> @@ -19,18 +19,12 @@
>>   #ifndef _SYS_THREAD_POINTER_H
>>   #define _SYS_THREAD_POINTER_H
>>   
>> -#include <sys/cdefs.h>
>> +register void *__thread_register asm ("$tp");
>>   
>>   static inline void *
>>   __thread_pointer (void)
>>   {
>> -#if __glibc_has_builtin (__builtin_thread_pointer)
>> -  return __builtin_thread_pointer ();
>> -#else
>> -  void *__thread_register;
>> -  __asm__ ("move %0, $tp" : "=r" (__thread_register));
>>     return __thread_register;
>> -#endif
>>   }
>>   
>>   #endif /* _SYS_THREAD_POINTER_H */
> This may result in:
>
> warning: register of ‘tp’ used for multiple global register variables
Indeed, a warning has been generated.
we use " register void *__thread_self asm ("$tp"); " in 
sysdeps/loongarch/nptl/tls.h like riscv or1k ...

So let's go with the following: (ps: When using the |-O2|optimization 
level during compilation, all implementations have no differences at the 
assembly level. )

diff --git a/sysdeps/loongarch/nptl/thread_pointer.h 
b/sysdeps/loongarch/nptl/thread_pointer.h index 5dec2ef4c6..3c5367af24 
100644 --- a/sysdeps/loongarch/nptl/thread_pointer.h +++ 
b/sysdeps/loongarch/nptl/thread_pointer.h @@ -19,18 +19,12 @@ #ifndef 
_SYS_THREAD_POINTER_H #define _SYS_THREAD_POINTER_H -#include 
<sys/cdefs.h> - static inline void * __thread_pointer (void) { -#if 
__glibc_has_builtin (__builtin_thread_pointer) - return 
__builtin_thread_pointer (); -#else void *__thread_register; __asm__ 
("move %0, $tp" : "=r" (__thread_register)); return __thread_register; 
-#endif } #endif /* _SYS_THREAD_POINTER_H */

>
> with GCC if the application already uses a similar construct.  I don't
> know if the warning is just cosmetic, or results in code generation
> problems.
>
> Thanks,
> Florian
caiyinyu Nov. 5, 2024, 2:43 a.m. UTC | #5 
sorry for the mess on libalpha :(   this is re-send.

see https://sourceware.org/pipermail/libc-alpha/2024-November/161237.html


在 2024/11/5 上午10:28, caiyinyu 写道:
>
>
> 在 2024/11/4 下午2:28, Florian Weimer 写道:
>>> This is the version 2, disscussed inhttps://sourceware.org/pipermail/libc-alpha/2024-November/161208.html
>>> v1:https://sourceware.org/pipermail/libc-alpha/2024-November/161185.html
>>> ---
>>>   sysdeps/loongarch/nptl/thread_pointer.h | 8 +-------
>>>   1 file changed, 1 insertion(+), 7 deletions(-)
>>>
>>> diff --git a/sysdeps/loongarch/nptl/thread_pointer.h b/sysdeps/loongarch/nptl/thread_pointer.h
>>> index 5dec2ef4c6..96ac47ecbf 100644
>>> --- a/sysdeps/loongarch/nptl/thread_pointer.h
>>> +++ b/sysdeps/loongarch/nptl/thread_pointer.h
>>> @@ -19,18 +19,12 @@
>>>   #ifndef _SYS_THREAD_POINTER_H
>>>   #define _SYS_THREAD_POINTER_H
>>>   
>>> -#include <sys/cdefs.h>
>>> +register void *__thread_register asm ("$tp");
>>>   
>>>   static inline void *
>>>   __thread_pointer (void)
>>>   {
>>> -#if __glibc_has_builtin (__builtin_thread_pointer)
>>> -  return __builtin_thread_pointer ();
>>> -#else
>>> -  void *__thread_register;
>>> -  __asm__ ("move %0, $tp" : "=r" (__thread_register));
>>>     return __thread_register;
>>> -#endif
>>>   }
>>>   
>>>   #endif /* _SYS_THREAD_POINTER_H */
>> This may result in:
>>
>> warning: register of ‘tp’ used for multiple global register variables
> Indeed, a warning has been generated.
> we use " register void *__thread_self asm ("$tp"); " in 
> sysdeps/loongarch/nptl/tls.h like riscv or1k ...
>
> So let's go with the following: (ps: When using the |-O2|optimization 
> level during compilation, all implementations have no differences at 
> the assembly level. )
>
> diff --git a/sysdeps/loongarch/nptl/thread_pointer.h 
> b/sysdeps/loongarch/nptl/thread_pointer.h index 5dec2ef4c6..3c5367af24 
> 100644 --- a/sysdeps/loongarch/nptl/thread_pointer.h +++ 
> b/sysdeps/loongarch/nptl/thread_pointer.h @@ -19,18 +19,12 @@ #ifndef 
> _SYS_THREAD_POINTER_H #define _SYS_THREAD_POINTER_H -#include 
> <sys/cdefs.h> - static inline void * __thread_pointer (void) { -#if 
> __glibc_has_builtin (__builtin_thread_pointer) - return 
> __builtin_thread_pointer (); -#else void *__thread_register; __asm__ 
> ("move %0, $tp" : "=r" (__thread_register)); return __thread_register; 
> -#endif } #endif /* _SYS_THREAD_POINTER_H */
>
>> with GCC if the application already uses a similar construct.  I don't
>> know if the warning is just cosmetic, or results in code generation
>> problems.
>>
>> Thanks,
>> Florian
diff mbox series

Patch

diff --git a/sysdeps/loongarch/nptl/thread_pointer.h b/sysdeps/loongarch/nptl/thread_pointer.h
index 5dec2ef4c6..96ac47ecbf 100644
--- a/sysdeps/loongarch/nptl/thread_pointer.h
+++ b/sysdeps/loongarch/nptl/thread_pointer.h
@@ -19,18 +19,12 @@ 
 #ifndef _SYS_THREAD_POINTER_H
 #define _SYS_THREAD_POINTER_H
 
-#include <sys/cdefs.h>
+register void *__thread_register asm ("$tp");
 
 static inline void *
 __thread_pointer (void)
 {
-#if __glibc_has_builtin (__builtin_thread_pointer)
-  return __builtin_thread_pointer ();
-#else
-  void *__thread_register;
-  __asm__ ("move %0, $tp" : "=r" (__thread_register));
   return __thread_register;
-#endif
 }
 
 #endif /* _SYS_THREAD_POINTER_H */