diff mbox series

[v4] nptl: Add <thread_pointer.h> for RISC-V

Message ID 20241016151610.1317813-1-mjeanson@efficios.com
State New
Headers show
Series [v4] nptl: Add <thread_pointer.h> for RISC-V | expand

Commit Message

Michael Jeanson Oct. 16, 2024, 3:16 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.

A cleaner solution for the fallback code would be to use a global
'register' variable, unfortunatly 'tls.h' a non-installed header already
defines '__thread_self' for the 'tp' register and GCC doesn't accept
multiple global register variables for the same register.

Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
---
Changes since v3:
- Revert to inline asm for the fallback code
Changes since v2:
- The 'register' variable has to be global to generate the correct asm
- Reuse '__thread_self' from 'tls.h'
Changes since v1:
- Use __glibc_has_builtin instead of GCC version check
- Use 'register' variable instead of inline asm
---
Cc: Florian Weimer <fweimer@redhat.com>
Cc: Palmer Dabbelt <palmer@rivosinc.com>
Cc: Darius Rad <darius@bluespec.com>
Cc: Jeff Law <jeffreyalaw@gmail.com>
---
 sysdeps/riscv/nptl/thread_pointer.h | 40 +++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 sysdeps/riscv/nptl/thread_pointer.h

Comments

Michael Jeanson Oct. 17, 2024, 5:58 p.m. UTC | #1 
On 2024-10-16 17:16, Michael Jeanson wrote:
> 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.
> 
> A cleaner solution for the fallback code would be to use a global
> 'register' variable, unfortunatly 'tls.h' a non-installed header already
> defines '__thread_self' for the 'tp' register and GCC doesn't accept
> multiple global register variables for the same register.

Would it make more sense to move the global register variable '__thread_self'
to the new 'thread_pointer.h' header and then include it in 'tls.h'?
Florian Weimer Oct. 17, 2024, 7:57 p.m. UTC | #2 
* Michael Jeanson:

> On 2024-10-16 17:16, Michael Jeanson wrote:
>> 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.
>> 
>> A cleaner solution for the fallback code would be to use a global
>> 'register' variable, unfortunatly 'tls.h' a non-installed header already
>> defines '__thread_self' for the 'tp' register and GCC doesn't accept
>> multiple global register variables for the same register.
>
> Would it make more sense to move the global register variable '__thread_self'
> to the new 'thread_pointer.h' header and then include it in 'tls.h'?

I think we should not define a global register value in an installed
header because it may be in conflict with what the application is
doing.

Hopefully this will be irrelvant some day because every compiler in
use supports__builtin_thread_pointer.
Michael Jeanson Oct. 17, 2024, 8:44 p.m. UTC | #3 
On 2024-10-17 21:57, Florian Weimer wrote:
> * Michael Jeanson:
> 
>> On 2024-10-16 17:16, Michael Jeanson wrote:
>>> 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.
>>>
>>> A cleaner solution for the fallback code would be to use a global
>>> 'register' variable, unfortunatly 'tls.h' a non-installed header already
>>> defines '__thread_self' for the 'tp' register and GCC doesn't accept
>>> multiple global register variables for the same register.
>>
>> Would it make more sense to move the global register variable '__thread_self'
>> to the new 'thread_pointer.h' header and then include it in 'tls.h'?
> 
> I think we should not define a global register value in an installed
> header because it may be in conflict with what the application is
> doing.
> 
> Hopefully this will be irrelvant some day because every compiler in
> use supports__builtin_thread_pointer.

I'm a bit confused, what defines an installed header and is <thread_pointer.h>
one? For example, the powerpc version of the header does define a global
register variable '__thread_register'.
Palmer Dabbelt Oct. 17, 2024, 8:46 p.m. UTC | #4 
On Thu, 17 Oct 2024 12:57:54 PDT (-0700), fw@deneb.enyo.de wrote:
> * Michael Jeanson:
>
>> On 2024-10-16 17:16, Michael Jeanson wrote:
>>> 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.
>>>
>>> A cleaner solution for the fallback code would be to use a global
>>> 'register' variable, unfortunatly 'tls.h' a non-installed header already
>>> defines '__thread_self' for the 'tp' register and GCC doesn't accept
>>> multiple global register variables for the same register.
>>
>> Would it make more sense to move the global register variable '__thread_self'
>> to the new 'thread_pointer.h' header and then include it in 'tls.h'?
>
> I think we should not define a global register value in an installed
> header because it may be in conflict with what the application is
> doing.

That seems reasonable to me.  The fallback inline asm doesn't seem all 
that ugly, so as long as it works everywhere I'm good with it.  Thus

Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
Acked-by: Palmer Dabbelt <palmer@rivosinc.com>

Thanks!

On a related note: Our internal_syscallN() macros have a similar flavor 
of local inline asm, I'm not sure if we're going to end up with a 
similar clang-related bug over there?

I also filed https://github.com/llvm/llvm-project/issues/112773 , as I 
couldn't find it anywhere else.

> Hopefully this will be irrelvant some day because every compiler in
> use supports__builtin_thread_pointer.
Florian Weimer Oct. 17, 2024, 8:58 p.m. UTC | #5 
* Michael Jeanson:

