nptl: Add <thread_pointer.h> for LoongArch

Message ID 20241023201826.573788-1-mjeanson@efficios.com (mailing list archive)
State Committed
Commit 3d24fb25efd957f564e0cda8bb278a54db28665f
Delegated to: Arjun Shankar
Headers
Series nptl: Add <thread_pointer.h> for LoongArch |

Checks

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

Commit Message

Michael Jeanson Oct. 23, 2024, 8:18 p.m. UTC
  This will be required by the rseq extensible ABI implementation on all
Linux architectures exposing the '__rseq_size' and '__rseq_offset'
symbols to set the initial value of the 'cpu_id' field which can be used
by applications to test if rseq is available and registered. As long as
the symbols are exposed it is valid for an application to perform this
test even if rseq is not yet implemented in libc for this architecture.

Both code paths are compile tested with build-many-glibcs.py but I don't
have access to any hardware to run the tests.

Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
---
Cc: Florian Weimer <fweimer@redhat.com>
Cc: Yinyu Cai <caiyinyu@loongson.cn>
Cc: mengqinggang <mengqinggang@loongson.cn>
---
 sysdeps/loongarch/nptl/thread_pointer.h | 36 +++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 sysdeps/loongarch/nptl/thread_pointer.h
  

Comments

Arjun Shankar Oct. 30, 2024, 3:21 p.m. UTC | #1
Hi Michael,

> This will be required by the rseq extensible ABI implementation on all
> Linux architectures exposing the '__rseq_size' and '__rseq_offset'
> symbols to set the initial value of the 'cpu_id' field which can be used
> by applications to test if rseq is available and registered. As long as
> the symbols are exposed it is valid for an application to perform this
> test even if rseq is not yet implemented in libc for this architecture.
>
> Both code paths are compile tested with build-many-glibcs.py but I don't
> have access to any hardware to run the tests.
>
> Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
> ---
> Cc: Florian Weimer <fweimer@redhat.com>
> Cc: Yinyu Cai <caiyinyu@loongson.cn>
> Cc: mengqinggang <mengqinggang@loongson.cn>

Once again, I don't have the hardware but am reviewing this for being
a reasonable looking change and for passing build-many-glibcs.

Reviewed-by: Arjun Shankar <arjun@redhat.com>

> ---
>  sysdeps/loongarch/nptl/thread_pointer.h | 36 +++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 sysdeps/loongarch/nptl/thread_pointer.h
>
> diff --git a/sysdeps/loongarch/nptl/thread_pointer.h b/sysdeps/loongarch/nptl/thread_pointer.h
> new file mode 100644
> index 0000000000..5dec2ef4c6
> --- /dev/null
> +++ b/sysdeps/loongarch/nptl/thread_pointer.h

OK. Good file location and name.

> @@ -0,0 +1,36 @@
> +/* __thread_pointer definition.  loongarch version.
> +   Copyright (C) 2021-2024 Free Software Foundation, Inc.

OK because it's a near copy of a similar file with the same copyright line.

> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library.  If not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#ifndef _SYS_THREAD_POINTER_H
> +#define _SYS_THREAD_POINTER_H

OK.

> +
> +#include <sys/cdefs.h>

OK. Needed for glibc_has_builtin.

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

OK. If the builtin exists, use it. Otherwise emit assembly. Looks
reasonable to me.

> +}
> +
> +#endif /* _SYS_THREAD_POINTER_H */

OK.

> --
> 2.43.0
>
  
Michael Jeanson Oct. 30, 2024, 6:45 p.m. UTC | #2
On 2024-10-30 11:21, Arjun Shankar wrote:
>> +
>> +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
> 
> OK. If the builtin exists, use it. Otherwise emit assembly. Looks
> reasonable to me.

Upon further testing on architectures where the current GCC doesn't
support __builtin_thread_pointer, it seems that __glibc_has_builtin
doesn't behave as I had expected since I get the following compiler
error:

../sysdeps/csky/nptl/thread_pointer.h:28:10: error: ‘__builtin_thread_pointer’ is not supported on this target
   28 |   return __builtin_thread_pointer ();
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~

So at least in some situations __glibc_has_builtin will evaluate to
true even if the backend doesn't support the builtin. Unless someone
has a better idea on how to test for the availability of
__builtin_thread_pointer I'll have to revert to GCC version checks
specific to each architecture.

Thoughts?
  