> On 2024-10-17 21:57, Florian Weimer wrote:
>> * Michael Jeanson:
>> 
>>> On 2024-10-16 17:16, Michael Jeanson wrote:
>>>> 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.
>>>>
>>>> A cleaner solution for the fallback code would be to use a global
>>>> 'register' variable, unfortunatly 'tls.h' a non-installed header already
>>>> defines '__thread_self' for the 'tp' register and GCC doesn't accept
>>>> multiple global register variables for the same register.
>>>
>>> Would it make more sense to move the global register variable '__thread_self'
>>> to the new 'thread_pointer.h' header and then include it in 'tls.h'?
>> 
>> I think we should not define a global register value in an installed
>> header because it may be in conflict with what the application is
>> doing.
>> 
>> Hopefully this will be irrelvant some day because every compiler in
>> use supports__builtin_thread_pointer.
>
> I'm a bit confused, what defines an installed header and is <thread_pointer.h>
> one? For example, the powerpc version of the header does define a global
> register variable '__thread_register'.

Sorry, I was confused.  We had some previous discussion about
providing a <sys/thread_pointer.h> header, but we never got around
installing it.  Installed headers are defined by listing them in the
“headers” variable in Makefiles.
Michael Jeanson Oct. 17, 2024, 9:13 p.m. UTC | #6 
On 2024-10-17 22:58, Florian Weimer wrote:
> Sorry, I was confused.  We had some previous discussion about
> providing a <sys/thread_pointer.h> header, but we never got around
> installing it.  Installed headers are defined by listing them in the
> “headers” variable in Makefiles.

If <sys/thread_pointer.h> is still a possibility I think it makes sense
to apply those constraints to our implementation of <thread_pointer.h>.

Otherwise the global register variable seems like a cleaner implementation.

My concern is not specific to riscv, I'm waiting for us to reach consensus
on this patch before I send patches for all the other architectures that
don't have a 'thread_pointer.h'.

Thanks,

Michael
Palmer Dabbelt Oct. 17, 2024, 9:24 p.m. UTC | #7 
On Thu, 17 Oct 2024 14:13:42 PDT (-0700), mjeanson@efficios.com wrote:
> On 2024-10-17 22:58, Florian Weimer wrote:
>> Sorry, I was confused.  We had some previous discussion about
>> providing a <sys/thread_pointer.h> header, but we never got around
>> installing it.  Installed headers are defined by listing them in the
>> “headers” variable in Makefiles.
>
> If <sys/thread_pointer.h> is still a possibility I think it makes sense
> to apply those constraints to our implementation of <thread_pointer.h>.
>
> Otherwise the global register variable seems like a cleaner implementation.
>
> My concern is not specific to riscv, I'm waiting for us to reach consensus
> on this patch before I send patches for all the other architectures that
> don't have a 'thread_pointer.h'.

I'm fine either way on the RISC-V side of things: it's just 2-3 lines of 
code in a single function (and maybe global variable), so it's not like 
there's a ton of complexity involved in supporting this.

> Thanks,
>
> Michael
Michael Jeanson Oct. 17, 2024, 9:32 p.m. UTC | #8 
On 2024-10-17 23:24, Palmer Dabbelt wrote:
> I'm fine either way on the RISC-V side of things: it's just 2-3 lines of code
> in a single function (and maybe global variable), so it's not like there's a
> ton of complexity involved in supporting this.

I guess what I was trying to say is I'd rather not have to deal with the
intricacies of inline assembly on multiple obscure architectures unless I
really have to ;) but it is indeed trivial for RISC-V.
Florian Weimer Oct. 18, 2024, 7:15 a.m. UTC | #9 
* Michael Jeanson:

> On 2024-10-17 22:58, Florian Weimer wrote:
>> Sorry, I was confused.  We had some previous discussion about
>> providing a <sys/thread_pointer.h> header, but we never got around
>> installing it.  Installed headers are defined by listing them in the
>> “headers” variable in Makefiles.
>
> If <sys/thread_pointer.h> is still a possibility I think it makes sense
> to apply those constraints to our implementation of <thread_pointer.h>.

I think we might still want to expose <sys/thread_pointer.h> once we
have broader architecture coverage.

> Otherwise the global register variable seems like a cleaner implementation.

We could still use the global register variable approach for glibc
itself.  The wrapper header in include/sys/thread_pointer.h could
redirect things to other headers.  On the other hand, the global
register variable approach will never be fully consistent across
architectures because many just don't have this type of thread pointer
register.
diff mbox series

Patch

diff --git a/sysdeps/riscv/nptl/thread_pointer.h b/sysdeps/riscv/nptl/thread_pointer.h
new file mode 100644
index 0000000000..d5dc76c2e2
--- /dev/null
+++ b/sysdeps/riscv/nptl/thread_pointer.h
@@ -0,0 +1,40 @@ 
+/* __thread_pointer definition.  riscv 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>
+
+#if __glibc_has_builtin (__builtin_thread_pointer)
+static inline void *
+__thread_pointer (void)
+{
+  return __builtin_thread_pointer ();
+}
+#else
+static inline void *
+__thread_pointer (void)
+{
+  void *__thread_register;
+  __asm__ ("mv %0, tp" : "=r" (__thread_register));
+  return __thread_register;
+}
+#endif
+
+#endif /* _SYS_THREAD_POINTER_H */