Adhemerval Zanella Netto Oct. 30, 2024, 8:23 p.m. UTC | #3
On 30/10/24 15:45, Michael Jeanson wrote:
> On 2024-10-30 11:21, Arjun Shankar wrote:
>>> +
>>> +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
>>
>> OK. If the builtin exists, use it. Otherwise emit assembly. Looks
>> reasonable to me.
> 
> Upon further testing on architectures where the current GCC doesn't
> support __builtin_thread_pointer, it seems that __glibc_has_builtin
> doesn't behave as I had expected since I get the following compiler
> error:
> 
> ../sysdeps/csky/nptl/thread_pointer.h:28:10: error: ‘__builtin_thread_pointer’ is not supported on this target
>    28 |   return __builtin_thread_pointer ();
>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> So at least in some situations __glibc_has_builtin will evaluate to
> true even if the backend doesn't support the builtin. Unless someone
> has a better idea on how to test for the availability of
> __builtin_thread_pointer I'll have to revert to GCC version checks
> specific to each architecture.
> 
> Thoughts?

I think we should report this to gcc, since it makes the __has_builtin
useless if the backend will just issue a compiler error.  For glibc
I think we will need to rely on the inline asm for the architectures
gcc does not support it.
  
Michael Jeanson Oct. 30, 2024, 8:31 p.m. UTC | #4
On 2024-10-30 16:23, Adhemerval Zanella Netto wrote:
> I think we should report this to gcc, since it makes the __has_builtin
> useless if the backend will just issue a compiler error.  For glibc
> I think we will need to rely on the inline asm for the architectures
> gcc does not support it.

A configure check for __builtin_thread_pointer could be another solution
as long thread_pointer.h remains a non-installed header. Similar checks
currently exists for __builtin_memset and __builtin_strstr.

Looks like it was reported [1] in 2020 but there was no solution proposed.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96952
  
Adhemerval Zanella Netto Oct. 30, 2024, 8:40 p.m. UTC | #5
On 30/10/24 17:31, Michael Jeanson wrote:
> On 2024-10-30 16:23, Adhemerval Zanella Netto wrote:
>> I think we should report this to gcc, since it makes the __has_builtin
>> useless if the backend will just issue a compiler error.  For glibc
>> I think we will need to rely on the inline asm for the architectures
>> gcc does not support it.
> 
> A configure check for __builtin_thread_pointer could be another solution
> as long thread_pointer.h remains a non-installed header. Similar checks
> currently exists for __builtin_memset and __builtin_strstr.
> 
> Looks like it was reported [1] in 2020 but there was no solution proposed.
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96952

A configure check might work, although it might require extra complexity if
the idea would to check if the backends lowers without a libcall (for the
case where it might be implemented in libgcc or a different library, although
not really implemented in any arch but it might be required for some archs
like arm32).

Is using the builtin really shows that much codegen improvement? Because
I really would prefer to just use inline asm and only enable the builtin
once gcc get this sort out.
  
Michael Jeanson Oct. 30, 2024, 8:54 p.m. UTC | #6
On 2024-10-30 16:40, Adhemerval Zanella Netto wrote:
> A configure check might work, although it might require extra complexity if
> the idea would to check if the backends lowers without a libcall (for the
> case where it might be implemented in libgcc or a different library, although
> not really implemented in any arch but it might be required for some archs
> like arm32).
> 
> Is using the builtin really shows that much codegen improvement? Because
> I really would prefer to just use inline asm and only enable the builtin
> once gcc get this sort out.

I don't think so but I'm not very familiar with all those architectures, I
just thought the compiler would 'know better' but since we can't reliably
test for the builtin I think only using the inline asm for the moment
would make sense.

Unless Florian has an objection, I can send a new version of the patches
with only the inline asm.
  
caiyinyu Oct. 31, 2024, 2:31 a.m. UTC | #7
OK.

We have agreed on the value of RSEQ_SIG with kernel.

diff --git a/sysdeps/unix/sysv/linux/loongarch/bits/rseq.h 
b/sysdeps/unix/sysv/linux/loongarch/bits/rseq.h new file mode 100644 
index 0000000000..7468a05243 --- /dev/null +++ 
b/sysdeps/unix/sysv/linux/loongarch/bits/rseq.h @@ -0,0 +1,36 @@ +/* 
Restartable Sequences Linux LoongArch architecture header. + Copyright 
(C) 2024 Free Software Foundation, Inc. + + The GNU C Library is free 
software; you can redistribute it and/or + modify it under the terms of 
the GNU Lesser General Public + License as published by the Free 
Software Foundation; either + version 2.1 of the License, or (at your 
option) any later version. + + The GNU C Library is distributed in the 
hope that it will be useful, + but WITHOUT ANY WARRANTY; without even 
the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR 
PURPOSE. See the GNU + Lesser General Public License for more details. + 
+ You should have received a copy of the GNU Lesser General Public + 
License along with the GNU C Library; if not, see + 
<https://www.gnu.org/licenses/>. */ + +#ifndef _SYS_RSEQ_H +# error 
"Never use <bits/rseq.h> directly; include <sys/rseq.h> instead." 
+#endif + +/* RSEQ_SIG is a signature required before each abort handler 
code. + + It is a 32-bit value that maps to actual architecture code 
compiled + into applications and libraries. It needs to be defined for 
each + architecture. When choosing this value, it needs to be taken into 
+ account that generating invalid instructions may have ill effects on + 
tools like objdump, and may also have impact on the CPU speculative + 
execution efficiency in some cases. + + RSEQ_SIG uses the following 
break instruction: + + 0x002a0010 break 0x10 +*/ + +#define RSEQ_SIG 
0x002a0010

在 2024/10/30 下午11:21, Arjun Shankar 写道:
> Hi Michael,
>
>> This will be required by the rseq extensible ABI implementation on all
>> Linux architectures exposing the '__rseq_size' and '__rseq_offset'
>> symbols to set the initial value of the 'cpu_id' field which can be used
>> by applications to test if rseq is available and registered. As long as
>> the symbols are exposed it is valid for an application to perform this
>> test even if rseq is not yet implemented in libc for this architecture.
>>
>> Both code paths are compile tested with build-many-glibcs.py but I don't
>> have access to any hardware to run the tests.
>>
>> Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
>> ---
>> Cc: Florian Weimer <fweimer@redhat.com>
>> Cc: Yinyu Cai <caiyinyu@loongson.cn>
>> Cc: mengqinggang <mengqinggang@loongson.cn>
> Once again, I don't have the hardware but am reviewing this for being
> a reasonable looking change and for passing build-many-glibcs.
>
> Reviewed-by: Arjun Shankar <arjun@redhat.com>
>
>> ---
>>   sysdeps/loongarch/nptl/thread_pointer.h | 36 +++++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>   create mode 100644 sysdeps/loongarch/nptl/thread_pointer.h
>>
>> diff --git a/sysdeps/loongarch/nptl/thread_pointer.h b/sysdeps/loongarch/nptl/thread_pointer.h
>> new file mode 100644
>> index 0000000000..5dec2ef4c6
>> --- /dev/null
>> +++ b/sysdeps/loongarch/nptl/thread_pointer.h
> OK. Good file location and name.
>
>> @@ -0,0 +1,36 @@
>> +/* __thread_pointer definition.  loongarch version.
>> +   Copyright (C) 2021-2024 Free Software Foundation, Inc.
> OK because it's a near copy of a similar file with the same copyright line.
>
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library.  If not, see
>> +   <https://www.gnu.org/licenses/>.  */
>> +
>> +#ifndef _SYS_THREAD_POINTER_H
>> +#define _SYS_THREAD_POINTER_H
> OK.
>
>> +
>> +#include <sys/cdefs.h>
> OK. Needed for glibc_has_builtin.
>
>> +
>> +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
> OK. If the builtin exists, use it. Otherwise emit assembly. Looks
> reasonable to me.
>
>> +}
>> +
>> +#endif /* _SYS_THREAD_POINTER_H */
> OK.
>
>> --
>> 2.43.0
>>
  
caiyinyu Oct. 31, 2024, 2:38 a.m. UTC | #8
Sorry, the content of the email on libc-alpha was messed up; this is a 
re-send.

We have agreed on the value of RSEQ_SIG with kernel.

patch link 
https://sourceware.org/pipermail/libc-alpha/2024-October/161108.html


在 2024/10/31 上午10:31, caiyinyu 写道:
>
> OK.
>
> We have agreed on the value of RSEQ_SIG with kernel.
>
> diff --git a/sysdeps/unix/sysv/linux/loongarch/bits/rseq.h 
> b/sysdeps/unix/sysv/linux/loongarch/bits/rseq.h new file mode 100644 
> index 0000000000..7468a05243 --- /dev/null +++ 
> b/sysdeps/unix/sysv/linux/loongarch/bits/rseq.h @@ -0,0 +1,36 @@ +/* 
> Restartable Sequences Linux LoongArch architecture header. + Copyright 
> (C) 2024 Free Software Foundation, Inc. + + The GNU C Library is free 
> software; you can redistribute it and/or + modify it under the terms 
> of the GNU Lesser General Public + License as published by the Free 
> Software Foundation; either + version 2.1 of the License, or (at your 
> option) any later version. + + The GNU C Library is distributed in the 
> hope that it will be useful, + but WITHOUT ANY WARRANTY; without even 
> the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR 
> PURPOSE. See the GNU + Lesser General Public License for more details. 
> + + You should have received a copy of the GNU Lesser General Public + 
> License along with the GNU C Library; if not, see + 
> <https://www.gnu.org/licenses/>. */ + +#ifndef _SYS_RSEQ_H +# error 
> "Never use <bits/rseq.h> directly; include <sys/rseq.h> instead." 
> +#endif + +/* RSEQ_SIG is a signature required before each abort 
> handler code. + + It is a 32-bit value that maps to actual 
> architecture code compiled + into applications and libraries. It needs 
> to be defined for each + architecture. When choosing this value, it 
> needs to be taken into + account that generating invalid instructions 
> may have ill effects on + tools like objdump, and may also have impact 
> on the CPU speculative + execution efficiency in some cases. + + 
> RSEQ_SIG uses the following break instruction: + + 0x002a0010 break 
> 0x10 +*/ + +#define RSEQ_SIG 0x002a0010
>
> 在 2024/10/30 下午11:21, Arjun Shankar 写道:
>> Hi Michael,
>>
>>> This will be required by the rseq extensible ABI implementation on all
>>> Linux architectures exposing the '__rseq_size' and '__rseq_offset'
>>> symbols to set the initial value of the 'cpu_id' field which can be used
>>> by applications to test if rseq is available and registered. As long as
>>> the symbols are exposed it is valid for an application to perform this
>>> test even if rseq is not yet implemented in libc for this architecture.
>>>
>>> Both code paths are compile tested with build-many-glibcs.py but I don't
>>> have access to any hardware to run the tests.
>>>
>>> Signed-off-by: Michael Jeanson<mjeanson@efficios.com>
>>> ---
>>> Cc: Florian Weimer<fweimer@redhat.com>
>>> Cc: Yinyu Cai<caiyinyu@loongson.cn>
>>> Cc: mengqinggang<mengqinggang@loongson.cn>
>> Once again, I don't have the hardware but am reviewing this for being
>> a reasonable looking change and for passing build-many-glibcs.
>>
>> Reviewed-by: Arjun Shankar<arjun@redhat.com>
>>
>>> ---
>>>   sysdeps/loongarch/nptl/thread_pointer.h | 36 +++++++++++++++++++++++++
>>>   1 file changed, 36 insertions(+)
>>>   create mode 100644 sysdeps/loongarch/nptl/thread_pointer.h
>>>
>>> diff --git a/sysdeps/loongarch/nptl/thread_pointer.h b/sysdeps/loongarch/nptl/thread_pointer.h
>>> new file mode 100644
>>> index 0000000000..5dec2ef4c6
>>> --- /dev/null
>>> +++ b/sysdeps/loongarch/nptl/thread_pointer.h
>> OK. Good file location and name.
>>
>>> @@ -0,0 +1,36 @@
>>> +/* __thread_pointer definition.  loongarch version.
>>> +   Copyright (C) 2021-2024 Free Software Foundation, Inc.
>> OK because it's a near copy of a similar file with the same copyright line.
>>
>>> +   This file is part of the GNU C Library.
>>> +
>>> +   The GNU C Library is free software; you can redistribute it and/or
>>> +   modify it under the terms of the GNU Lesser General Public
>>> +   License as published by the Free Software Foundation; either
>>> +   version 2.1 of the License, or (at your option) any later version.
>>> +
>>> +   The GNU C Library is distributed in the hope that it will be useful,
>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> +   Lesser General Public License for more details.
>>> +
>>> +   You should have received a copy of the GNU Lesser General Public
>>> +   License along with the GNU C Library.  If not, see
>>> +<https://www.gnu.org/licenses/>.  */
>>> +
>>> +#ifndef _SYS_THREAD_POINTER_H
>>> +#define _SYS_THREAD_POINTER_H
>> OK.
>>
>>> +
>>> +#include <sys/cdefs.h>
>> OK. Needed for glibc_has_builtin.
>>
>>> +
>>> +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
>> OK. If the builtin exists, use it. Otherwise emit assembly. Looks
>> reasonable to me.
>>
>>> +}
>>> +
>>> +#endif /* _SYS_THREAD_POINTER_H */
>> OK.
>>
>>> --
>>> 2.43.0
>>>
  

Patch

diff --git a/sysdeps/loongarch/nptl/thread_pointer.h b/sysdeps/loongarch/nptl/thread_pointer.h
new file mode 100644
index 0000000000..5dec2ef4c6
--- /dev/null
+++ b/sysdeps/loongarch/nptl/thread_pointer.h
@@ -0,0 +1,36 @@ 
+/* __thread_pointer definition.  loongarch version.
+   Copyright (C) 2021-2024 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library.  If not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#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 */