* [PATCH glibc 0/3] Restartable Sequences enablement
@ 2020-05-01 2:14 Mathieu Desnoyers via Libc-alpha
2020-05-01 2:14 ` [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) Mathieu Desnoyers via Libc-alpha
` (3 more replies)
0 siblings, 4 replies; 27+ messages in thread
From: Mathieu Desnoyers via Libc-alpha @ 2020-05-01 2:14 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha, Joseph Myers, Mathieu Desnoyers
Hi,
Please find the rseq enablement patchset for inclusion into
glibc.
Florian, if you find the patches OK and possibly with a bit of
tweaking of the commit messages, can you please commit them
to glibc ?
Thanks for the feedback!
Mathieu
Mathieu Desnoyers (3):
glibc: Perform rseq registration at C startup and thread creation
(v19)
glibc: sched_getcpu(): use rseq cpu_id TLS on Linux (v7)
rseq registration tests (v10)
NEWS | 10 +
elf/libc_early_init.c | 4 +
manual/threads.texi | 66 +++-
nptl/pthread_create.c | 13 +
sysdeps/generic/rseq-internal.h | 26 ++
sysdeps/unix/sysv/linux/Makefile | 11 +-
sysdeps/unix/sysv/linux/Versions | 1 +
sysdeps/unix/sysv/linux/aarch64/bits/rseq.h | 43 +++
sysdeps/unix/sysv/linux/aarch64/libc.abilist | 1 +
sysdeps/unix/sysv/linux/alpha/libc.abilist | 1 +
sysdeps/unix/sysv/linux/arm/be/libc.abilist | 1 +
sysdeps/unix/sysv/linux/arm/bits/rseq.h | 83 +++++
sysdeps/unix/sysv/linux/arm/le/libc.abilist | 1 +
sysdeps/unix/sysv/linux/bits/rseq.h | 29 ++
sysdeps/unix/sysv/linux/csky/libc.abilist | 1 +
sysdeps/unix/sysv/linux/hppa/libc.abilist | 1 +
sysdeps/unix/sysv/linux/i386/libc.abilist | 1 +
sysdeps/unix/sysv/linux/ia64/libc.abilist | 1 +
.../sysv/linux/m68k/coldfire/libc.abilist | 1 +
.../unix/sysv/linux/m68k/m680x0/libc.abilist | 1 +
.../sysv/linux/microblaze/be/libc.abilist | 1 +
.../sysv/linux/microblaze/le/libc.abilist | 1 +
sysdeps/unix/sysv/linux/mips/bits/rseq.h | 62 ++++
.../sysv/linux/mips/mips32/fpu/libc.abilist | 1 +
.../sysv/linux/mips/mips32/nofpu/libc.abilist | 1 +
.../sysv/linux/mips/mips64/n32/libc.abilist | 1 +
.../sysv/linux/mips/mips64/n64/libc.abilist | 1 +
sysdeps/unix/sysv/linux/nios2/libc.abilist | 1 +
sysdeps/unix/sysv/linux/powerpc/bits/rseq.h | 37 ++
.../linux/powerpc/powerpc32/fpu/libc.abilist | 1 +
.../powerpc/powerpc32/nofpu/libc.abilist | 1 +
.../linux/powerpc/powerpc64/be/libc.abilist | 1 +
.../linux/powerpc/powerpc64/le/libc.abilist | 1 +
.../unix/sysv/linux/riscv/rv64/libc.abilist | 1 +
sysdeps/unix/sysv/linux/rseq-internal.h | 73 ++++
sysdeps/unix/sysv/linux/rseq-sym.c | 26 ++
sysdeps/unix/sysv/linux/s390/bits/rseq.h | 37 ++
.../unix/sysv/linux/s390/s390-32/libc.abilist | 1 +
.../unix/sysv/linux/s390/s390-64/libc.abilist | 1 +
sysdeps/unix/sysv/linux/sched_getcpu.c | 27 +-
sysdeps/unix/sysv/linux/sh/be/libc.abilist | 1 +
sysdeps/unix/sysv/linux/sh/le/libc.abilist | 1 +
.../sysv/linux/sparc/sparc32/libc.abilist | 1 +
.../sysv/linux/sparc/sparc64/libc.abilist | 1 +
sysdeps/unix/sysv/linux/sys/rseq.h | 207 +++++++++++
sysdeps/unix/sysv/linux/tst-rseq-nptl.c | 338 ++++++++++++++++++
sysdeps/unix/sysv/linux/tst-rseq.c | 108 ++++++
sysdeps/unix/sysv/linux/x86/bits/rseq.h | 30 ++
.../unix/sysv/linux/x86_64/64/libc.abilist | 1 +
.../unix/sysv/linux/x86_64/x32/libc.abilist | 1 +
50 files changed, 1253 insertions(+), 8 deletions(-)
create mode 100644 sysdeps/generic/rseq-internal.h
create mode 100644 sysdeps/unix/sysv/linux/aarch64/bits/rseq.h
create mode 100644 sysdeps/unix/sysv/linux/arm/bits/rseq.h
create mode 100644 sysdeps/unix/sysv/linux/bits/rseq.h
create mode 100644 sysdeps/unix/sysv/linux/mips/bits/rseq.h
create mode 100644 sysdeps/unix/sysv/linux/powerpc/bits/rseq.h
create mode 100644 sysdeps/unix/sysv/linux/rseq-internal.h
create mode 100644 sysdeps/unix/sysv/linux/rseq-sym.c
create mode 100644 sysdeps/unix/sysv/linux/s390/bits/rseq.h
create mode 100644 sysdeps/unix/sysv/linux/sys/rseq.h
create mode 100644 sysdeps/unix/sysv/linux/tst-rseq-nptl.c
create mode 100644 sysdeps/unix/sysv/linux/tst-rseq.c
create mode 100644 sysdeps/unix/sysv/linux/x86/bits/rseq.h
--
2.17.1
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) 2020-05-01 2:14 [PATCH glibc 0/3] Restartable Sequences enablement Mathieu Desnoyers via Libc-alpha @ 2020-05-01 2:14 ` Mathieu Desnoyers via Libc-alpha 2020-05-20 11:40 ` Florian Weimer via Libc-alpha 2020-05-01 2:14 ` [PATCH glibc 2/3] glibc: sched_getcpu(): use rseq cpu_id TLS on Linux (v7) Mathieu Desnoyers via Libc-alpha ` (2 subsequent siblings) 3 siblings, 1 reply; 27+ messages in thread From: Mathieu Desnoyers via Libc-alpha @ 2020-05-01 2:14 UTC (permalink / raw) To: Florian Weimer Cc: Rich Felker, libc-alpha, linux-api, Boqun Feng, Will Deacon, linux-kernel, Peter Zijlstra, Ben Maurer, Mathieu Desnoyers, Dave Watson, Thomas Gleixner, Paul E. McKenney, Paul Turner, Joseph Myers Register rseq TLS for each thread (including main), and unregister for each thread (excluding main). "rseq" stands for Restartable Sequences. See the rseq(2) man page proposed here: https://lkml.org/lkml/2018/9/19/647 Those are based on glibc master branch commit 6f0baacf0f89. The rseq system call was merged into Linux 4.18. CC: Carlos O'Donell <carlos@redhat.com> CC: Florian Weimer <fweimer@redhat.com> CC: Joseph Myers <joseph@codesourcery.com> CC: Szabolcs Nagy <szabolcs.nagy@arm.com> CC: Thomas Gleixner <tglx@linutronix.de> CC: Ben Maurer <bmaurer@fb.com> CC: Peter Zijlstra <peterz@infradead.org> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> CC: Boqun Feng <boqun.feng@gmail.com> CC: Will Deacon <will.deacon@arm.com> CC: Dave Watson <davejwatson@fb.com> CC: Paul Turner <pjt@google.com> CC: Rich Felker <dalias@libc.org> CC: libc-alpha@sourceware.org CC: linux-kernel@vger.kernel.org CC: linux-api@vger.kernel.org --- Changes since v1: - Move __rseq_refcount to an extra field at the end of __rseq_abi to eliminate one symbol. All libraries/programs which try to register rseq (glibc, early-adopter applications, early-adopter libraries) should use the rseq refcount. It becomes part of the ABI within a user-space process, but it's not part of the ABI shared with the kernel per se. - Restructure how this code is organized so glibc keeps building on non-Linux targets. - Use non-weak symbol for __rseq_abi. - Move rseq registration/unregistration implementation into its own nptl/rseq.c compile unit. - Move __rseq_abi symbol under GLIBC_2.29. Changes since v2: - Move __rseq_refcount to its own symbol, which is less ugly than trying to play tricks with the rseq uapi. - Move __rseq_abi from nptl to csu (C start up), so it can be used across glibc, including memory allocator and sched_getcpu(). The __rseq_refcount symbol is kept in nptl, because there is no reason to use it elsewhere in glibc. Changes since v3: - Set __rseq_refcount TLS to 1 on register/set to 0 on unregister because glibc is the first/last user. - Unconditionally register/unregister rseq at thread start/exit, because glibc is the first/last user. - Add missing abilist items. - Rebase on glibc master commit a502c5294. - Add NEWS entry. Changes since v4: - Do not use "weak" symbols for __rseq_abi and __rseq_refcount. Based on "System V Application Binary Interface", weak only affects the link editor, not the dynamic linker. - Install a new sys/rseq.h system header on Linux, which contains the RSEQ_SIG definition, __rseq_abi declaration and __rseq_refcount declaration. Move those definition/declarations from rseq-internal.h to the installed sys/rseq.h header. - Considering that rseq is only available on Linux, move csu/rseq.c to sysdeps/unix/sysv/linux/rseq-sym.c. - Move __rseq_refcount from nptl/rseq.c to sysdeps/unix/sysv/linux/rseq-sym.c, so it is only defined on Linux. - Move both ABI definitions for __rseq_abi and __rseq_refcount to sysdeps/unix/sysv/linux/Versions, so they only appear on Linux. - Document __rseq_abi and __rseq_refcount volatile. - Document the RSEQ_SIG signature define. - Move registration functions from rseq.c to rseq-internal.h static inline functions. Introduce empty stubs in misc/rseq-internal.h, which can be overridden by architecture code in sysdeps/unix/sysv/linux/rseq-internal.h. - Rename __rseq_register_current_thread and __rseq_unregister_current_thread to rseq_register_current_thread and rseq_unregister_current_thread, now that those are only visible as internal static inline functions. - Invoke rseq_register_current_thread() from libc-start.c LIBC_START_MAIN rather than nptl init, so applications not linked against libpthread.so have rseq registered for their main() thread. Note that it is invoked separately for SHARED and !SHARED builds. Changes since v5: - Replace __rseq_refcount by __rseq_lib_abi, which contains two uint32_t: register_state and refcount. The "register_state" field allows inhibiting rseq registration from signal handlers nested on top of glibc registration and occuring after rseq unregistration by glibc. - Introduce enum rseq_register_state, which contains the states allowed for the struct rseq_lib_abi register_state field. Changes since v6: - Introduce bits/rseq.h to define RSEQ_SIG for each architecture. The generic bits/rseq.h does not define RSEQ_SIG, meaning that each architecture implementing rseq needs to implement bits/rseq.h. - Rename enum item RSEQ_REGISTER_NESTED to RSEQ_REGISTER_ONGOING. - Port to glibc-2.29. Changes since v7: - Remove __rseq_lib_abi symbol, including refcount and register_state fields. - Remove reference counting and nested signals handling from registration/unregistration functions. - Introduce new __rseq_handled exported symbol, which is set to 1 by glibc on C startup when it handles restartable sequences. This allows glibc to coexist with early adopter libraries and applications wishing to register restartable sequences when it is not handled by glibc. - Introduce rseq_init (), which sets __rseq_handled to 1 from C startup. - Update NEWS entry. - Update comments at the beginning of new files. - Registration depends on both __NR_rseq and RSEQ_SIG. - Remove ARM, powerpc, MIPS RSEQ_SIG until we agree with maintainers on the signature choice. - Update x86, s390 RSEQ_SIG based on discussion with arch maintainers. - Remove rseq-internal.h from headers list of misc/Makefile, so it it not installed by make install. Changes since v8: - Introduce RSEQ_SIG_CODE and RSEQ_SIG_DATA on aarch64 to handle compiling with -mbig-endian. Changes since v9: - Update Changelog. - Remove unneeded new file comment header newlines. Changes since v10: - Remove volatile from __rseq_abi declaration. - Document that __rseq_handled is about library managing rseq registration, independently of whether rseq is available or not. - Move __rseq_handled symbol to ld.so, initialize this symbol within the dynamic linker initialization for both shared (rtld.c) and static (dl-support.c) builds. - Only register the rseq TLS on initialization once in multiple-libc scenarios. Use rtld_active () for this purpose. - In the static libc case, register the rseq TLS after LD_PRELOAD constructors are run, so it matches the order of this initialization vs LD_PRELOAD contructors execution for the shared libc. - Agreed on signature choice with powerpc and MIPS maintainers, re-adding those signatures, - The main architecture still left out signature-wise is ARM32. Changes since v11: - Rebase on glibc 2.30. - Re-introduce ARM RSEQ_SIG following feedback from Will Deacon. Changes since v12: - Remove __rseq_handled, - Rely on OS implicit rseq unregistration on thread teardown, - Register main thread in __libc_early_init (). - Add Restartable Sequences entry to threads manual. Changes since v13: - Update following be/le abilist split for arm, microblaze, and sh. - Update manual to add the __rseq_abi variable and RSEQ_SIG macro to generate manual index entries, and add missing "Restartable Sequences" menu entry to the threads chapter. Changes since v14: - Update copyright range to include 2020. - Introduce __ASSUME_RSEQ defined for --enable-kernel=4.18.0 and higher. - Use ifdef __ASSUME_RSEQ rather than ifdef __NR_rseq to discover rseq availability. This is necessary now that the system call numbers are integrated within glibc. Changes since v15: - Remove __ASSUME_RSEQ from kernel features. - rseq internal: remove assume rseq - remove assume rseq and struct rseq def from sysdeps/unix/sysv/linux/rseq-sym.c - sys/rseq.h: detect rseq header, implement fallback - sysdeps/unix/sysv/linux/sys/rseq.h include cdefs.h, add _Static_assert to validate struct rseq and struct rseq_cs alignment. - sys/rseq.h: document that posix_memalign should be used rather than malloc if allocating struct rseq or struct rseq_cs on the heap. This is required to guarantee 32-byte alignement. Changes since v16: - Move rseq NEWS entry under 2.32. - Move new __rseq_abi symbol to GLIBC_2.32. Changes since v17: - Change copyright year to 2020. - Refer to GNU C Library manual rather than rseq manpage in NEWS. - Use "initial" parameter from __libc_early_init (). - Manual: rseq is Linux rather than GNU std. - Remove rseq_unregister_current_thread () (unused). - rseq_register_current_thread () returns void. - Coding style fixes. - sys/rseq.h: use "32" for alignment. - Change http:// for https:// in comments. - Add const struct rseq_cs * field to rseq_cs union. Changes since v18: - NEWS update. - Manual update. - Move misc/rseq-internal.h to sysdeps/generic/rseq-internal.h. - Fix coding style in sysdeps/unix/sysv/linux/sys/rseq.h. - Abort libc if __rseq_abi is already initialized in rseq_register_current_thread (), which is unexpected. - Abort libc in rseq_register_current_thread () on all errno except EPERM (seccomp), EACCES (seccomp), and ENOSYS (not implemented). Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> CC: Carlos O'Donell <carlos@redhat.com> CC: Florian Weimer <fweimer@redhat.com> CC: Joseph Myers <joseph@codesourcery.com> CC: Szabolcs Nagy <szabolcs.nagy@arm.com> CC: Thomas Gleixner <tglx@linutronix.de> CC: Ben Maurer <bmaurer@fb.com> CC: Peter Zijlstra <peterz@infradead.org> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> CC: Boqun Feng <boqun.feng@gmail.com> CC: Will Deacon <will.deacon@arm.com> CC: Paul Turner <pjt@google.com> CC: Rich Felker <dalias@libc.org> CC: libc-alpha@sourceware.org CC: linux-kernel@vger.kernel.org CC: linux-api@vger.kernel.org --- NEWS | 10 + elf/libc_early_init.c | 4 + manual/threads.texi | 66 +++++- nptl/pthread_create.c | 13 ++ sysdeps/generic/rseq-internal.h | 26 +++ sysdeps/unix/sysv/linux/Makefile | 5 +- sysdeps/unix/sysv/linux/Versions | 1 + sysdeps/unix/sysv/linux/aarch64/bits/rseq.h | 43 ++++ sysdeps/unix/sysv/linux/aarch64/libc.abilist | 1 + sysdeps/unix/sysv/linux/alpha/libc.abilist | 1 + sysdeps/unix/sysv/linux/arm/be/libc.abilist | 1 + sysdeps/unix/sysv/linux/arm/bits/rseq.h | 83 +++++++ sysdeps/unix/sysv/linux/arm/le/libc.abilist | 1 + sysdeps/unix/sysv/linux/bits/rseq.h | 29 +++ sysdeps/unix/sysv/linux/csky/libc.abilist | 1 + sysdeps/unix/sysv/linux/hppa/libc.abilist | 1 + sysdeps/unix/sysv/linux/i386/libc.abilist | 1 + sysdeps/unix/sysv/linux/ia64/libc.abilist | 1 + .../sysv/linux/m68k/coldfire/libc.abilist | 1 + .../unix/sysv/linux/m68k/m680x0/libc.abilist | 1 + .../sysv/linux/microblaze/be/libc.abilist | 1 + .../sysv/linux/microblaze/le/libc.abilist | 1 + sysdeps/unix/sysv/linux/mips/bits/rseq.h | 62 ++++++ .../sysv/linux/mips/mips32/fpu/libc.abilist | 1 + .../sysv/linux/mips/mips32/nofpu/libc.abilist | 1 + .../sysv/linux/mips/mips64/n32/libc.abilist | 1 + .../sysv/linux/mips/mips64/n64/libc.abilist | 1 + sysdeps/unix/sysv/linux/nios2/libc.abilist | 1 + sysdeps/unix/sysv/linux/powerpc/bits/rseq.h | 37 ++++ .../linux/powerpc/powerpc32/fpu/libc.abilist | 1 + .../powerpc/powerpc32/nofpu/libc.abilist | 1 + .../linux/powerpc/powerpc64/be/libc.abilist | 1 + .../linux/powerpc/powerpc64/le/libc.abilist | 1 + .../unix/sysv/linux/riscv/rv64/libc.abilist | 1 + sysdeps/unix/sysv/linux/rseq-internal.h | 73 ++++++ sysdeps/unix/sysv/linux/rseq-sym.c | 26 +++ sysdeps/unix/sysv/linux/s390/bits/rseq.h | 37 ++++ .../unix/sysv/linux/s390/s390-32/libc.abilist | 1 + .../unix/sysv/linux/s390/s390-64/libc.abilist | 1 + sysdeps/unix/sysv/linux/sh/be/libc.abilist | 1 + sysdeps/unix/sysv/linux/sh/le/libc.abilist | 1 + .../sysv/linux/sparc/sparc32/libc.abilist | 1 + .../sysv/linux/sparc/sparc64/libc.abilist | 1 + sysdeps/unix/sysv/linux/sys/rseq.h | 207 ++++++++++++++++++ sysdeps/unix/sysv/linux/x86/bits/rseq.h | 30 +++ .../unix/sysv/linux/x86_64/64/libc.abilist | 1 + .../unix/sysv/linux/x86_64/x32/libc.abilist | 1 + 47 files changed, 778 insertions(+), 4 deletions(-) create mode 100644 sysdeps/generic/rseq-internal.h create mode 100644 sysdeps/unix/sysv/linux/aarch64/bits/rseq.h create mode 100644 sysdeps/unix/sysv/linux/arm/bits/rseq.h create mode 100644 sysdeps/unix/sysv/linux/bits/rseq.h create mode 100644 sysdeps/unix/sysv/linux/mips/bits/rseq.h create mode 100644 sysdeps/unix/sysv/linux/powerpc/bits/rseq.h create mode 100644 sysdeps/unix/sysv/linux/rseq-internal.h create mode 100644 sysdeps/unix/sysv/linux/rseq-sym.c create mode 100644 sysdeps/unix/sysv/linux/s390/bits/rseq.h create mode 100644 sysdeps/unix/sysv/linux/sys/rseq.h create mode 100644 sysdeps/unix/sysv/linux/x86/bits/rseq.h diff --git a/NEWS b/NEWS index 141078c319..c4e0370fc4 100644 --- a/NEWS +++ b/NEWS @@ -23,6 +23,16 @@ Major new features: toolchains. It is recommended to use GCC 8 or newer when testing this option. +* Support for automatically registering threads with the Linux rseq + system call has been added. This system call is implemented starting + from Linux 4.18. The Restartable Sequences ABI accelerates user-space + operations on per-cpu data. It allows user-space to perform updates + on per-cpu data without requiring heavy-weight atomic operations. + Automatically registering threads allows all libraries, including libc, + to make immediate use of the rseq(2) support by using the documented ABI. + The GNU C Library manual has details on integration of Restartable + Sequences. + Deprecated and removed features, and other changes affecting compatibility: * The deprecated <sys/sysctl.h> header and the sysctl function have been diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c index e6c64fb526..f0fcf6448e 100644 --- a/elf/libc_early_init.c +++ b/elf/libc_early_init.c @@ -18,10 +18,14 @@ #include <ctype.h> #include <libc-early-init.h> +#include <rseq-internal.h> void __libc_early_init (_Bool initial) { /* Initialize ctype data. */ __ctype_init (); + /* Register rseq ABI to the kernel for the main program's libc. */ + if (initial) + rseq_register_current_thread (); } diff --git a/manual/threads.texi b/manual/threads.texi index 0858ef8f92..a565095c43 100644 --- a/manual/threads.texi +++ b/manual/threads.texi @@ -9,8 +9,10 @@ This chapter describes functions used for managing threads. POSIX threads. @menu -* ISO C Threads:: Threads based on the ISO C specification. -* POSIX Threads:: Threads based on the POSIX specification. +* ISO C Threads:: Threads based on the ISO C specification. +* POSIX Threads:: Threads based on the POSIX specification. +* Restartable Sequences:: Linux-specific Restartable Sequences + integration. @end menu @@ -881,3 +883,63 @@ Behaves like @code{pthread_timedjoin_np} except that the absolute time in @c pthread_spin_unlock @c pthread_testcancel @c pthread_yield + +@node Restartable Sequences +@section Restartable Sequences +@cindex Restartable Sequences + +This section describes Restartable Sequences integration for +@theglibc{}. + +@deftypevar {struct rseq} __rseq_abi +@standards{Linux, sys/rseq.h} +@Theglibc{} implements a @code{__rseq_abi} TLS symbol to interact with the +Restartable Sequences system call (Linux-specific). The layout of this +structure is defined by the @file{sys/rseq.h} header. Registration of each +thread's @code{__rseq_abi} is performed by @theglibc{} at library +initialization and thread creation. + +The main executable and shared libraries may either have an undefined +@code{__rseq_abi} TLS symbol, or define their own, with the same +declaration as the one present in @file{sys/rseq.h}. The dynamic linker +will ensure that only one of those available symbols will be used at +runtime across the process. + +If the main executable or shared libraries observe an uninitialized +@code{__rseq_abi.cpu_id} field (value @code{RSEQ_CPU_ID_UNINITIALIZED}), they +may perform rseq registration to the kernel: this means either glibc was +prevented from doing the registration, or an older glibc version, which does +not include rseq support, is in use. When the main executable or a library +thus takes ownership of the registration, the memory used to hold the +@code{__rseq_abi} TLS variable must stay allocated, and is not re-used, until +the very end of the thread lifetime or until an explicit rseq unregistration +for that thread is performed. It is not recommended to @code{dlclose} libraries +owning the @code{__rseq_abi} TLS variable. + +Users of the @code{__rseq_abi} TLS symbol can store the address of a +@code{struct rseq_cs} to the @code{__rseq_abi.rseq_cs.uptr.ptr} TLS variable, +thus informing the kernel that it enters a Restartable Sequence critical +section. This pointer and the code areas it itself points to must not be left +pointing to memory areas which are freed or re-used. Several approaches can +guarantee this. If the application or library can guarantee that the memory +used to hold the @code{struct rseq_cs} and the code areas it refers to are +never freed or re-used, no special action must be taken. Else, before that +memory is re-used of freed, the application is responsible for setting the +@code{__rseq_abi.rseq_cs.uptr.ptr} TLS variable to @code{NULL} in each thread's +TLS to guarantee that it does not leak dangling references. Because the +application does not typically have knowledge of libraries' use of Restartable +Sequences, it is recommended that libraries using Restartable Sequences which +may end up freeing or re-using their memory set the +@code{__rseq_abi.rseq_cs.uptr.ptr} TLS variable to @code{NULL} before returning +from library functions which use Restartable Sequences. + +@end deftypevar + +@deftypevr Macro int RSEQ_SIG +@standards{Linux, sys/rseq.h} +Each supported architecture provides a @code{RSEQ_SIG} macro in +@file{sys/rseq.h} which contains a signature. That signature is expected to be +present in the code before each Restartable Sequences abort handler. Failure +to provide the expected signature may terminate the process with a segmentation +fault. +@end deftypevr diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c index afd379e89a..6dacb0e284 100644 --- a/nptl/pthread_create.c +++ b/nptl/pthread_create.c @@ -33,6 +33,7 @@ #include <default-sched.h> #include <futex-internal.h> #include <tls-setup.h> +#include <rseq-internal.h> #include "libioP.h" #include <shlib-compat.h> @@ -384,6 +385,9 @@ START_THREAD_DEFN /* Initialize pointers to locale data. */ __ctype_init (); + /* Register rseq TLS to the kernel. */ + rseq_register_current_thread (); + #ifndef __ASSUME_SET_ROBUST_LIST if (__set_robust_list_avail >= 0) #endif @@ -578,6 +582,15 @@ START_THREAD_DEFN process is really dead since 'clone' got passed the CLONE_CHILD_CLEARTID flag. The 'tid' field in the TCB will be set to zero. + rseq TLS is still registered at this point. Rely on implicit + unregistration performed by the kernel on thread teardown. This is not a + problem because the rseq TLS lives on the stack, and the stack outlives + the thread. If TCB allocation is ever changed, additional steps may be + required, such as performing explicit rseq unregistration before + reclaiming the rseq TLS area memory. It is NOT sufficient to block + signals because the kernel may write to the rseq area even without + signals. + The exit code is zero since in case all threads exit by calling 'pthread_exit' the exit status must be 0 (zero). */ __exit_thread (); diff --git a/sysdeps/generic/rseq-internal.h b/sysdeps/generic/rseq-internal.h new file mode 100644 index 0000000000..16f197397f --- /dev/null +++ b/sysdeps/generic/rseq-internal.h @@ -0,0 +1,26 @@ +/* Restartable Sequences internal API. Stub version. + Copyright (C) 2020 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 RSEQ_INTERNAL_H +#define RSEQ_INTERNAL_H + +static inline void +rseq_register_current_thread (void) +{ +} + +#endif /* rseq-internal.h */ diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile index 0326f92c40..e3ed374d9e 100644 --- a/sysdeps/unix/sysv/linux/Makefile +++ b/sysdeps/unix/sysv/linux/Makefile @@ -41,7 +41,7 @@ update-syscall-lists: arch-syscall.h endif ifeq ($(subdir),csu) -sysdep_routines += errno-loc +sysdep_routines += errno-loc rseq-sym endif ifeq ($(subdir),assert) @@ -92,7 +92,8 @@ sysdep_headers += sys/mount.h sys/acct.h \ bits/termios-c_lflag.h bits/termios-tcflow.h \ bits/termios-misc.h \ bits/types/struct_semid_ds.h \ - bits/ipc-perm.h + bits/ipc-perm.h \ + sys/rseq.h bits/rseq.h tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \ tst-quota tst-sync_file_range tst-sysconf-iov_max tst-ttyname \ diff --git a/sysdeps/unix/sysv/linux/Versions b/sysdeps/unix/sysv/linux/Versions index 9a58dda9f2..52ca223ab2 100644 --- a/sysdeps/unix/sysv/linux/Versions +++ b/sysdeps/unix/sysv/linux/Versions @@ -178,6 +178,7 @@ libc { getdents64; gettid; tgkill; } GLIBC_2.32 { + __rseq_abi; } GLIBC_PRIVATE { # functions used in other libraries diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/rseq.h b/sysdeps/unix/sysv/linux/aarch64/bits/rseq.h new file mode 100644 index 0000000000..37d83fcb4a --- /dev/null +++ b/sysdeps/unix/sysv/linux/aarch64/bits/rseq.h @@ -0,0 +1,43 @@ +/* Restartable Sequences Linux aarch64 architecture header. + Copyright (C) 2020 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. + + aarch64 -mbig-endian generates mixed endianness code vs data: + little-endian code and big-endian data. Ensure the RSEQ_SIG signature + matches code endianness. */ + +#define RSEQ_SIG_CODE 0xd428bc00 /* BRK #0x45E0. */ + +#ifdef __AARCH64EB__ +#define RSEQ_SIG_DATA 0x00bc28d4 /* BRK #0x45E0. */ +#else +#define RSEQ_SIG_DATA RSEQ_SIG_CODE +#endif + +#define RSEQ_SIG RSEQ_SIG_DATA diff --git a/sysdeps/unix/sysv/linux/aarch64/libc.abilist b/sysdeps/unix/sysv/linux/aarch64/libc.abilist index 41bb214bb9..0c9cefbada 100644 --- a/sysdeps/unix/sysv/linux/aarch64/libc.abilist +++ b/sysdeps/unix/sysv/linux/aarch64/libc.abilist @@ -2146,4 +2146,5 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F diff --git a/sysdeps/unix/sysv/linux/alpha/libc.abilist b/sysdeps/unix/sysv/linux/alpha/libc.abilist index 6430af207f..53c18cf47d 100644 --- a/sysdeps/unix/sysv/linux/alpha/libc.abilist +++ b/sysdeps/unix/sysv/linux/alpha/libc.abilist @@ -2226,6 +2226,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 _IO_fprintf F GLIBC_2.4 _IO_printf F diff --git a/sysdeps/unix/sysv/linux/arm/be/libc.abilist b/sysdeps/unix/sysv/linux/arm/be/libc.abilist index f4ea1756d5..29b02fc165 100644 --- a/sysdeps/unix/sysv/linux/arm/be/libc.abilist +++ b/sysdeps/unix/sysv/linux/arm/be/libc.abilist @@ -133,6 +133,7 @@ GLIBC_2.30 twalk_r F GLIBC_2.31 msgctl F GLIBC_2.31 semctl F GLIBC_2.31 shmctl F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 _Exit F GLIBC_2.4 _IO_2_1_stderr_ D 0xa0 diff --git a/sysdeps/unix/sysv/linux/arm/bits/rseq.h b/sysdeps/unix/sysv/linux/arm/bits/rseq.h new file mode 100644 index 0000000000..c132f0327c --- /dev/null +++ b/sysdeps/unix/sysv/linux/arm/bits/rseq.h @@ -0,0 +1,83 @@ +/* Restartable Sequences Linux arm architecture header. + Copyright (C) 2020 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. + + - ARM little endian + + RSEQ_SIG uses the udf A32 instruction with an uncommon immediate operand + value 0x5de3. This traps if user-space reaches this instruction by mistake, + and the uncommon operand ensures the kernel does not move the instruction + pointer to attacker-controlled code on rseq abort. + + The instruction pattern in the A32 instruction set is: + + e7f5def3 udf #24035 ; 0x5de3 + + This translates to the following instruction pattern in the T16 instruction + set: + + little endian: + def3 udf #243 ; 0xf3 + e7f5 b.n <7f5> + + - ARMv6+ big endian (BE8): + + ARMv6+ -mbig-endian generates mixed endianness code vs data: little-endian + code and big-endian data. The data value of the signature needs to have its + byte order reversed to generate the trap instruction: + + Data: 0xf3def5e7 + + Translates to this A32 instruction pattern: + + e7f5def3 udf #24035 ; 0x5de3 + + Translates to this T16 instruction pattern: + + def3 udf #243 ; 0xf3 + e7f5 b.n <7f5> + + - Prior to ARMv6 big endian (BE32): + + Prior to ARMv6, -mbig-endian generates big-endian code and data + (which match), so the endianness of the data representation of the + signature should not be reversed. However, the choice between BE32 + and BE8 is done by the linker, so we cannot know whether code and + data endianness will be mixed before the linker is invoked. So rather + than try to play tricks with the linker, the rseq signature is simply + data (not a trap instruction) prior to ARMv6 on big endian. This is + why the signature is expressed as data (.word) rather than as + instruction (.inst) in assembler. */ + +#ifdef __ARMEB__ +#define RSEQ_SIG 0xf3def5e7 /* udf #24035 ; 0x5de3 (ARMv6+) */ +#else +#define RSEQ_SIG 0xe7f5def3 /* udf #24035 ; 0x5de3 */ +#endif diff --git a/sysdeps/unix/sysv/linux/arm/le/libc.abilist b/sysdeps/unix/sysv/linux/arm/le/libc.abilist index f1456b26b2..7fa5f13745 100644 --- a/sysdeps/unix/sysv/linux/arm/le/libc.abilist +++ b/sysdeps/unix/sysv/linux/arm/le/libc.abilist @@ -130,6 +130,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 _Exit F GLIBC_2.4 _IO_2_1_stderr_ D 0xa0 diff --git a/sysdeps/unix/sysv/linux/bits/rseq.h b/sysdeps/unix/sysv/linux/bits/rseq.h new file mode 100644 index 0000000000..014c08fe0f --- /dev/null +++ b/sysdeps/unix/sysv/linux/bits/rseq.h @@ -0,0 +1,29 @@ +/* Restartable Sequences architecture header. Stub version. + Copyright (C) 2020 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. */ diff --git a/sysdeps/unix/sysv/linux/csky/libc.abilist b/sysdeps/unix/sysv/linux/csky/libc.abilist index c54aed2f8e..2784c9c2b4 100644 --- a/sysdeps/unix/sysv/linux/csky/libc.abilist +++ b/sysdeps/unix/sysv/linux/csky/libc.abilist @@ -2090,4 +2090,5 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F diff --git a/sysdeps/unix/sysv/linux/hppa/libc.abilist b/sysdeps/unix/sysv/linux/hppa/libc.abilist index 87373f755b..fa57e7b9a1 100644 --- a/sysdeps/unix/sysv/linux/hppa/libc.abilist +++ b/sysdeps/unix/sysv/linux/hppa/libc.abilist @@ -2047,6 +2047,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 __confstr_chk F GLIBC_2.4 __fgets_chk F diff --git a/sysdeps/unix/sysv/linux/i386/libc.abilist b/sysdeps/unix/sysv/linux/i386/libc.abilist index 1bd2e02f79..ddb068d77f 100644 --- a/sysdeps/unix/sysv/linux/i386/libc.abilist +++ b/sysdeps/unix/sysv/linux/i386/libc.abilist @@ -2213,6 +2213,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 __confstr_chk F GLIBC_2.4 __fgets_chk F diff --git a/sysdeps/unix/sysv/linux/ia64/libc.abilist b/sysdeps/unix/sysv/linux/ia64/libc.abilist index 07e51d46bf..20cbe0a67e 100644 --- a/sysdeps/unix/sysv/linux/ia64/libc.abilist +++ b/sysdeps/unix/sysv/linux/ia64/libc.abilist @@ -2079,6 +2079,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 __confstr_chk F GLIBC_2.4 __fgets_chk F diff --git a/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist b/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist index 42ea4c24bf..6d7aff8738 100644 --- a/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist +++ b/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist @@ -134,6 +134,7 @@ GLIBC_2.30 twalk_r F GLIBC_2.31 msgctl F GLIBC_2.31 semctl F GLIBC_2.31 shmctl F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 _Exit F GLIBC_2.4 _IO_2_1_stderr_ D 0x98 diff --git a/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist b/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist index e9358fb092..d935a1220f 100644 --- a/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist +++ b/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist @@ -2159,6 +2159,7 @@ GLIBC_2.30 twalk_r F GLIBC_2.31 msgctl F GLIBC_2.31 semctl F GLIBC_2.31 shmctl F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 __confstr_chk F GLIBC_2.4 __fgets_chk F diff --git a/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist b/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist index 2cefe739c0..8d8ae846d0 100644 --- a/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist +++ b/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist @@ -2141,4 +2141,5 @@ GLIBC_2.30 twalk_r F GLIBC_2.31 msgctl F GLIBC_2.31 semctl F GLIBC_2.31 shmctl F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F diff --git a/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist b/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist index 3474ef1490..6b25430fe6 100644 --- a/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist +++ b/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist @@ -2138,4 +2138,5 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F diff --git a/sysdeps/unix/sysv/linux/mips/bits/rseq.h b/sysdeps/unix/sysv/linux/mips/bits/rseq.h new file mode 100644 index 0000000000..cbad4290cc --- /dev/null +++ b/sysdeps/unix/sysv/linux/mips/bits/rseq.h @@ -0,0 +1,62 @@ +/* Restartable Sequences Linux mips architecture header. + Copyright (C) 2020 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 break instruction. The instruction pattern is: + + On MIPS: + 0350000d break 0x350 + + On nanoMIPS: + 00100350 break 0x350 + + On microMIPS: + 0000d407 break 0x350 + + For nanoMIPS32 and microMIPS, the instruction stream is encoded as + 16-bit halfwords, so the signature halfwords need to be swapped + accordingly for little-endian. */ + +#if defined(__nanomips__) +# ifdef __MIPSEL__ +# define RSEQ_SIG 0x03500010 +# else +# define RSEQ_SIG 0x00100350 +# endif +#elif defined(__mips_micromips) +# ifdef __MIPSEL__ +# define RSEQ_SIG 0xd4070000 +# else +# define RSEQ_SIG 0x0000d407 +# endif +#elif defined(__mips__) +# define RSEQ_SIG 0x0350000d +#else +/* Unknown MIPS architecture. */ +#endif diff --git a/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist index a6f99a7369..d78dd9c881 100644 --- a/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist +++ b/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist @@ -2130,6 +2130,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 __confstr_chk F GLIBC_2.4 __fgets_chk F diff --git a/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist index 48222af11c..3b576e05f0 100644 --- a/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist +++ b/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist @@ -2128,6 +2128,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 __confstr_chk F GLIBC_2.4 __fgets_chk F diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist index 99965cfb0f..ee06d83cbe 100644 --- a/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist +++ b/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist @@ -2136,6 +2136,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 __confstr_chk F GLIBC_2.4 __fgets_chk F diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist index 2c8bafc669..2e8e658422 100644 --- a/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist +++ b/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist @@ -2130,6 +2130,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 __confstr_chk F GLIBC_2.4 __fgets_chk F diff --git a/sysdeps/unix/sysv/linux/nios2/libc.abilist b/sysdeps/unix/sysv/linux/nios2/libc.abilist index 52cf72052c..6b07bd9ef7 100644 --- a/sysdeps/unix/sysv/linux/nios2/libc.abilist +++ b/sysdeps/unix/sysv/linux/nios2/libc.abilist @@ -2179,4 +2179,5 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F diff --git a/sysdeps/unix/sysv/linux/powerpc/bits/rseq.h b/sysdeps/unix/sysv/linux/powerpc/bits/rseq.h new file mode 100644 index 0000000000..0313b9cba9 --- /dev/null +++ b/sysdeps/unix/sysv/linux/powerpc/bits/rseq.h @@ -0,0 +1,37 @@ +/* Restartable Sequences Linux powerpc architecture header. + Copyright (C) 2020 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 trap instruction: + + powerpc-be: 0f e5 00 0b twui r5,11 + powerpc64-le: 0b 00 e5 0f twui r5,11 + powerpc64-be: 0f e5 00 0b twui r5,11 */ + +#define RSEQ_SIG 0x0fe5000b diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist index 2ca5bbccf3..7cdfda4a4e 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist @@ -2186,6 +2186,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 _IO_fprintf F GLIBC_2.4 _IO_printf F diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist index e6c4d002d5..aa85a07020 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist @@ -2219,6 +2219,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 _IO_fprintf F GLIBC_2.4 _IO_printf F diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist index 82d77b7e48..7f63a82038 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist @@ -2049,6 +2049,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 _IO_fprintf F GLIBC_2.4 _IO_printf F diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist index 0c2513a4b3..18a9b1dc1c 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist @@ -2288,6 +2288,7 @@ GLIBC_2.32 __qecvtieee128_r F GLIBC_2.32 __qfcvtieee128 F GLIBC_2.32 __qfcvtieee128_r F GLIBC_2.32 __qgcvtieee128 F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 __scanfieee128 F GLIBC_2.32 __snprintf_chkieee128 F GLIBC_2.32 __snprintfieee128 F diff --git a/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist b/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist index 234d34929a..c0d9de3fe2 100644 --- a/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist +++ b/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist @@ -2108,4 +2108,5 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F diff --git a/sysdeps/unix/sysv/linux/rseq-internal.h b/sysdeps/unix/sysv/linux/rseq-internal.h new file mode 100644 index 0000000000..6583691285 --- /dev/null +++ b/sysdeps/unix/sysv/linux/rseq-internal.h @@ -0,0 +1,73 @@ +/* Restartable Sequences internal API. Linux implementation. + Copyright (C) 2020 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 RSEQ_INTERNAL_H +#define RSEQ_INTERNAL_H + +#include <sysdep.h> +#include <errno.h> +#include <kernel-features.h> +#include <stdio.h> +#include <sys/rseq.h> + +#ifdef RSEQ_SIG +static inline void +rseq_register_current_thread (void) +{ + int ret; + + if (__rseq_abi.cpu_id != RSEQ_CPU_ID_UNINITIALIZED) + __libc_fatal ("glibc fatal error: " + "rseq already initialized for this thread\n"); + ret = INTERNAL_SYSCALL_CALL (rseq, &__rseq_abi, sizeof (struct rseq), + 0, RSEQ_SIG); + if (INTERNAL_SYSCALL_ERROR_P (ret)) + { + const char *msg = NULL; + + switch (INTERNAL_SYSCALL_ERRNO (ret)) + { + case ENOSYS: /* rseq system call not implemented. */ + case EPERM: /* rseq system call filtered by seccomp. */ + case EACCES: /* rseq system call filtered by seccomp. */ + __rseq_abi.cpu_id = RSEQ_CPU_ID_REGISTRATION_FAILED; + break; + case EBUSY: + msg = "glibc fatal error: rseq already registered for this thread\n"; + break; + case EFAULT: + msg = "glibc fatal error: rseq parameter is an invalid address\n"; + break; + case EINVAL: + msg = "glibc fatal error: rseq parameters are invalid\n"; + break; + default: + msg = "glibc fatal error: unexpected rseq errno\n"; + break; + } + if (msg) + __libc_fatal (msg); + } +} +#else +static inline void +rseq_register_current_thread (void) +{ +} +#endif + +#endif /* rseq-internal.h */ diff --git a/sysdeps/unix/sysv/linux/rseq-sym.c b/sysdeps/unix/sysv/linux/rseq-sym.c new file mode 100644 index 0000000000..090093408f --- /dev/null +++ b/sysdeps/unix/sysv/linux/rseq-sym.c @@ -0,0 +1,26 @@ +/* Restartable Sequences exported symbols. Linux Implementation. + Copyright (C) 2020 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/>. */ + +#include <sys/syscall.h> +#include <stdint.h> +#include <kernel-features.h> +#include <sys/rseq.h> + +__thread struct rseq __rseq_abi = + { + .cpu_id = RSEQ_CPU_ID_UNINITIALIZED, + }; diff --git a/sysdeps/unix/sysv/linux/s390/bits/rseq.h b/sysdeps/unix/sysv/linux/s390/bits/rseq.h new file mode 100644 index 0000000000..ef0cedaac3 --- /dev/null +++ b/sysdeps/unix/sysv/linux/s390/bits/rseq.h @@ -0,0 +1,37 @@ +/* Restartable Sequences Linux s390 architecture header. + Copyright (C) 2020 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 trap4 instruction. As Linux does not make use of the + access-register mode nor the linkage stack this instruction will always + cause a special-operation exception (the trap-enabled bit in the DUCT + is and will stay 0). The instruction pattern is + b2 ff 0f ff trap4 4095(%r0) */ + +#define RSEQ_SIG 0xB2FF0FFF diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist b/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist index 1f06cce028..1699bcbf6d 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist +++ b/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist @@ -2184,6 +2184,7 @@ GLIBC_2.30 twalk_r F GLIBC_2.31 msgctl F GLIBC_2.31 semctl F GLIBC_2.31 shmctl F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 _IO_fprintf F GLIBC_2.4 _IO_printf F diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist b/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist index 26c2ce32e5..cc7c04c1d0 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist +++ b/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist @@ -2085,6 +2085,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 _IO_fprintf F GLIBC_2.4 _IO_printf F diff --git a/sysdeps/unix/sysv/linux/sh/be/libc.abilist b/sysdeps/unix/sysv/linux/sh/be/libc.abilist index 7ad2e920c3..0641e0fca0 100644 --- a/sysdeps/unix/sysv/linux/sh/be/libc.abilist +++ b/sysdeps/unix/sysv/linux/sh/be/libc.abilist @@ -2054,6 +2054,7 @@ GLIBC_2.30 twalk_r F GLIBC_2.31 msgctl F GLIBC_2.31 semctl F GLIBC_2.31 shmctl F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 __confstr_chk F GLIBC_2.4 __fgets_chk F diff --git a/sysdeps/unix/sysv/linux/sh/le/libc.abilist b/sysdeps/unix/sysv/linux/sh/le/libc.abilist index d2611bf0a5..f06d7066b8 100644 --- a/sysdeps/unix/sysv/linux/sh/le/libc.abilist +++ b/sysdeps/unix/sysv/linux/sh/le/libc.abilist @@ -2051,6 +2051,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 __confstr_chk F GLIBC_2.4 __fgets_chk F diff --git a/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist b/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist index 18a528f0e9..ef5a0b688e 100644 --- a/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist +++ b/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist @@ -2175,6 +2175,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 _IO_fprintf F GLIBC_2.4 _IO_printf F diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist b/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist index a1d48b0f3c..096bb62978 100644 --- a/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist +++ b/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist @@ -2102,6 +2102,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 __confstr_chk F GLIBC_2.4 __fgets_chk F diff --git a/sysdeps/unix/sysv/linux/sys/rseq.h b/sysdeps/unix/sysv/linux/sys/rseq.h new file mode 100644 index 0000000000..ea51194bf8 --- /dev/null +++ b/sysdeps/unix/sysv/linux/sys/rseq.h @@ -0,0 +1,207 @@ +/* Restartable Sequences exported symbols. Linux header. + Copyright (C) 2020 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 +#define _SYS_RSEQ_H 1 + +/* Architecture-specific rseq signature. */ +#include <bits/rseq.h> + +#include <stdint.h> +#include <sys/cdefs.h> + +#ifdef __has_include +# if __has_include ("linux/rseq.h") +# define __GLIBC_HAVE_KERNEL_RSEQ +# endif +#else +# include <linux/version.h> +# if LINUX_VERSION_CODE >= KERNEL_VERSION (4, 18, 0) +# define __GLIBC_HAVE_KERNEL_RSEQ +# endif +#endif + +#ifdef __GLIBC_HAVE_KERNEL_RSEQ +/* We use the structures declarations from the kernel headers. */ +# include <linux/rseq.h> +#else +/* We use a copy of the include/uapi/linux/rseq.h kernel header. */ + +# include <asm/byteorder.h> + +enum rseq_cpu_id_state + { + RSEQ_CPU_ID_UNINITIALIZED = -1, + RSEQ_CPU_ID_REGISTRATION_FAILED = -2, + }; + +enum rseq_flags + { + RSEQ_FLAG_UNREGISTER = (1 << 0), + }; + +enum rseq_cs_flags_bit + { + RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT_BIT = 0, + RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL_BIT = 1, + RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE_BIT = 2, + }; + +enum rseq_cs_flags + { + RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT = + (1U << RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT_BIT), + RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL = + (1U << RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL_BIT), + RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE = + (1U << RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE_BIT), + }; + +/* struct rseq_cs is aligned on 32 bytes to ensure it is always + contained within a single cache-line. It is usually declared as + link-time constant data. */ +struct rseq_cs + { + /* Version of this structure. */ + uint32_t version; + /* enum rseq_cs_flags. */ + uint32_t flags; + uint64_t start_ip; + /* Offset from start_ip. */ + uint64_t post_commit_offset; + uint64_t abort_ip; + } __attribute__ ((aligned (32))); + +/* struct rseq is aligned on 32 bytes to ensure it is always + contained within a single cache-line. + + A single struct rseq per thread is allowed. */ +struct rseq + { + /* Restartable sequences cpu_id_start field. Updated by the + kernel. Read by user-space with single-copy atomicity + semantics. This field should only be read by the thread which + registered this data structure. Aligned on 32-bit. Always + contains a value in the range of possible CPUs, although the + value may not be the actual current CPU (e.g. if rseq is not + initialized). This CPU number value should always be compared + against the value of the cpu_id field before performing a rseq + commit or returning a value read from a data structure indexed + using the cpu_id_start value. */ + uint32_t cpu_id_start; + /* Restartable sequences cpu_id field. Updated by the kernel. + Read by user-space with single-copy atomicity semantics. This + field should only be read by the thread which registered this + data structure. Aligned on 32-bit. Values + RSEQ_CPU_ID_UNINITIALIZED and RSEQ_CPU_ID_REGISTRATION_FAILED + have a special semantic: the former means "rseq uninitialized", + and latter means "rseq initialization failed". This value is + meant to be read within rseq critical sections and compared + with the cpu_id_start value previously read, before performing + the commit instruction, or read and compared with the + cpu_id_start value before returning a value loaded from a data + structure indexed using the cpu_id_start value. */ + uint32_t cpu_id; + /* Restartable sequences rseq_cs field. + + Contains NULL when no critical section is active for the current + thread, or holds a pointer to the currently active struct rseq_cs. + + Updated by user-space, which sets the address of the currently + active rseq_cs at the beginning of assembly instruction sequence + block, and set to NULL by the kernel when it restarts an assembly + instruction sequence block, as well as when the kernel detects that + it is preempting or delivering a signal outside of the range + targeted by the rseq_cs. Also needs to be set to NULL by user-space + before reclaiming memory that contains the targeted struct rseq_cs. + + Read and set by the kernel. Set by user-space with single-copy + atomicity semantics. This field should only be updated by the + thread which registered this data structure. Aligned on 64-bit. + + User-space may perform the update through the rseq_cs.uptr.ptr + field. The padding needs to be initialized to zero on 32-bit. */ + union + { + uint64_t ptr64; +# ifdef __LP64__ + uint64_t ptr; +# else + struct + { +# if (defined (__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || defined (__BIG_ENDIAN) + uint32_t padding; /* Initialized to zero. */ + uint32_t ptr32; +# else /* LITTLE */ + uint32_t ptr32; + uint32_t padding; /* Initialized to zero. */ +# endif /* ENDIAN */ + } ptr; +# endif + +# ifndef __KERNEL__ + struct + { +# ifdef __LP64__ + const struct rseq_cs *ptr; +# else +# if (defined (__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || defined (__BIG_ENDIAN) + uint32_t padding; /* Initialized to zero. */ + const struct rseq_cs *ptr; +# else /* LITTLE */ + const struct rseq_cs *ptr; + uint32_t padding; /* Initialized to zero. */ +# endif /* ENDIAN */ +# endif + } uptr; +# endif + } rseq_cs; + + /* Restartable sequences flags field. + + This field should only be updated by the thread which + registered this data structure. Read by the kernel. + Mainly used for single-stepping through rseq critical sections + with debuggers. + + - RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT + Inhibit instruction sequence block restart on preemption + for this thread. + - RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL + Inhibit instruction sequence block restart on signal + delivery for this thread. + - RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE + Inhibit instruction sequence block restart on migration for + this thread. */ + uint32_t flags; + } __attribute__ ((aligned (32))); + +#endif + +/* Ensure the compiler supports __attribute__ ((aligned)). */ +_Static_assert (__alignof__ (struct rseq_cs) >= 32, "alignment"); +_Static_assert (__alignof__ (struct rseq) >= 32, "alignment"); + +/* Allocations of struct rseq and struct rseq_cs on the heap need to + be aligned on 32 bytes. Therefore, use of malloc is discouraged + because it does not guarantee alignment. posix_memalign should be + used instead. */ + +extern __thread struct rseq __rseq_abi + __attribute__ ((tls_model ("initial-exec"))); + +#endif /* sys/rseq.h */ diff --git a/sysdeps/unix/sysv/linux/x86/bits/rseq.h b/sysdeps/unix/sysv/linux/x86/bits/rseq.h new file mode 100644 index 0000000000..33a6aa7f47 --- /dev/null +++ b/sysdeps/unix/sysv/linux/x86/bits/rseq.h @@ -0,0 +1,30 @@ +/* Restartable Sequences Linux x86 architecture header. + Copyright (C) 2020 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. + + RSEQ_SIG is used with the following reserved undefined instructions, which + trap in user-space: + + x86-32: 0f b9 3d 53 30 05 53 ud1 0x53053053,%edi + x86-64: 0f b9 3d 53 30 05 53 ud1 0x53053053(%rip),%edi */ + +#define RSEQ_SIG 0x53053053 diff --git a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist index 6418ace78a..f78e9f894e 100644 --- a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist +++ b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist @@ -2060,6 +2060,7 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F GLIBC_2.4 __confstr_chk F GLIBC_2.4 __fgets_chk F diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist index edb9f2f004..2867b0adb2 100644 --- a/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist +++ b/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist @@ -2159,4 +2159,5 @@ GLIBC_2.30 getdents64 F GLIBC_2.30 gettid F GLIBC_2.30 tgkill F GLIBC_2.30 twalk_r F +GLIBC_2.32 __rseq_abi T 0x20 GLIBC_2.32 pthread_sigmask F -- 2.17.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) 2020-05-01 2:14 ` [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) Mathieu Desnoyers via Libc-alpha @ 2020-05-20 11:40 ` Florian Weimer via Libc-alpha 2020-05-25 14:51 ` Mathieu Desnoyers via Libc-alpha 0 siblings, 1 reply; 27+ messages in thread From: Florian Weimer via Libc-alpha @ 2020-05-20 11:40 UTC (permalink / raw) To: Mathieu Desnoyers via Libc-alpha Cc: Rich Felker, Peter Zijlstra, linux-api, Boqun Feng, Will Deacon, linux-kernel, Ben Maurer, Mathieu Desnoyers, Dave Watson, Thomas Gleixner, Paul E. McKenney, Paul Turner, Joseph Myers * Mathieu Desnoyers via Libc-alpha: > diff --git a/NEWS b/NEWS > index 141078c319..c4e0370fc4 100644 > --- a/NEWS > +++ b/NEWS > @@ -23,6 +23,16 @@ Major new features: > toolchains. It is recommended to use GCC 8 or newer when testing > this option. > > +* Support for automatically registering threads with the Linux rseq > + system call has been added. This system call is implemented starting > + from Linux 4.18. The Restartable Sequences ABI accelerates user-space > + operations on per-cpu data. It allows user-space to perform updates > + on per-cpu data without requiring heavy-weight atomic operations. > + Automatically registering threads allows all libraries, including libc, > + to make immediate use of the rseq(2) support by using the documented ABI. > + The GNU C Library manual has details on integration of Restartable > + Sequences. “rseq” instead “rseq(2)”. > diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c > index e6c64fb526..f0fcf6448e 100644 > --- a/elf/libc_early_init.c > +++ b/elf/libc_early_init.c > @@ -18,10 +18,14 @@ > > #include <ctype.h> > #include <libc-early-init.h> > +#include <rseq-internal.h> > > void > __libc_early_init (_Bool initial) > { > /* Initialize ctype data. */ > __ctype_init (); > + /* Register rseq ABI to the kernel for the main program's libc. */ > + if (initial) > + rseq_register_current_thread (); > } Okay. > diff --git a/manual/threads.texi b/manual/threads.texi > index 0858ef8f92..a565095c43 100644 > --- a/manual/threads.texi > +++ b/manual/threads.texi > @@ -9,8 +9,10 @@ This chapter describes functions used for managing threads. > POSIX threads. > > @menu > -* ISO C Threads:: Threads based on the ISO C specification. > -* POSIX Threads:: Threads based on the POSIX specification. > +* ISO C Threads:: Threads based on the ISO C specification. > +* POSIX Threads:: Threads based on the POSIX specification. > +* Restartable Sequences:: Linux-specific Restartable Sequences > + integration. > @end menu This should go into the extensions menu (@node Non-POSIX Extensions). General comment: Please wrap the lines around 72 or so characters. Thanks. > @@ -881,3 +883,63 @@ Behaves like @code{pthread_timedjoin_np} except that the absolute time in > @c pthread_spin_unlock > @c pthread_testcancel > @c pthread_yield > + > +@node Restartable Sequences > +@section Restartable Sequences > +@cindex Restartable Sequences > + > +This section describes Restartable Sequences integration for > +@theglibc{}. “This functionality is only available on Linux.” (The @standards parts are not visible to readers.) > + > +@deftypevar {struct rseq} __rseq_abi > +@standards{Linux, sys/rseq.h} > +@Theglibc{} implements a @code{__rseq_abi} TLS symbol to interact with the > +Restartable Sequences system call (Linux-specific). The layout of this > +structure is defined by the @file{sys/rseq.h} header. Registration of each > +thread's @code{__rseq_abi} is performed by @theglibc{} at library > +initialization and thread creation. Can drop “(Linux-specific)” here. > diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/rseq.h b/sysdeps/unix/sysv/linux/aarch64/bits/rseq.h > new file mode 100644 > index 0000000000..37d83fcb4a > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/aarch64/bits/rseq.h > +#ifdef __AARCH64EB__ > +#define RSEQ_SIG_DATA 0x00bc28d4 /* BRK #0x45E0. */ > +#else > +#define RSEQ_SIG_DATA RSEQ_SIG_CODE > +#endif Missing indentation on the #defines (sorry!). > diff --git a/sysdeps/unix/sysv/linux/arm/bits/rseq.h b/sysdeps/unix/sysv/linux/arm/bits/rseq.h > new file mode 100644 > index 0000000000..c132f0327c > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/arm/bits/rseq.h > +#ifdef __ARMEB__ > +#define RSEQ_SIG 0xf3def5e7 /* udf #24035 ; 0x5de3 (ARMv6+) */ > +#else > +#define RSEQ_SIG 0xe7f5def3 /* udf #24035 ; 0x5de3 */ > +#endif Likewise, missing indentation. > diff --git a/sysdeps/unix/sysv/linux/bits/rseq.h b/sysdeps/unix/sysv/linux/bits/rseq.h > new file mode 100644 > index 0000000000..014c08fe0f > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/bits/rseq.h > @@ -0,0 +1,29 @@ > +/* 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. */ I wonder if we should say something somewhere that the correct way to check for compile-time rseq support in glibc is something like this? #if __has_include (<sys/rseq.h>) # include <sys/rseq.h> #endif #ifdef RSEQ_SIG … #endif Or perhaps we should suppress installation of the headers if we only have support for the stub. (I think this fine tuning can be deferred to later patch.) > diff --git a/sysdeps/unix/sysv/linux/mips/bits/rseq.h b/sysdeps/unix/sysv/linux/mips/bits/rseq.h > new file mode 100644 > index 0000000000..cbad4290cc > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/mips/bits/rseq.h > @@ -0,0 +1,62 @@ > +#if defined(__nanomips__) > +# ifdef __MIPSEL__ > +# define RSEQ_SIG 0x03500010 > +# else > +# define RSEQ_SIG 0x00100350 > +# endif > +#elif defined(__mips_micromips) > +# ifdef __MIPSEL__ > +# define RSEQ_SIG 0xd4070000 > +# else > +# define RSEQ_SIG 0x0000d407 > +# endif > +#elif defined(__mips__) > +# define RSEQ_SIG 0x0350000d > +#else > +/* Unknown MIPS architecture. */ > +#endif Please use “defined (”, with a space. > diff --git a/sysdeps/unix/sysv/linux/rseq-internal.h b/sysdeps/unix/sysv/linux/rseq-internal.h > new file mode 100644 > index 0000000000..6583691285 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/rseq-internal.h > +#ifdef RSEQ_SIG > +static inline void > +rseq_register_current_thread (void) > + if (msg) > + __libc_fatal (msg); > + } “if (msg != NULL)”, please. > +#else > +static inline void > +rseq_register_current_thread (void) > +{ > +} > +#endif Maybe add /* RSEQ_SIG */ comments to #else/#endif here as well. > diff --git a/sysdeps/unix/sysv/linux/sys/rseq.h b/sysdeps/unix/sysv/linux/sys/rseq.h > new file mode 100644 > index 0000000000..ea51194bf8 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/sys/rseq.h > +#ifdef __has_include > +# if __has_include ("linux/rseq.h") > +# define __GLIBC_HAVE_KERNEL_RSEQ > +# endif > +#else > +# include <linux/version.h> > +# if LINUX_VERSION_CODE >= KERNEL_VERSION (4, 18, 0) > +# define __GLIBC_HAVE_KERNEL_RSEQ > +# endif > +#endif Too much indentation on the define line, I think. Still missing: #ifdef __ASSEMBLER__. .S files should be able to include <sys/rseq.h> to get the definition of RSEQ_SIG. But I think this can be deferred to a follow-up. > +#ifdef __GLIBC_HAVE_KERNEL_RSEQ > +/* We use the structures declarations from the kernel headers. */ > +# include <linux/rseq.h> > +#else > +/* We use a copy of the include/uapi/linux/rseq.h kernel header. */ This comment is not true, the kernel headers do not have uptr support. If we revert the uptr change, we also need to update the manual, I think. > +/* Ensure the compiler supports __attribute__ ((aligned)). */ > +_Static_assert (__alignof__ (struct rseq_cs) >= 32, "alignment"); > +_Static_assert (__alignof__ (struct rseq) >= 32, "alignment"); This needs #ifndef __cplusplus or something like that. I'm surprised that this passes the installed header tests. > +/* Allocations of struct rseq and struct rseq_cs on the heap need to > + be aligned on 32 bytes. Therefore, use of malloc is discouraged > + because it does not guarantee alignment. posix_memalign should be > + used instead. */ > + > +extern __thread struct rseq __rseq_abi > + __attribute__ ((tls_model ("initial-exec"))); Should be __tls_model__. We're getting really close now. 8-) Thanks, Florian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) 2020-05-20 11:40 ` Florian Weimer via Libc-alpha @ 2020-05-25 14:51 ` Mathieu Desnoyers via Libc-alpha 2020-05-25 15:20 ` Florian Weimer via Libc-alpha 0 siblings, 1 reply; 27+ messages in thread From: Mathieu Desnoyers via Libc-alpha @ 2020-05-25 14:51 UTC (permalink / raw) To: Florian Weimer Cc: Rich Felker, libc-alpha, Peter Zijlstra, linux-api, Boqun Feng, Will Deacon, linux-kernel, Ben Maurer, Dave Watson, Thomas Gleixner, Paul, Paul Turner, Joseph Myers ----- On May 20, 2020, at 7:40 AM, Florian Weimer fweimer@redhat.com wrote: > * Mathieu Desnoyers via Libc-alpha: > >> diff --git a/NEWS b/NEWS >> index 141078c319..c4e0370fc4 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -23,6 +23,16 @@ Major new features: >> toolchains. It is recommended to use GCC 8 or newer when testing >> this option. >> >> +* Support for automatically registering threads with the Linux rseq >> + system call has been added. This system call is implemented starting >> + from Linux 4.18. The Restartable Sequences ABI accelerates user-space >> + operations on per-cpu data. It allows user-space to perform updates >> + on per-cpu data without requiring heavy-weight atomic operations. >> + Automatically registering threads allows all libraries, including libc, >> + to make immediate use of the rseq(2) support by using the documented ABI. >> + The GNU C Library manual has details on integration of Restartable >> + Sequences. > > “rseq” instead “rseq(2)”. OK [...] > >> diff --git a/manual/threads.texi b/manual/threads.texi >> index 0858ef8f92..a565095c43 100644 >> --- a/manual/threads.texi >> +++ b/manual/threads.texi >> @@ -9,8 +9,10 @@ This chapter describes functions used for managing threads. >> POSIX threads. >> >> @menu >> -* ISO C Threads:: Threads based on the ISO C specification. >> -* POSIX Threads:: Threads based on the POSIX specification. >> +* ISO C Threads:: Threads based on the ISO C specification. >> +* POSIX Threads:: Threads based on the POSIX specification. >> +* Restartable Sequences:: Linux-specific Restartable Sequences >> + integration. >> @end menu > > This should go into the extensions menu (@node Non-POSIX Extensions). OK > > General comment: Please wrap the lines around 72 or so characters. > Thanks. OK > >> @@ -881,3 +883,63 @@ Behaves like @code{pthread_timedjoin_np} except that the >> absolute time in >> @c pthread_spin_unlock >> @c pthread_testcancel >> @c pthread_yield >> + >> +@node Restartable Sequences >> +@section Restartable Sequences >> +@cindex Restartable Sequences >> + >> +This section describes Restartable Sequences integration for >> +@theglibc{}. > > “This functionality is only available on Linux.” (The @standards parts > are not visible to readers.) OK > >> + >> +@deftypevar {struct rseq} __rseq_abi >> +@standards{Linux, sys/rseq.h} >> +@Theglibc{} implements a @code{__rseq_abi} TLS symbol to interact with the >> +Restartable Sequences system call (Linux-specific). The layout of this >> +structure is defined by the @file{sys/rseq.h} header. Registration of each >> +thread's @code{__rseq_abi} is performed by @theglibc{} at library >> +initialization and thread creation. > > Can drop “(Linux-specific)” here. OK > >> diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/rseq.h >> b/sysdeps/unix/sysv/linux/aarch64/bits/rseq.h >> new file mode 100644 >> index 0000000000..37d83fcb4a >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/aarch64/bits/rseq.h > >> +#ifdef __AARCH64EB__ >> +#define RSEQ_SIG_DATA 0x00bc28d4 /* BRK #0x45E0. */ >> +#else >> +#define RSEQ_SIG_DATA RSEQ_SIG_CODE >> +#endif > > Missing indentation on the #defines (sorry!). Sorry! My bad! > >> diff --git a/sysdeps/unix/sysv/linux/arm/bits/rseq.h >> b/sysdeps/unix/sysv/linux/arm/bits/rseq.h >> new file mode 100644 >> index 0000000000..c132f0327c >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/arm/bits/rseq.h > >> +#ifdef __ARMEB__ >> +#define RSEQ_SIG 0xf3def5e7 /* udf #24035 ; 0x5de3 (ARMv6+) */ >> +#else >> +#define RSEQ_SIG 0xe7f5def3 /* udf #24035 ; 0x5de3 */ >> +#endif > > Likewise, missing indentation. At least I was consistently wrong. ;-) > >> diff --git a/sysdeps/unix/sysv/linux/bits/rseq.h >> b/sysdeps/unix/sysv/linux/bits/rseq.h >> new file mode 100644 >> index 0000000000..014c08fe0f >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/bits/rseq.h >> @@ -0,0 +1,29 @@ > >> +/* 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. */ > > I wonder if we should say something somewhere that the correct way to > check for compile-time rseq support in glibc is something like this? > > #if __has_include (<sys/rseq.h>) > # include <sys/rseq.h> > #endif > #ifdef RSEQ_SIG > … > #endif > > Or perhaps we should suppress installation of the headers if we only > have support for the stub. > > (I think this fine tuning can be deferred to later patch.) I would prefer we keep this for a later patch. That being said, I notice that gcc prior to 4.9 has trouble compiling __has_include. Therefore, I would tend to recommend the following as a backward-compatible way of using the rseq header from external projects: - At configure time, check for availability of sys/rseq.h to ensure the glibc version is recent enough. In the autotools world, this is usually done with a small test program which will fail to build if the header is not there. - At preprocessing time, include <sys/rseq.h> and check whether RSEQ_SIG is defined. If a project only aims at building with gcc 4.9 or newer, then it can use __has_include. > >> diff --git a/sysdeps/unix/sysv/linux/mips/bits/rseq.h >> b/sysdeps/unix/sysv/linux/mips/bits/rseq.h >> new file mode 100644 >> index 0000000000..cbad4290cc >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/mips/bits/rseq.h >> @@ -0,0 +1,62 @@ > >> +#if defined(__nanomips__) >> +# ifdef __MIPSEL__ >> +# define RSEQ_SIG 0x03500010 >> +# else >> +# define RSEQ_SIG 0x00100350 >> +# endif >> +#elif defined(__mips_micromips) >> +# ifdef __MIPSEL__ >> +# define RSEQ_SIG 0xd4070000 >> +# else >> +# define RSEQ_SIG 0x0000d407 >> +# endif >> +#elif defined(__mips__) >> +# define RSEQ_SIG 0x0350000d >> +#else >> +/* Unknown MIPS architecture. */ >> +#endif > > Please use “defined (”, with a space. OK > >> diff --git a/sysdeps/unix/sysv/linux/rseq-internal.h >> b/sysdeps/unix/sysv/linux/rseq-internal.h >> new file mode 100644 >> index 0000000000..6583691285 >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/rseq-internal.h > >> +#ifdef RSEQ_SIG >> +static inline void >> +rseq_register_current_thread (void) > >> + if (msg) >> + __libc_fatal (msg); >> + } > > “if (msg != NULL)”, please. OK > >> +#else >> +static inline void >> +rseq_register_current_thread (void) >> +{ >> +} >> +#endif > > Maybe add /* RSEQ_SIG */ comments to #else/#endif here as well. OK > >> diff --git a/sysdeps/unix/sysv/linux/sys/rseq.h >> b/sysdeps/unix/sysv/linux/sys/rseq.h >> new file mode 100644 >> index 0000000000..ea51194bf8 >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/sys/rseq.h > >> +#ifdef __has_include >> +# if __has_include ("linux/rseq.h") >> +# define __GLIBC_HAVE_KERNEL_RSEQ >> +# endif >> +#else >> +# include <linux/version.h> >> +# if LINUX_VERSION_CODE >= KERNEL_VERSION (4, 18, 0) >> +# define __GLIBC_HAVE_KERNEL_RSEQ >> +# endif >> +#endif > > Too much indentation on the define line, I think. OK > > Still missing: #ifdef __ASSEMBLER__. .S files should be able to include > <sys/rseq.h> to get the definition of RSEQ_SIG. But I think this can be > deferred to a follow-up. Would the plan be to only do the #include <bits/rseq.h> bits in #ifdef __ASSEMBLER__ and skip all the rest ? > >> +#ifdef __GLIBC_HAVE_KERNEL_RSEQ >> +/* We use the structures declarations from the kernel headers. */ >> +# include <linux/rseq.h> >> +#else >> +/* We use a copy of the include/uapi/linux/rseq.h kernel header. */ > > This comment is not true, the kernel headers do not have uptr support. > > If we revert the uptr change, we also need to update the manual, I > think. Ah, I get to your uptr concern now! Good :) The larger question here is: considering that we re-implement the entire uapi header within glibc (which includes the uptr addition), do we still care about using the header provided by the Linux kernel ? Having different definitions depending on whether a kernel header is installed or not when including a glibc header seems rather unexpected. *If* we want to use the uapi header, I think something is semantically missing. Here is the scheme I envision. We could rely on the kernel header version.h to figure out which of glibc or kernel uapi header is more recent. Any new concept we try to integrate into glibc (e.g. uptr) should go into the upstream Linux uapi header first. For the coming glibc e.g. 2.32, we use the kernel uapi header if kernel version is >= 4.18.0. Within glibc, the fallback implements exactly the API exposed by the kernel rseq.h header. As we eventually introduce the uptr change into the Linux kernel, and say it gets merged for Linux 5.9.0, we mirror this change into glibc (e.g. release 2.33), and bump the Linux kernel version cutoff to 5.9.0. So starting from that version, we use the Linux kernel header only if version >= 5.9.0, else we fallback on glibc's own implementation. This is clearly something we need to get right. > >> +/* Ensure the compiler supports __attribute__ ((aligned)). */ >> +_Static_assert (__alignof__ (struct rseq_cs) >= 32, "alignment"); >> +_Static_assert (__alignof__ (struct rseq) >= 32, "alignment"); > > This needs #ifndef __cplusplus or something like that. I'm surprised > that this passes the installed header tests. Would the following be ok ? #ifdef __cplusplus #define rseq_static_assert static_assert #else #define rseq_static_assert _Static_assert #endif /* Ensure the compiler supports __attribute__ ((aligned)). */ rseq_static_assert (__alignof__ (struct rseq_cs) >= 32, "alignment"); rseq_static_assert (__alignof__ (struct rseq) >= 32, "alignment"); > >> +/* Allocations of struct rseq and struct rseq_cs on the heap need to >> + be aligned on 32 bytes. Therefore, use of malloc is discouraged >> + because it does not guarantee alignment. posix_memalign should be >> + used instead. */ >> + >> +extern __thread struct rseq __rseq_abi >> + __attribute__ ((tls_model ("initial-exec"))); > > Should be __tls_model__. OK > > We're getting really close now. 8-) Great! Thanks for the feedback! Mathieu > > Thanks, > Florian -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) 2020-05-25 14:51 ` Mathieu Desnoyers via Libc-alpha @ 2020-05-25 15:20 ` Florian Weimer via Libc-alpha 2020-05-25 17:36 ` Mathieu Desnoyers via Libc-alpha 0 siblings, 1 reply; 27+ messages in thread From: Florian Weimer via Libc-alpha @ 2020-05-25 15:20 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Rich Felker, libc-alpha, Peter Zijlstra, linux-api, Boqun Feng, Will Deacon, linux-kernel, Ben Maurer, Dave Watson, Thomas Gleixner, Paul, Paul Turner, Joseph Myers * Mathieu Desnoyers: > The larger question here is: considering that we re-implement the entire > uapi header within glibc (which includes the uptr addition), do we still > care about using the header provided by the Linux kernel ? We don't care, but our users do. Eventually, they want to include <sys/rseq.h> and <linux/rseq.h> to get new constants that are not yet known to glibc. > Having different definitions depending on whether a kernel header is > installed or not when including a glibc header seems rather unexpected. Indeed. > *If* we want to use the uapi header, I think something is semantically > missing. Here is the scheme I envision. We could rely on the kernel header > version.h to figure out which of glibc or kernel uapi header is more > recent. Any new concept we try to integrate into glibc (e.g. uptr) > should go into the upstream Linux uapi header first. I think we should always prefer the uapi header. The Linux version check does not tell you anything about backports. > For the coming glibc e.g. 2.32, we use the kernel uapi header if > kernel version is >= 4.18.0. Within glibc, the fallback implements > exactly the API exposed by the kernel rseq.h header. Agreed. > As we eventually introduce the uptr change into the Linux kernel, and > say it gets merged for Linux 5.9.0, we mirror this change into glibc > (e.g. release 2.33), and bump the Linux kernel version cutoff to 5.9.0. > So starting from that version, we use the Linux kernel header only if > version >= 5.9.0, else we fallback on glibc's own implementation. Fortunately, we don't need to settle this today. 8-) Let's stick to the 4.18 definitions for the fallback for now, and discuss the incorporation of future changes later. >>> +/* Ensure the compiler supports __attribute__ ((aligned)). */ >>> +_Static_assert (__alignof__ (struct rseq_cs) >= 32, "alignment"); >>> +_Static_assert (__alignof__ (struct rseq) >= 32, "alignment"); >> >> This needs #ifndef __cplusplus or something like that. I'm surprised >> that this passes the installed header tests. > > Would the following be ok ? > > #ifdef __cplusplus > #define rseq_static_assert static_assert > #else > #define rseq_static_assert _Static_assert > #endif > > /* Ensure the compiler supports __attribute__ ((aligned)). */ > rseq_static_assert (__alignof__ (struct rseq_cs) >= 32, "alignment"); > rseq_static_assert (__alignof__ (struct rseq) >= 32, "alignment"); Seems reasonable, yes. __alignof__ is still a GCC extension. C++11 has alignof, C11 has _Alignof. So you could use something like this (perhaps without indentation for the kernel header version): #ifdef __cplusplus # if __cplusplus >= 201103L # define rseq_static_assert(x) static_assert x; # define rseq_alignof alignof # endif #elif __STDC_VERSION__ >= 201112L # define rseq_static_assert(x) _Static_assert x; # define rseq_alignof _Alignof #endif #ifndef rseq_static_assert # define rseq_static_assert /* nothing */ #endif rseq_static_assert ((rseq_alignof__ (struct rseq_cs) >= 32, "alignment")) rseq_static_assert ((rseq_alignof (struct rseq) >= 32, "alignment")) And something similar for _Alignas/attribute aligned, with an error for older standards and !__GNUC__ compilers (because neither the type nor __thread can be represented there). Thanks, Florian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) 2020-05-25 15:20 ` Florian Weimer via Libc-alpha @ 2020-05-25 17:36 ` Mathieu Desnoyers via Libc-alpha 2020-05-26 12:41 ` Florian Weimer via Libc-alpha 0 siblings, 1 reply; 27+ messages in thread From: Mathieu Desnoyers via Libc-alpha @ 2020-05-25 17:36 UTC (permalink / raw) To: Florian Weimer Cc: Rich Felker, libc-alpha, Peter Zijlstra, linux-api, Boqun Feng, Will Deacon, linux-kernel, Ben Maurer, Dave Watson, Thomas Gleixner, Paul, Paul Turner, Joseph Myers ----- On May 25, 2020, at 11:20 AM, Florian Weimer fweimer@redhat.com wrote: > * Mathieu Desnoyers: > >> The larger question here is: considering that we re-implement the entire >> uapi header within glibc (which includes the uptr addition), do we still >> care about using the header provided by the Linux kernel ? > > We don't care, but our users do. Eventually, they want to include > <sys/rseq.h> and <linux/rseq.h> to get new constants that are not yet > known to glibc. Good point! > >> Having different definitions depending on whether a kernel header is >> installed or not when including a glibc header seems rather unexpected. > > Indeed. > >> *If* we want to use the uapi header, I think something is semantically >> missing. Here is the scheme I envision. We could rely on the kernel header >> version.h to figure out which of glibc or kernel uapi header is more >> recent. Any new concept we try to integrate into glibc (e.g. uptr) >> should go into the upstream Linux uapi header first. > > I think we should always prefer the uapi header. The Linux version > check does not tell you anything about backports. Fair enough. > >> For the coming glibc e.g. 2.32, we use the kernel uapi header if >> kernel version is >= 4.18.0. Within glibc, the fallback implements >> exactly the API exposed by the kernel rseq.h header. > > Agreed. > >> As we eventually introduce the uptr change into the Linux kernel, and >> say it gets merged for Linux 5.9.0, we mirror this change into glibc >> (e.g. release 2.33), and bump the Linux kernel version cutoff to 5.9.0. >> So starting from that version, we use the Linux kernel header only if >> version >= 5.9.0, else we fallback on glibc's own implementation. > > Fortunately, we don't need to settle this today. 8-) > > Let's stick to the 4.18 definitions for the fallback for now, and > discuss the incorporation of future changes later. OK > >>>> +/* Ensure the compiler supports __attribute__ ((aligned)). */ >>>> +_Static_assert (__alignof__ (struct rseq_cs) >= 32, "alignment"); >>>> +_Static_assert (__alignof__ (struct rseq) >= 32, "alignment"); >>> >>> This needs #ifndef __cplusplus or something like that. I'm surprised >>> that this passes the installed header tests. >> >> Would the following be ok ? >> >> #ifdef __cplusplus >> #define rseq_static_assert static_assert >> #else >> #define rseq_static_assert _Static_assert >> #endif >> >> /* Ensure the compiler supports __attribute__ ((aligned)). */ >> rseq_static_assert (__alignof__ (struct rseq_cs) >= 32, "alignment"); >> rseq_static_assert (__alignof__ (struct rseq) >= 32, "alignment"); > > Seems reasonable, yes. __alignof__ is still a GCC extension. C++11 has > alignof, C11 has _Alignof. So you could use something like this > (perhaps without indentation for the kernel header version): > > #ifdef __cplusplus > # if __cplusplus >= 201103L > # define rseq_static_assert(x) static_assert x; > # define rseq_alignof alignof > # endif > #elif __STDC_VERSION__ >= 201112L > # define rseq_static_assert(x) _Static_assert x; > # define rseq_alignof _Alignof > #endif > #ifndef rseq_static_assert > # define rseq_static_assert /* nothing */ > #endif > rseq_static_assert ((rseq_alignof__ (struct rseq_cs) >= 32, "alignment")) > rseq_static_assert ((rseq_alignof (struct rseq) >= 32, "alignment")) Something like this ? #ifdef __cplusplus # if __cplusplus >= 201103L # define rseq_static_assert (expr, diagnostic) static_assert (expr, diagnostic) # define rseq_alignof alignof # endif #elif __STDC_VERSION__ >= 201112L # define rseq_static_assert (expr, diagnostic) _Static_assert (expr, diagnostic) # define rseq_alignof _Alignof #endif #ifndef rseq_static_assert # define rseq_static_assert (expr, diagnostic) /* nothing */ #endif /* Ensure the compiler supports __attribute__ ((aligned)). */ rseq_static_assert ((rseq_alignof (struct rseq_cs) >= 32, "alignment")); rseq_static_assert ((rseq_alignof (struct rseq) >= 32, "alignment")); > And something similar for _Alignas/attribute aligned, I don't see where _Alignas is needed here ? For attribute aligned, what would be the oldest supported C and C++ standards ? > with an error for > older standards and !__GNUC__ compilers (because neither the type nor > __thread can be represented there). By "type" you mean "struct rseq" here ? What does it contain that requires a __GNUC__ compiler ? About __thread, I recall other compilers have other means to declare it. In liburcu, I end up with the following: #if defined (__cplusplus) && (__cplusplus >= 201103L) # define URCU_TLS_STORAGE_CLASS thread_local #elif defined (__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L) # define URCU_TLS_STORAGE_CLASS _Thread_local #elif defined (_MSC_VER) # define URCU_TLS_STORAGE_CLASS __declspec(thread) #else # define URCU_TLS_STORAGE_CLASS __thread #endif Would something along those lines be OK for libc ? Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) 2020-05-25 17:36 ` Mathieu Desnoyers via Libc-alpha @ 2020-05-26 12:41 ` Florian Weimer via Libc-alpha 2020-05-26 14:32 ` Mathieu Desnoyers via Libc-alpha 0 siblings, 1 reply; 27+ messages in thread From: Florian Weimer via Libc-alpha @ 2020-05-26 12:41 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Rich Felker, libc-alpha, Peter Zijlstra, linux-api, Boqun Feng, Will Deacon, linux-kernel, Ben Maurer, Dave Watson, Thomas Gleixner, Paul, Paul Turner, Joseph Myers * Mathieu Desnoyers: > Something like this ? > > #ifdef __cplusplus > # if __cplusplus >= 201103L > # define rseq_static_assert (expr, diagnostic) static_assert (expr, diagnostic) > # define rseq_alignof alignof > # endif > #elif __STDC_VERSION__ >= 201112L > # define rseq_static_assert (expr, diagnostic) _Static_assert (expr, diagnostic) > # define rseq_alignof _Alignof > #endif > > #ifndef rseq_static_assert > # define rseq_static_assert (expr, diagnostic) /* nothing */ > #endif You can't have a space in #defines like that, no matter what GNU style says. 8-) > /* Ensure the compiler supports __attribute__ ((aligned)). */ > rseq_static_assert ((rseq_alignof (struct rseq_cs) >= 32, "alignment")); > rseq_static_assert ((rseq_alignof (struct rseq) >= 32, "alignment")); You need to move the ; into rseq_static_assert. And if you use explicit arguments, you can't use double parentheses. >> And something similar for _Alignas/attribute aligned, > > I don't see where _Alignas is needed here ? > > For attribute aligned, what would be the oldest supported C and C++ > standards ? There are no standardized attributes for C, there is only _Alignas. C++11 has an alignas specifier; it's not an attribute either. I think these are syntactically similar. >> with an error for >> older standards and !__GNUC__ compilers (because neither the type nor >> __thread can be represented there). > > By "type" you mean "struct rseq" here ? What does it contain that requires > a __GNUC__ compiler ? __attribute__ and __thread support. > About __thread, I recall other compilers have other means to declare it. > In liburcu, I end up with the following: > > #if defined (__cplusplus) && (__cplusplus >= 201103L) > # define URCU_TLS_STORAGE_CLASS thread_local > #elif defined (__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L) > # define URCU_TLS_STORAGE_CLASS _Thread_local > #elif defined (_MSC_VER) > # define URCU_TLS_STORAGE_CLASS __declspec(thread) > #else > # define URCU_TLS_STORAGE_CLASS __thread > #endif > > Would something along those lines be OK for libc ? Yes, it would be okay (minus the Visual C++ part). This part does not have to go into UAPI headers first. A fallback definition of __thread should be okay. Outside glibc, the TLS model declaration is optional, I think. The glibc *definition* ensures that the variable is initial-exec. Thanks, Florian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) 2020-05-26 12:41 ` Florian Weimer via Libc-alpha @ 2020-05-26 14:32 ` Mathieu Desnoyers via Libc-alpha 2020-05-26 14:38 ` Florian Weimer via Libc-alpha 0 siblings, 1 reply; 27+ messages in thread From: Mathieu Desnoyers via Libc-alpha @ 2020-05-26 14:32 UTC (permalink / raw) To: Florian Weimer Cc: Rich Felker, libc-alpha, Peter Zijlstra, linux-api, Boqun Feng, Will Deacon, linux-kernel, Ben Maurer, Dave Watson, Thomas Gleixner, Paul, Paul Turner, Joseph Myers ----- On May 26, 2020, at 8:41 AM, Florian Weimer fweimer@redhat.com wrote: > * Mathieu Desnoyers: > >> Something like this ? >> >> #ifdef __cplusplus >> # if __cplusplus >= 201103L >> # define rseq_static_assert (expr, diagnostic) static_assert (expr, >> diagnostic) >> # define rseq_alignof alignof >> # endif >> #elif __STDC_VERSION__ >= 201112L >> # define rseq_static_assert (expr, diagnostic) _Static_assert (expr, >> diagnostic) >> # define rseq_alignof _Alignof >> #endif >> >> #ifndef rseq_static_assert >> # define rseq_static_assert (expr, diagnostic) /* nothing */ >> #endif > > You can't have a space in #defines like that, no matter what GNU style > says. 8-) Yes, I noticed when failing to build this ;) > >> /* Ensure the compiler supports __attribute__ ((aligned)). */ >> rseq_static_assert ((rseq_alignof (struct rseq_cs) >= 32, "alignment")); >> rseq_static_assert ((rseq_alignof (struct rseq) >= 32, "alignment")); > > You need to move the ; into rseq_static_assert. And if you use explicit > arguments, you can't use double parentheses. Why move the ";" into the macro ? AFAIU, the only gain here would be to make sure we don't emit useless ";" in the "/* nothing */" case. But does it matter ? Examples I can find of "static_assert" explicitly have the ";" at the end, so I find it weird to integrate it into the rseq_static_assert macro, which makes it different from static_assert. Agreed on the need to remove the double-parentheses. > >>> And something similar for _Alignas/attribute aligned, >> >> I don't see where _Alignas is needed here ? >> >> For attribute aligned, what would be the oldest supported C and C++ >> standards ? > > There are no standardized attributes for C, there is only _Alignas. > C++11 has an alignas specifier; it's not an attribute either. I think > these are syntactically similar. There appears to be an interesting difference between attribute aligned and alignas. It seems like alignas cannot be used on a structure declaration, only on fields, e.g.: struct blah { int a; } _Alignas (16); o.c:3:1: warning: useless ‘_Alignas’ in empty declaration } _Alignas (16); But struct blah { int _Alignas (16) a; }; is OK. So if I change e.g. struct rseq_cs to align the first field: struct rseq_cs { /* Version of this structure. */ uint32_t rseq_align (32) version; /* enum rseq_cs_flags. */ uint32_t flags; uint64_t start_ip; /* Offset from start_ip. */ uint64_t post_commit_offset; uint64_t abort_ip; }; It should work. > >>> with an error for >>> older standards and !__GNUC__ compilers (because neither the type nor >>> __thread can be represented there). >> >> By "type" you mean "struct rseq" here ? What does it contain that requires >> a __GNUC__ compiler ? > > __attribute__ and __thread support. OK > >> About __thread, I recall other compilers have other means to declare it. >> In liburcu, I end up with the following: >> >> #if defined (__cplusplus) && (__cplusplus >= 201103L) >> # define URCU_TLS_STORAGE_CLASS thread_local >> #elif defined (__STDC_VERSION__) && (__STDC_VERSION__ >= 201112L) >> # define URCU_TLS_STORAGE_CLASS _Thread_local >> #elif defined (_MSC_VER) >> # define URCU_TLS_STORAGE_CLASS __declspec(thread) >> #else >> # define URCU_TLS_STORAGE_CLASS __thread >> #endif >> >> Would something along those lines be OK for libc ? > > Yes, it would be okay (minus the Visual C++ part). This part does not > have to go into UAPI headers first. A fallback definition of __thread > should be okay. Outside glibc, the TLS model declaration is optional, I > think. The glibc *definition* ensures that the variable is > initial-exec. AFAIU you are technically correct when stating that the tls model on the declaration is optional, but I think it's a good thing to have it there rather than only at the definition. It makes it clear to all users of this variable that its model is IE. Especially in scenarios where early-adopter libraries and applications can define their own __rseq_abi symbol, I think it's good to explicitly keep the IE tls model attribute in the header. I end up with the following: #ifdef __cplusplus # if __cplusplus >= 201103L # define rseq_static_assert(expr, diagnostic) static_assert (expr, diagnostic) # define rseq_alignof(type) alignof (type) # define rseq_alignas(x) alignas (x) # define rseq_tls_storage_class thread_local # endif #elif (defined __STDC_VERSION__ ? __STDC_VERSION__ : 0) >= 201112L # define rseq_static_assert(expr, diagnostic) _Static_assert (expr, diagnostic) # define rseq_alignof(type) _Alignof (type) # define rseq_alignas(x) _Alignas (x) # define rseq_tls_storage_class _Thread_local #endif #ifndef rseq_static_assert /* Try to use _Static_assert macro from sys/cdefs.h. */ # ifdef _Static_assert # define rseq_static_assert(expr, diagnostic) _Static_assert (expr, diagnostic) # else # define rseq_static_assert(expr, diagnostic) /* Nothing. */ # endif #endif /* Rely on GNU extensions for older standards and tls model. */ #ifdef __GNUC__ # ifndef rseq_alignof # define rseq_alignof(x) __alignof__ (x) # endif # ifndef rseq_alignas # define rseq_alignas(x) __attribute__ ((aligned (x))) # endif # define rseq_tls_model_ie __attribute__ ((__tls_model__ ("initial-exec"))) #else /* Specifying the TLS model on the declaration is optional. */ # define rseq_tls_model_ie /* Nothing. */ #endif /* Fall back to __thread for TLS storage class. */ #ifndef rseq_tls_storage_class # define rseq_tls_storage_class __thread #endif [...] /* Ensure the compiler supports rseq_align. */ rseq_static_assert (rseq_alignof (struct rseq_cs) >= 32, "alignment"); rseq_static_assert (rseq_alignof (struct rseq) >= 32, "alignment"); /* Allocations of struct rseq and struct rseq_cs on the heap need to be aligned on 32 bytes. Therefore, use of malloc is discouraged because it does not guarantee alignment. posix_memalign should be used instead. */ extern rseq_tls_storage_class struct rseq __rseq_abi rseq_tls_model_ie; Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) 2020-05-26 14:32 ` Mathieu Desnoyers via Libc-alpha @ 2020-05-26 14:38 ` Florian Weimer via Libc-alpha 2020-05-26 14:53 ` Mathieu Desnoyers via Libc-alpha 0 siblings, 1 reply; 27+ messages in thread From: Florian Weimer via Libc-alpha @ 2020-05-26 14:38 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Rich Felker, libc-alpha, Peter Zijlstra, linux-api, Boqun Feng, Will Deacon, linux-kernel, Ben Maurer, Dave Watson, Thomas Gleixner, Paul, Paul Turner, Joseph Myers * Mathieu Desnoyers: > AFAIU, the only gain here would be to make sure we don't emit useless > ";" in the "/* nothing */" case. But does it matter ? I don't think C allows empty constructs like this at the top level. >>>> And something similar for _Alignas/attribute aligned, >>> >>> I don't see where _Alignas is needed here ? >>> >>> For attribute aligned, what would be the oldest supported C and C++ >>> standards ? >> >> There are no standardized attributes for C, there is only _Alignas. >> C++11 has an alignas specifier; it's not an attribute either. I think >> these are syntactically similar. > > There appears to be an interesting difference between attribute aligned > and alignas. It seems like alignas cannot be used on a structure declaration, > only on fields, e.g.: > > struct blah { > int a; > } _Alignas (16); > > o.c:3:1: warning: useless ‘_Alignas’ in empty declaration > } _Alignas (16); > > But > > struct blah { > int _Alignas (16) a; > }; Like the attribute, it needs to come right after the struct keyword, I think. (Trailing attributes can be ambiguous, but not in this case.) > is OK. So if I change e.g. struct rseq_cs to align > the first field: > > struct rseq_cs > { > /* Version of this structure. */ > uint32_t rseq_align (32) version; > /* enum rseq_cs_flags. */ > uint32_t flags; > uint64_t start_ip; > /* Offset from start_ip. */ > uint64_t post_commit_offset; > uint64_t abort_ip; > }; > > It should work. Indeed. > /* Rely on GNU extensions for older standards and tls model. */ > #ifdef __GNUC__ > # ifndef rseq_alignof > # define rseq_alignof(x) __alignof__ (x) > # endif > # ifndef rseq_alignas > # define rseq_alignas(x) __attribute__ ((aligned (x))) > # endif > # define rseq_tls_model_ie __attribute__ ((__tls_model__ ("initial-exec"))) > #else > /* Specifying the TLS model on the declaration is optional. */ > # define rseq_tls_model_ie /* Nothing. */ > #endif > > /* Fall back to __thread for TLS storage class. */ > #ifndef rseq_tls_storage_class > # define rseq_tls_storage_class __thread > #endif If they are only used in the glibc headers, they should have __rseq prefixes, so that application code doesn't start using them (in case we have to change/fix them, or move the into <sys/cdefs.h> later). Rest looks fine. Thanks, Florian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) 2020-05-26 14:38 ` Florian Weimer via Libc-alpha @ 2020-05-26 14:53 ` Mathieu Desnoyers via Libc-alpha 2020-05-26 14:57 ` Florian Weimer via Libc-alpha 0 siblings, 1 reply; 27+ messages in thread From: Mathieu Desnoyers via Libc-alpha @ 2020-05-26 14:53 UTC (permalink / raw) To: Florian Weimer Cc: Rich Felker, libc-alpha, Peter Zijlstra, linux-api, Boqun Feng, Will Deacon, linux-kernel, Ben Maurer, Dave Watson, Thomas Gleixner, Paul, Paul Turner, Joseph Myers ----- On May 26, 2020, at 10:38 AM, Florian Weimer fweimer@redhat.com wrote: > * Mathieu Desnoyers: > >> AFAIU, the only gain here would be to make sure we don't emit useless >> ";" in the "/* nothing */" case. But does it matter ? > > I don't think C allows empty constructs like this at the top level. > >>>>> And something similar for _Alignas/attribute aligned, >>>> >>>> I don't see where _Alignas is needed here ? >>>> >>>> For attribute aligned, what would be the oldest supported C and C++ >>>> standards ? >>> >>> There are no standardized attributes for C, there is only _Alignas. >>> C++11 has an alignas specifier; it's not an attribute either. I think >>> these are syntactically similar. >> >> There appears to be an interesting difference between attribute aligned >> and alignas. It seems like alignas cannot be used on a structure declaration, >> only on fields, e.g.: >> >> struct blah { >> int a; >> } _Alignas (16); >> >> o.c:3:1: warning: useless ‘_Alignas’ in empty declaration >> } _Alignas (16); >> >> But >> >> struct blah { >> int _Alignas (16) a; >> }; > > Like the attribute, it needs to come right after the struct keyword, I > think. (Trailing attributes can be ambiguous, but not in this case.) Nope. _Alignas really _is_ special :-( struct _Alignas (16) blah { int a; }; p.c:1:8: error: expected ‘{’ before ‘_Alignas’ struct _Alignas (16) blah { Also: struct blah _Alignas (16) { int a; }; p.c:1:27: error: expected identifier or ‘(’ before ‘{’ token struct blah _Alignas (16) { > >> is OK. So if I change e.g. struct rseq_cs to align >> the first field: >> >> struct rseq_cs >> { >> /* Version of this structure. */ >> uint32_t rseq_align (32) version; >> /* enum rseq_cs_flags. */ >> uint32_t flags; >> uint64_t start_ip; >> /* Offset from start_ip. */ >> uint64_t post_commit_offset; >> uint64_t abort_ip; >> }; >> >> It should work. > > Indeed. OK, so let's go for that approach. > >> /* Rely on GNU extensions for older standards and tls model. */ >> #ifdef __GNUC__ >> # ifndef rseq_alignof >> # define rseq_alignof(x) __alignof__ (x) >> # endif >> # ifndef rseq_alignas >> # define rseq_alignas(x) __attribute__ ((aligned (x))) >> # endif >> # define rseq_tls_model_ie __attribute__ ((__tls_model__ ("initial-exec"))) >> #else >> /* Specifying the TLS model on the declaration is optional. */ >> # define rseq_tls_model_ie /* Nothing. */ >> #endif >> >> /* Fall back to __thread for TLS storage class. */ >> #ifndef rseq_tls_storage_class >> # define rseq_tls_storage_class __thread >> #endif > > If they are only used in the glibc headers, they should have __rseq > prefixes, so that application code doesn't start using them (in case we > have to change/fix them, or move the into <sys/cdefs.h> later). OK will do. > > Rest looks fine. One last thing I'm planning to add in sys/rseq.h to cover acessing the rseq_cs pointers with both the UAPI headers and the glibc struct rseq declarations: /* The rseq_cs_ptr macro can be used to access the pointer to the current rseq critical section descriptor. */ #ifdef __LP64__ # define rseq_cs_ptr(rseq) \ ((const struct rseq_cs *) (rseq)->rseq_cs.ptr) #else /* __LP64__ */ # define rseq_cs_ptr(rseq) \ ((const struct rseq_cs *) (rseq)->rseq_cs.ptr.ptr32) #endif /* __LP64__ */ Does it make sense ? Thanks, Mathieu > > Thanks, > Florian -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) 2020-05-26 14:53 ` Mathieu Desnoyers via Libc-alpha @ 2020-05-26 14:57 ` Florian Weimer via Libc-alpha 2020-05-26 15:22 ` Mathieu Desnoyers via Libc-alpha 0 siblings, 1 reply; 27+ messages in thread From: Florian Weimer via Libc-alpha @ 2020-05-26 14:57 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Rich Felker, libc-alpha, Peter Zijlstra, linux-api, Boqun Feng, Will Deacon, linux-kernel, Ben Maurer, Dave Watson, Thomas Gleixner, Paul, Paul Turner, Joseph Myers * Mathieu Desnoyers: >> Like the attribute, it needs to come right after the struct keyword, I >> think. (Trailing attributes can be ambiguous, but not in this case.) > > Nope. _Alignas really _is_ special :-( > > struct _Alignas (16) blah { > int a; > }; > > p.c:1:8: error: expected ‘{’ before ‘_Alignas’ > struct _Alignas (16) blah { Meh, yet another unnecessary C++ incompatibility. C does not support empty structs, so I assume they didn't see the field requirement as a burden. > One last thing I'm planning to add in sys/rseq.h to cover acessing the > rseq_cs pointers with both the UAPI headers and the glibc struct rseq > declarations: > > /* The rseq_cs_ptr macro can be used to access the pointer to the current > rseq critical section descriptor. */ > #ifdef __LP64__ > # define rseq_cs_ptr(rseq) \ > ((const struct rseq_cs *) (rseq)->rseq_cs.ptr) > #else /* __LP64__ */ > # define rseq_cs_ptr(rseq) \ > ((const struct rseq_cs *) (rseq)->rseq_cs.ptr.ptr32) > #endif /* __LP64__ */ > > Does it make sense ? Written this way, it's an aliasing violation. I don't think it's very useful. Thanks, Florian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) 2020-05-26 14:57 ` Florian Weimer via Libc-alpha @ 2020-05-26 15:22 ` Mathieu Desnoyers via Libc-alpha 0 siblings, 0 replies; 27+ messages in thread From: Mathieu Desnoyers via Libc-alpha @ 2020-05-26 15:22 UTC (permalink / raw) To: Florian Weimer Cc: Rich Felker, libc-alpha, Peter Zijlstra, linux-api, Boqun Feng, Will Deacon, linux-kernel, Ben Maurer, Dave Watson, Thomas Gleixner, Paul, Paul Turner, Joseph Myers ----- On May 26, 2020, at 10:57 AM, Florian Weimer fweimer@redhat.com wrote: > * Mathieu Desnoyers: > >>> Like the attribute, it needs to come right after the struct keyword, I >>> think. (Trailing attributes can be ambiguous, but not in this case.) >> >> Nope. _Alignas really _is_ special :-( >> >> struct _Alignas (16) blah { >> int a; >> }; >> >> p.c:1:8: error: expected ‘{’ before ‘_Alignas’ >> struct _Alignas (16) blah { > > Meh, yet another unnecessary C++ incompatibility. C does not support > empty structs, so I assume they didn't see the field requirement as a > burden. Indeed, it's weird. > >> One last thing I'm planning to add in sys/rseq.h to cover acessing the >> rseq_cs pointers with both the UAPI headers and the glibc struct rseq >> declarations: >> >> /* The rseq_cs_ptr macro can be used to access the pointer to the current >> rseq critical section descriptor. */ >> #ifdef __LP64__ >> # define rseq_cs_ptr(rseq) \ >> ((const struct rseq_cs *) (rseq)->rseq_cs.ptr) >> #else /* __LP64__ */ >> # define rseq_cs_ptr(rseq) \ >> ((const struct rseq_cs *) (rseq)->rseq_cs.ptr.ptr32) >> #endif /* __LP64__ */ >> >> Does it make sense ? > > Written this way, it's an aliasing violation. I don't think it's very > useful. OK, I'll just remove it. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH glibc 2/3] glibc: sched_getcpu(): use rseq cpu_id TLS on Linux (v7) 2020-05-01 2:14 [PATCH glibc 0/3] Restartable Sequences enablement Mathieu Desnoyers via Libc-alpha 2020-05-01 2:14 ` [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) Mathieu Desnoyers via Libc-alpha @ 2020-05-01 2:14 ` Mathieu Desnoyers via Libc-alpha 2020-05-20 10:14 ` Florian Weimer via Libc-alpha 2020-05-01 2:14 ` [PATCH glibc 3/3] rseq registration tests (v10) Mathieu Desnoyers via Libc-alpha 2020-05-20 11:44 ` [PATCH glibc 0/3] Restartable Sequences enablement Florian Weimer via Libc-alpha 3 siblings, 1 reply; 27+ messages in thread From: Mathieu Desnoyers via Libc-alpha @ 2020-05-01 2:14 UTC (permalink / raw) To: Florian Weimer Cc: libc-alpha, linux-api, Boqun Feng, Will Deacon, linux-kernel, Peter Zijlstra, Ben Maurer, Mathieu Desnoyers, Thomas Gleixner, Paul E. McKenney, Paul Turner, Joseph Myers When available, use the cpu_id field from __rseq_abi on Linux to implement sched_getcpu(). Fall-back on the vgetcpu vDSO if unavailable. Benchmarks: x86-64: Intel E5-2630 v3@2.40GHz, 16-core, hyperthreading glibc sched_getcpu(): 13.7 ns (baseline) glibc sched_getcpu() using rseq: 2.5 ns (speedup: 5.5x) inline load cpuid from __rseq_abi TLS: 0.8 ns (speedup: 17.1x) CC: Carlos O'Donell <carlos@redhat.com> CC: Florian Weimer <fweimer@redhat.com> CC: Joseph Myers <joseph@codesourcery.com> CC: Szabolcs Nagy <szabolcs.nagy@arm.com> CC: Thomas Gleixner <tglx@linutronix.de> CC: Ben Maurer <bmaurer@fb.com> CC: Peter Zijlstra <peterz@infradead.org> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> CC: Boqun Feng <boqun.feng@gmail.com> CC: Will Deacon <will.deacon@arm.com> CC: Paul Turner <pjt@google.com> CC: libc-alpha@sourceware.org CC: linux-kernel@vger.kernel.org CC: linux-api@vger.kernel.org --- Changes since v1: - rseq is only used if both __NR_rseq and RSEQ_SIG are defined. Changes since v2: - remove duplicated __rseq_abi extern declaration. Changes since v3: - update ChangeLog. Changes since v4: - Use atomic_load_relaxed to load the __rseq_abi.cpu_id field, a consequence of the fact that __rseq_abi is not volatile anymore. - Include atomic.h which provides atomic_load_relaxed. Changes since v5: - Use __ASSUME_RSEQ to detect rseq availability. Changes since v6: - Remove use of __ASSUME_RSEQ. --- sysdeps/unix/sysv/linux/sched_getcpu.c | 27 ++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/sysdeps/unix/sysv/linux/sched_getcpu.c b/sysdeps/unix/sysv/linux/sched_getcpu.c index c019cfb3cf..2269c4f2bd 100644 --- a/sysdeps/unix/sysv/linux/sched_getcpu.c +++ b/sysdeps/unix/sysv/linux/sched_getcpu.c @@ -18,10 +18,15 @@ #include <errno.h> #include <sched.h> #include <sysdep.h> +#include <atomic.h> #include <sysdep-vdso.h> -int -sched_getcpu (void) +#ifdef HAVE_GETCPU_VSYSCALL +# define HAVE_VSYSCALL +#endif + +static int +vsyscall_sched_getcpu (void) { unsigned int cpu; int r = -1; @@ -32,3 +37,21 @@ sched_getcpu (void) #endif return r == -1 ? r : cpu; } + +#include <sys/rseq.h> + +#ifdef RSEQ_SIG +int +sched_getcpu (void) +{ + int cpu_id = atomic_load_relaxed (&__rseq_abi.cpu_id); + + return cpu_id >= 0 ? cpu_id : vsyscall_sched_getcpu (); +} +#else +int +sched_getcpu (void) +{ + return vsyscall_sched_getcpu (); +} +#endif -- 2.17.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 2/3] glibc: sched_getcpu(): use rseq cpu_id TLS on Linux (v7) 2020-05-01 2:14 ` [PATCH glibc 2/3] glibc: sched_getcpu(): use rseq cpu_id TLS on Linux (v7) Mathieu Desnoyers via Libc-alpha @ 2020-05-20 10:14 ` Florian Weimer via Libc-alpha 0 siblings, 0 replies; 27+ messages in thread From: Florian Weimer via Libc-alpha @ 2020-05-20 10:14 UTC (permalink / raw) To: Mathieu Desnoyers via Libc-alpha Cc: Peter Zijlstra, linux-api, Boqun Feng, Will Deacon, linux-kernel, Ben Maurer, Mathieu Desnoyers, Thomas Gleixner, Paul E. McKenney, Paul Turner, Joseph Myers * Mathieu Desnoyers via Libc-alpha: > diff --git a/sysdeps/unix/sysv/linux/sched_getcpu.c b/sysdeps/unix/sysv/linux/sched_getcpu.c > index c019cfb3cf..2269c4f2bd 100644 > --- a/sysdeps/unix/sysv/linux/sched_getcpu.c > +++ b/sysdeps/unix/sysv/linux/sched_getcpu.c > @@ -18,10 +18,15 @@ > #include <errno.h> > #include <sched.h> > #include <sysdep.h> > +#include <atomic.h> > #include <sysdep-vdso.h> > > -int > -sched_getcpu (void) > +#ifdef HAVE_GETCPU_VSYSCALL > +# define HAVE_VSYSCALL > +#endif I think the #ifdef is a result of an incorrect merge of commit d0def09ff6bbf1537beec305fdfe96a21174fb31 ("linux: Fix vDSO macros build with time64 interfaces") and it should be removed. The commit subject should probably say “Linux: Use rseq in sched_getcpu if available”. Thanks, Florian ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH glibc 3/3] rseq registration tests (v10) 2020-05-01 2:14 [PATCH glibc 0/3] Restartable Sequences enablement Mathieu Desnoyers via Libc-alpha 2020-05-01 2:14 ` [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) Mathieu Desnoyers via Libc-alpha 2020-05-01 2:14 ` [PATCH glibc 2/3] glibc: sched_getcpu(): use rseq cpu_id TLS on Linux (v7) Mathieu Desnoyers via Libc-alpha @ 2020-05-01 2:14 ` Mathieu Desnoyers via Libc-alpha 2020-05-20 10:52 ` Florian Weimer via Libc-alpha 2020-05-20 11:44 ` [PATCH glibc 0/3] Restartable Sequences enablement Florian Weimer via Libc-alpha 3 siblings, 1 reply; 27+ messages in thread From: Mathieu Desnoyers via Libc-alpha @ 2020-05-01 2:14 UTC (permalink / raw) To: Florian Weimer Cc: libc-alpha, Boqun Feng, Will Deacon, Peter Zijlstra, Ben Maurer, Mathieu Desnoyers, Thomas Gleixner, Paul E. McKenney, Paul Turner, Joseph Myers These tests validate that rseq is registered from various execution contexts (main thread, constructor, destructor, other threads, other threads created from constructor and destructor, forked process (without exec), pthread_atfork handlers, pthread setspecific destructors, C++ thread and process destructors, signal handlers, atexit handlers). tst-rseq.c only links against libc.so, testing registration of rseq in a non-multithreaded environment. tst-rseq-nptl.c also links against libpthread.so, testing registration of rseq in a multithreaded environment. See the Linux kernel selftests for extensive rseq stress-tests. CC: Carlos O'Donell <carlos@redhat.com> CC: Florian Weimer <fweimer@redhat.com> CC: Joseph Myers <joseph@codesourcery.com> CC: Szabolcs Nagy <szabolcs.nagy@arm.com> CC: Thomas Gleixner <tglx@linutronix.de> CC: Ben Maurer <bmaurer@fb.com> CC: Peter Zijlstra <peterz@infradead.org> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> CC: Boqun Feng <boqun.feng@gmail.com> CC: Will Deacon <will.deacon@arm.com> CC: Paul Turner <pjt@google.com> CC: libc-alpha@sourceware.org --- Changes since v1: - Rename tst-rseq.c to tst-rseq-nptl.c. - Introduce tst-rseq.c testing rseq registration in a non-multithreaded environment. Chances since v2: - Update file headers. - use xpthread key create/delete. - remove set stacksize. - Tests depend on both __NR_rseq and RSEQ_SIG being defined. Changes since v3: - Update ChangeLog. Changes since v4: - Remove volatile from sys_rseq() rseq_abi parameter. - Use atomic_load_relaxed to load __rseq_abi.cpu_id, consequence of the fact that __rseq_abi is not volatile anymore. - Include atomic.h from tst-rseq.c for use of atomic_load_relaxed. Move tst-rseq.c to internal tests within Makefile due to its use of atomic.h. - Test __rseq_handled initialization by glibc. Changes since v5: - Rebase on glibc 2.30. Changes since v6: - Remove __rseq_handled. Changes since v7: - Update copyright range to include 2020. - Use __ASSUME_RSEQ to detect rseq availability. Changes since v8: - Remove use of __ASSUME_RSEQ. Changes since v9: - Adapt to new prototype for xpthread_key_create. - Update copyright year to 2020. - Remove constructor test (moved to later patch due to test harness modification dependency). - Change http:// for https://. --- sysdeps/unix/sysv/linux/Makefile | 6 +- sysdeps/unix/sysv/linux/tst-rseq-nptl.c | 338 ++++++++++++++++++++++++ sysdeps/unix/sysv/linux/tst-rseq.c | 108 ++++++++ 3 files changed, 450 insertions(+), 2 deletions(-) create mode 100644 sysdeps/unix/sysv/linux/tst-rseq-nptl.c create mode 100644 sysdeps/unix/sysv/linux/tst-rseq.c diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile index e3ed374d9e..598c0ecfdd 100644 --- a/sysdeps/unix/sysv/linux/Makefile +++ b/sysdeps/unix/sysv/linux/Makefile @@ -100,7 +100,9 @@ tests += tst-clone tst-clone2 tst-clone3 tst-fanotify tst-personality \ test-errno-linux tst-memfd_create tst-mlock2 tst-pkey \ tst-rlimit-infinity tst-ofdlocks tst-gettid tst-gettid-kill \ tst-tgkill -tests-internal += tst-ofdlocks-compat tst-sigcontext-get_pc + +tests-internal += tst-ofdlocks-compat tst-sigcontext-get_pc \ + tst-ofdlocks-compat tst-rseq CFLAGS-tst-sigcontext-get_pc.c = -fasynchronous-unwind-tables @@ -303,5 +305,5 @@ ifeq ($(subdir),nptl) tests += tst-align-clone tst-getpid1 \ tst-thread-affinity-pthread tst-thread-affinity-pthread2 \ tst-thread-affinity-sched -tests-internal += tst-setgetname +tests-internal += tst-setgetname tst-rseq-nptl endif diff --git a/sysdeps/unix/sysv/linux/tst-rseq-nptl.c b/sysdeps/unix/sysv/linux/tst-rseq-nptl.c new file mode 100644 index 0000000000..3e3996d686 --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-rseq-nptl.c @@ -0,0 +1,338 @@ +/* Restartable Sequences NPTL test. + + Copyright (C) 2020 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/>. */ + +/* These tests validate that rseq is registered from various execution + contexts (main thread, destructor, other threads, other threads created + from destructor, forked process (without exec), pthread_atfork handlers, + pthread setspecific destructors, C++ thread and process destructors, + signal handlers, atexit handlers). + + See the Linux kernel selftests for extensive rseq stress-tests. */ + +#include <sys/syscall.h> +#include <unistd.h> +#include <stdio.h> +#include <support/check.h> +#include <support/xthread.h> +#include <sys/rseq.h> + +#ifdef RSEQ_SIG +#include <pthread.h> +#include <syscall.h> +#include <stdlib.h> +#include <error.h> +#include <errno.h> +#include <string.h> +#include <stdint.h> +#include <sys/types.h> +#include <sys/wait.h> +#include <signal.h> +#include <atomic.h> + +static pthread_key_t rseq_test_key; + +static int +rseq_thread_registered (void) +{ + return (int32_t) atomic_load_relaxed (&__rseq_abi.cpu_id) >= 0; +} + +static void +atfork_prepare (void) +{ + if (!rseq_thread_registered ()) + { + printf ("rseq not registered in pthread atfork prepare\n"); + support_record_failure (); + } +} + +static void +atfork_parent (void) +{ + if (!rseq_thread_registered ()) + { + printf ("rseq not registered in pthread atfork parent\n"); + support_record_failure (); + } +} + +static void +atfork_child (void) +{ + if (!rseq_thread_registered ()) + { + printf ("rseq not registered in pthread atfork child\n"); + support_record_failure (); + } +} + +static void +rseq_key_destructor (void *arg) +{ + /* Cannot use deferred failure reporting after main () returns. */ + if (!rseq_thread_registered ()) + FAIL_EXIT1 ("rseq not registered in pthread key destructor"); +} + +static void +atexit_handler (void) +{ + /* Cannot use deferred failure reporting after main () returns. */ + if (!rseq_thread_registered ()) + FAIL_EXIT1 ("rseq not registered in atexit handler"); +} + +static int +do_rseq_main_test (void) +{ + if (atexit (atexit_handler)) + FAIL_EXIT1 ("error calling atexit"); + rseq_test_key = xpthread_key_create (rseq_key_destructor); + if (pthread_atfork (atfork_prepare, atfork_parent, atfork_child)) + FAIL_EXIT1 ("error calling pthread_atfork"); + if (raise (SIGUSR1)) + FAIL_EXIT1 ("error raising signal"); + if (pthread_setspecific (rseq_test_key, (void *) 1l)) + FAIL_EXIT1 ("error in pthread_setspecific"); + if (!rseq_thread_registered ()) + { + FAIL_RET ("rseq not registered in main thread"); + } + return 0; +} + +static void +cancel_routine (void *arg) +{ + if (!rseq_thread_registered ()) + { + printf ("rseq not registered in cancel routine\n"); + support_record_failure (); + } +} + +static int cancel_thread_ready; + +static void +test_cancel_thread (void) +{ + pthread_cleanup_push (cancel_routine, NULL); + atomic_store_release (&cancel_thread_ready, 1); + for (;;) + usleep (100); + pthread_cleanup_pop (0); +} + +static void * +thread_function (void * arg) +{ + int i = (int) (intptr_t) arg; + + if (raise (SIGUSR1)) + FAIL_EXIT1 ("error raising signal"); + if (i == 0) + test_cancel_thread (); + if (pthread_setspecific (rseq_test_key, (void *) 1l)) + FAIL_EXIT1 ("error in pthread_setspecific"); + return rseq_thread_registered () ? NULL : (void *) 1l; +} + +static void +sighandler (int sig) +{ + if (!rseq_thread_registered ()) + { + printf ("rseq not registered in signal handler\n"); + support_record_failure (); + } +} + +static void +setup_signals (void) +{ + struct sigaction sa; + + sigemptyset (&sa.sa_mask); + sigaddset (&sa.sa_mask, SIGUSR1); + sa.sa_flags = 0; + sa.sa_handler = sighandler; + if (sigaction (SIGUSR1, &sa, NULL) != 0) + { + FAIL_EXIT1 ("sigaction failure: %s", strerror (errno)); + } +} + +#define N 7 +static const int t[N] = { 1, 2, 6, 5, 4, 3, 50 }; + +static int +do_rseq_threads_test (int nr_threads) +{ + pthread_t th[nr_threads]; + int i; + int result = 0; + + cancel_thread_ready = 0; + for (i = 0; i < nr_threads; ++i) + if (pthread_create (&th[i], NULL, thread_function, + (void *) (intptr_t) i) != 0) + { + FAIL_EXIT1 ("creation of thread %d failed", i); + } + + while (!atomic_load_acquire (&cancel_thread_ready)) + usleep (100); + + if (pthread_cancel (th[0])) + FAIL_EXIT1 ("error in pthread_cancel"); + + for (i = 0; i < nr_threads; ++i) + { + void *v; + if (pthread_join (th[i], &v) != 0) + { + printf ("join of thread %d failed\n", i); + result = 1; + } + else if (i != 0 && v != NULL) + { + printf ("join %d successful, but child failed\n", i); + result = 1; + } + else if (i == 0 && v == NULL) + { + printf ("join %d successful, child did not fail as expected\n", i); + result = 1; + } + } + return result; +} + +static int +sys_rseq (struct rseq *rseq_abi, uint32_t rseq_len, int flags, uint32_t sig) +{ + return syscall (__NR_rseq, rseq_abi, rseq_len, flags, sig); +} + +static int +rseq_available (void) +{ + int rc; + + rc = sys_rseq (NULL, 0, 0, 0); + if (rc != -1) + FAIL_EXIT1 ("Unexpected rseq return value %d", rc); + switch (errno) + { + case ENOSYS: + return 0; + case EINVAL: + return 1; + default: + FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno)); + } +} + +static int +do_rseq_fork_test (void) +{ + int status; + pid_t pid, retpid; + + pid = fork (); + switch (pid) + { + case 0: + exit (do_rseq_main_test ()); + case -1: + FAIL_EXIT1 ("Unexpected fork error %s", strerror (errno)); + } + retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)); + if (retpid != pid) + { + FAIL_EXIT1 ("waitpid returned %ld, expected %ld", + (long int) retpid, (long int) pid); + } + if (WEXITSTATUS (status)) + { + printf ("rseq not registered in child\n"); + return 1; + } + return 0; +} + +static int +do_rseq_test (void) +{ + int i, result = 0; + + if (!rseq_available ()) + { + FAIL_UNSUPPORTED ("kernel does not support rseq, skipping test"); + } + setup_signals (); + if (raise (SIGUSR1)) + FAIL_EXIT1 ("error raising signal"); + if (do_rseq_main_test ()) + result = 1; + for (i = 0; i < N; i++) + { + if (do_rseq_threads_test (t[i])) + result = 1; + } + if (do_rseq_fork_test ()) + result = 1; + return result; +} + +static void __attribute__ ((destructor)) +do_rseq_destructor_test (void) +{ + /* Cannot use deferred failure reporting after main () returns. */ + if (do_rseq_test ()) + FAIL_EXIT1 ("rseq not registered within destructor"); + xpthread_key_delete (rseq_test_key); +} + +/* Test C++ destructor called at thread and process exit. */ +void +__call_tls_dtors (void) +{ + /* Cannot use deferred failure reporting after main () returns. */ + if (!rseq_thread_registered ()) + FAIL_EXIT1 ("rseq not registered in C++ thread/process exit destructor"); +} +#else +static int +do_rseq_test (void) +{ +#ifndef RSEQ_SIG + FAIL_UNSUPPORTED ("glibc does not define RSEQ_SIG, skipping test"); +#endif + return 0; +} +#endif + +static int +do_test (void) +{ + return do_rseq_test (); +} + +#include <support/test-driver.c> diff --git a/sysdeps/unix/sysv/linux/tst-rseq.c b/sysdeps/unix/sysv/linux/tst-rseq.c new file mode 100644 index 0000000000..48a61f9998 --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-rseq.c @@ -0,0 +1,108 @@ +/* Restartable Sequences single-threaded tests. + + Copyright (C) 2020 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/>. */ + +/* These tests validate that rseq is registered from main in an executable + not linked against libpthread. */ + +#include <sys/syscall.h> +#include <unistd.h> +#include <stdio.h> +#include <support/check.h> +#include <sys/rseq.h> + +#ifdef RSEQ_SIG +#include <syscall.h> +#include <stdlib.h> +#include <error.h> +#include <errno.h> +#include <stdint.h> +#include <string.h> +#include <atomic.h> + +static int +rseq_thread_registered (void) +{ + return (int32_t) atomic_load_relaxed (&__rseq_abi.cpu_id) >= 0; +} + +static int +do_rseq_main_test (void) +{ + if (!rseq_thread_registered ()) + { + FAIL_RET ("rseq not registered in main thread"); + } + return 0; +} + +static int +sys_rseq (struct rseq *rseq_abi, uint32_t rseq_len, int flags, uint32_t sig) +{ + return syscall (__NR_rseq, rseq_abi, rseq_len, flags, sig); +} + +static int +rseq_available (void) +{ + int rc; + + rc = sys_rseq (NULL, 0, 0, 0); + if (rc != -1) + FAIL_EXIT1 ("Unexpected rseq return value %d", rc); + switch (errno) + { + case ENOSYS: + return 0; + case EINVAL: + return 1; + default: + FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno)); + } +} + +static int +do_rseq_test (void) +{ + int result = 0; + + if (!rseq_available ()) + { + FAIL_UNSUPPORTED ("kernel does not support rseq, skipping test"); + } + if (do_rseq_main_test ()) + result = 1; + return result; +} +#else +static int +do_rseq_test (void) +{ +#ifndef RSEQ_SIG + FAIL_UNSUPPORTED ("glibc does not define RSEQ_SIG, skipping test"); +#endif + return 0; +} +#endif + +static int +do_test (void) +{ + return do_rseq_test (); +} + +#include <support/test-driver.c> -- 2.17.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 3/3] rseq registration tests (v10) 2020-05-01 2:14 ` [PATCH glibc 3/3] rseq registration tests (v10) Mathieu Desnoyers via Libc-alpha @ 2020-05-20 10:52 ` Florian Weimer via Libc-alpha 2020-05-25 17:07 ` Mathieu Desnoyers via Libc-alpha 0 siblings, 1 reply; 27+ messages in thread From: Florian Weimer via Libc-alpha @ 2020-05-20 10:52 UTC (permalink / raw) To: Mathieu Desnoyers via Libc-alpha Cc: Peter Zijlstra, Boqun Feng, Will Deacon, Ben Maurer, Mathieu Desnoyers, Thomas Gleixner, Paul E. McKenney, Paul Turner, Joseph Myers * Mathieu Desnoyers via Libc-alpha: > - Include atomic.h from tst-rseq.c for use of atomic_load_relaxed. > Move tst-rseq.c to internal tests within Makefile due to its use of > atomic.h. You could use __atomic builtins to avoid that, or <stdatomic.h>. Both are fine in tests. I suggest to do that, so that the test can be built more easily outside of glibc. However, this needs to remain an internal test because the test needs a defintiion of __NR_rseq from the internal system call list. Regular tests use the installed kernel headers, which may not define __NR_rseq. Maybe mention this in a comment in nptl/Makefile, along with the tests-internal change? > diff --git a/sysdeps/unix/sysv/linux/tst-rseq-nptl.c b/sysdeps/unix/sysv/linux/tst-rseq-nptl.c > new file mode 100644 > index 0000000000..3e3996d686 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/tst-rseq-nptl.c > @@ -0,0 +1,338 @@ > +/* Restartable Sequences NPTL test. > + > + Copyright (C) 2020 Free Software Foundation, Inc. Maybe rmoeve this empty line. > +#ifdef RSEQ_SIG > +#include <pthread.h> > +#include <syscall.h> > +#include <stdlib.h> > +#include <error.h> > +#include <errno.h> > +#include <string.h> > +#include <stdint.h> > +#include <sys/types.h> > +#include <sys/wait.h> > +#include <signal.h> > +#include <atomic.h> This should be: #ifdef RSEQ_SIG # include <pthread.h> # include <stdlib.h> And so on. Nested preprocessor conditionals need to be indented per our style rules. Please also remove the redundant <syscall.h> inclusion. > +static pthread_key_t rseq_test_key; > + > +static int > +rseq_thread_registered (void) > +{ > + return (int32_t) atomic_load_relaxed (&__rseq_abi.cpu_id) >= 0; > +} The return type should be bool. > +static void > +rseq_key_destructor (void *arg) > +{ > + /* Cannot use deferred failure reporting after main () returns. */ No () after function name. > + if (!rseq_thread_registered ()) > + FAIL_EXIT1 ("rseq not registered in pthread key destructor"); > +} > + > +static void > +atexit_handler (void) > +{ > + /* Cannot use deferred failure reporting after main () returns. */ > + if (!rseq_thread_registered ()) > + FAIL_EXIT1 ("rseq not registered in atexit handler"); > +} > + > +static int > +do_rseq_main_test (void) > +{ > + if (atexit (atexit_handler)) > + FAIL_EXIT1 ("error calling atexit"); > + rseq_test_key = xpthread_key_create (rseq_key_destructor); > + if (pthread_atfork (atfork_prepare, atfork_parent, atfork_child)) > + FAIL_EXIT1 ("error calling pthread_atfork"); > + if (raise (SIGUSR1)) > + FAIL_EXIT1 ("error raising signal"); > + if (pthread_setspecific (rseq_test_key, (void *) 1l)) > + FAIL_EXIT1 ("error in pthread_setspecific"); > + if (!rseq_thread_registered ()) > + { > + FAIL_RET ("rseq not registered in main thread"); > + } You could use TEST_COMPARE (atexit (atexit_handler), 0); … TEST_VERIFY (rseq_thread_registered ()); from <support/check.h>. The automatically generated error messages will provide sufficient context, I think. > + return 0; > +} > + > +static void > +cancel_routine (void *arg) > +{ > + if (!rseq_thread_registered ()) > + { > + printf ("rseq not registered in cancel routine\n"); > + support_record_failure (); > + } > +} Please add "error: " to the error message, to make it clearer that it is an error. > + > +static int cancel_thread_ready; > + > +static void > +test_cancel_thread (void) > +{ > + pthread_cleanup_push (cancel_routine, NULL); > + atomic_store_release (&cancel_thread_ready, 1); > + for (;;) > + usleep (100); > + pthread_cleanup_pop (0); > +} This can use a real barrier (pthread_barrier_t), I think. No need for polling then. > +static void * > +thread_function (void * arg) > +{ > + int i = (int) (intptr_t) arg; > + > + if (raise (SIGUSR1)) > + FAIL_EXIT1 ("error raising signal"); This can use xraise. > + if (i == 0) > + test_cancel_thread (); > + if (pthread_setspecific (rseq_test_key, (void *) 1l)) > + FAIL_EXIT1 ("error in pthread_setspecific"); Could use TEST_COMPARE. > + return rseq_thread_registered () ? NULL : (void *) 1l; > +} > + > +static void > +sighandler (int sig) > +{ > + if (!rseq_thread_registered ()) > + { > + printf ("rseq not registered in signal handler\n"); > + support_record_failure (); > + } > +} Please add "error: ". > +static void > +setup_signals (void) > +{ > + struct sigaction sa; > + > + sigemptyset (&sa.sa_mask); > + sigaddset (&sa.sa_mask, SIGUSR1); > + sa.sa_flags = 0; > + sa.sa_handler = sighandler; > + if (sigaction (SIGUSR1, &sa, NULL) != 0) > + { > + FAIL_EXIT1 ("sigaction failure: %s", strerror (errno)); > + } > +} This could use xsigaction. > +#define N 7 > +static const int t[N] = { 1, 2, 6, 5, 4, 3, 50 }; Should these definitions be local to do_rseq_test below? You can avoid defining N by using array_length from <array_length.h>. > +static int > +do_rseq_threads_test (int nr_threads) > +{ > + pthread_t th[nr_threads]; > + int i; > + int result = 0; > + > + cancel_thread_ready = 0; > + for (i = 0; i < nr_threads; ++i) > + if (pthread_create (&th[i], NULL, thread_function, > + (void *) (intptr_t) i) != 0) > + { > + FAIL_EXIT1 ("creation of thread %d failed", i); > + } Could use xpthread_create. > + while (!atomic_load_acquire (&cancel_thread_ready)) > + usleep (100); See above, could use xpthread_barrier_wait. The present code does not wait until all threads have entered their cancellation region, so I'm not sure if the test object is actually met here. > + if (pthread_cancel (th[0])) > + FAIL_EXIT1 ("error in pthread_cancel"); Could use xpthread_cancel. > + > + for (i = 0; i < nr_threads; ++i) > + { > + void *v; > + if (pthread_join (th[i], &v) != 0) > + { > + printf ("join of thread %d failed\n", i); > + result = 1; > + } > + else if (i != 0 && v != NULL) > + { > + printf ("join %d successful, but child failed\n", i); > + result = 1; > + } > + else if (i == 0 && v == NULL) > + { > + printf ("join %d successful, child did not fail as expected\n", i); > + result = 1; > + } Could use xpthread_join. > +static int > +sys_rseq (struct rseq *rseq_abi, uint32_t rseq_len, int flags, uint32_t sig) > +{ > + return syscall (__NR_rseq, rseq_abi, rseq_len, flags, sig); > +} (This is the only remaining place where internal headers are potentially required, I think.) > +static int > +rseq_available (void) > +{ > + int rc; > + > + rc = sys_rseq (NULL, 0, 0, 0); > + if (rc != -1) > + FAIL_EXIT1 ("Unexpected rseq return value %d", rc); > + switch (errno) > + { > + case ENOSYS: > + return 0; > + case EINVAL: > + return 1; > + default: > + FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno)); > + } > +} Maybe add a comment to explain what EINVAL means in this context? > +static int > +do_rseq_fork_test (void) > +{ > + int status; > + pid_t pid, retpid; > + > + pid = fork (); > + switch (pid) > + { > + case 0: > + exit (do_rseq_main_test ()); > + case -1: > + FAIL_EXIT1 ("Unexpected fork error %s", strerror (errno)); > + } Could use xfork. > + retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)); > + if (retpid != pid) > + { > + FAIL_EXIT1 ("waitpid returned %ld, expected %ld", > + (long int) retpid, (long int) pid); > + } Hmm. Is the TEMP_FAILURE_RETRY really needed? Our xwaitpid does not have this. > + if (WEXITSTATUS (status)) > + { > + printf ("rseq not registered in child\n"); > + return 1; > + } This whole thing looks a lot lie support_isolate_in_subprocess from <support/namespace.h>. > +static int > +do_rseq_test (void) > +{ > + int i, result = 0; > + > + if (!rseq_available ()) > + { > + FAIL_UNSUPPORTED ("kernel does not support rseq, skipping test"); > + } Braces are not needed here. > + setup_signals (); > + if (raise (SIGUSR1)) > + FAIL_EXIT1 ("error raising signal"); Could use xraise. > + if (do_rseq_main_test ()) > + result = 1; > + for (i = 0; i < N; i++) > + { > + if (do_rseq_threads_test (t[i])) > + result = 1; > + } Braces are unnecessary. > +/* Test C++ destructor called at thread and process exit. */ > +void > +__call_tls_dtors (void) > +{ > + /* Cannot use deferred failure reporting after main () returns. */ > + if (!rseq_thread_registered ()) > + FAIL_EXIT1 ("rseq not registered in C++ thread/process exit destructor"); > +} Uhm, what is this supposed to accomplish, under the __call_tls_dtors name in particular? I don't think this gets ever called. It may make sense to have a separate, smaller C++ test to cover this (perhaps as a separate patch). > +#else This needs a comment on the same line. I think it corresponds to the earlier “#ifdef RSEQ_SIG”. > +static int > +do_rseq_test (void) > +{ > +#ifndef RSEQ_SIG > + FAIL_UNSUPPORTED ("glibc does not define RSEQ_SIG, skipping test"); > +#endif And with the comment, it should be obvious that the #ifndef is not needed here. 8-) > + return 0; > +} > +#endif Likewise: Add comment. > diff --git a/sysdeps/unix/sysv/linux/tst-rseq.c b/sysdeps/unix/sysv/linux/tst-rseq.c > new file mode 100644 > index 0000000000..48a61f9998 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/tst-rseq.c > @@ -0,0 +1,108 @@ > +/* Restartable Sequences single-threaded tests. > + > + Copyright (C) 2020 Free Software Foundation, Inc. Perhaps remove the empty line? > +#include <sys/syscall.h> > +#include <unistd.h> > +#include <stdio.h> > +#include <support/check.h> > +#include <sys/rseq.h> > + > +#ifdef RSEQ_SIG > +#include <syscall.h> Duplicate <syscall.h>. > +#include <stdlib.h> > +#include <error.h> > +#include <errno.h> > +#include <stdint.h> > +#include <string.h> > +#include <atomic.h> > + > +static int > +rseq_thread_registered (void) > +{ > + return (int32_t) atomic_load_relaxed (&__rseq_abi.cpu_id) >= 0; > +} (See above.) > +static int > +do_rseq_main_test (void) > +{ > + if (!rseq_thread_registered ()) > + { > + FAIL_RET ("rseq not registered in main thread"); > + } > + return 0; > +} Additional braces. > +static int > +sys_rseq (struct rseq *rseq_abi, uint32_t rseq_len, int flags, uint32_t sig) > +{ > + return syscall (__NR_rseq, rseq_abi, rseq_len, flags, sig); > +} > + > +static int > +rseq_available (void) > +{ > + int rc; > + > + rc = sys_rseq (NULL, 0, 0, 0); > + if (rc != -1) > + FAIL_EXIT1 ("Unexpected rseq return value %d", rc); > + switch (errno) > + { > + case ENOSYS: > + return 0; > + case EINVAL: > + return 1; > + default: > + FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno)); > + } > +} It looks these three functions could be moved in to a tst-rseq.h header file (just create the file, no Makefile updates needed). > +static int > +do_rseq_test (void) > +{ > + int result = 0; > + > + if (!rseq_available ()) > + { > + FAIL_UNSUPPORTED ("kernel does not support rseq, skipping test"); > + } > + if (do_rseq_main_test ()) > + result = 1; > + return result; > +} > +#else > +static int > +do_rseq_test (void) > +{ > +#ifndef RSEQ_SIG > + FAIL_UNSUPPORTED ("glibc does not define RSEQ_SIG, skipping test"); > +#endif > + return 0; > +} > +#endif Comments on #else and #endif, see above. C++ test could exercise the thread exit path via thread_local, without linking against libpthread. Thanks, Florian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 3/3] rseq registration tests (v10) 2020-05-20 10:52 ` Florian Weimer via Libc-alpha @ 2020-05-25 17:07 ` Mathieu Desnoyers via Libc-alpha 2020-05-26 12:47 ` Florian Weimer via Libc-alpha 0 siblings, 1 reply; 27+ messages in thread From: Mathieu Desnoyers via Libc-alpha @ 2020-05-25 17:07 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, Joseph Myers (trimming CC list) ----- On May 20, 2020, at 6:52 AM, Florian Weimer fweimer@redhat.com wrote: > * Mathieu Desnoyers via Libc-alpha: > >> - Include atomic.h from tst-rseq.c for use of atomic_load_relaxed. >> Move tst-rseq.c to internal tests within Makefile due to its use of >> atomic.h. > > You could use __atomic builtins to avoid that, or <stdatomic.h>. Both > are fine in tests. I suggest to do that, so that the test can be built > more easily outside of glibc. OK will use __atomic_load (&__rseq_abi.cpu_id, &v, __ATOMIC_RELAXED); > > However, this needs to remain an internal test because the test needs a > defintiion of __NR_rseq from the internal system call list. Regular > tests use the installed kernel headers, which may not define __NR_rseq. > Maybe mention this in a comment in nptl/Makefile, along with the > tests-internal change? OK > >> diff --git a/sysdeps/unix/sysv/linux/tst-rseq-nptl.c >> b/sysdeps/unix/sysv/linux/tst-rseq-nptl.c >> new file mode 100644 >> index 0000000000..3e3996d686 >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/tst-rseq-nptl.c >> @@ -0,0 +1,338 @@ >> +/* Restartable Sequences NPTL test. >> + >> + Copyright (C) 2020 Free Software Foundation, Inc. > > Maybe rmoeve this empty line. OK > >> +#ifdef RSEQ_SIG >> +#include <pthread.h> >> +#include <syscall.h> >> +#include <stdlib.h> >> +#include <error.h> >> +#include <errno.h> >> +#include <string.h> >> +#include <stdint.h> >> +#include <sys/types.h> >> +#include <sys/wait.h> >> +#include <signal.h> >> +#include <atomic.h> > > This should be: > > #ifdef RSEQ_SIG > # include <pthread.h> > # include <stdlib.h> > > And so on. > > Nested preprocessor conditionals need to be indented per our style > rules. OK > > Please also remove the redundant <syscall.h> inclusion. OK. I will remove the first <sys/syscall.h> include and keep the <syscall.h> include. > >> +static pthread_key_t rseq_test_key; >> + >> +static int >> +rseq_thread_registered (void) >> +{ >> + return (int32_t) atomic_load_relaxed (&__rseq_abi.cpu_id) >= 0; >> +} > > The return type should be bool. OK, and will include <stdbool.h> as well. > >> +static void >> +rseq_key_destructor (void *arg) >> +{ >> + /* Cannot use deferred failure reporting after main () returns. */ > > No () after function name. OK > >> + if (!rseq_thread_registered ()) >> + FAIL_EXIT1 ("rseq not registered in pthread key destructor"); >> +} >> + >> +static void >> +atexit_handler (void) >> +{ >> + /* Cannot use deferred failure reporting after main () returns. */ >> + if (!rseq_thread_registered ()) >> + FAIL_EXIT1 ("rseq not registered in atexit handler"); >> +} >> + >> +static int >> +do_rseq_main_test (void) >> +{ >> + if (atexit (atexit_handler)) >> + FAIL_EXIT1 ("error calling atexit"); >> + rseq_test_key = xpthread_key_create (rseq_key_destructor); >> + if (pthread_atfork (atfork_prepare, atfork_parent, atfork_child)) >> + FAIL_EXIT1 ("error calling pthread_atfork"); >> + if (raise (SIGUSR1)) >> + FAIL_EXIT1 ("error raising signal"); >> + if (pthread_setspecific (rseq_test_key, (void *) 1l)) >> + FAIL_EXIT1 ("error in pthread_setspecific"); >> + if (!rseq_thread_registered ()) >> + { >> + FAIL_RET ("rseq not registered in main thread"); >> + } > > You could use > > TEST_COMPARE (atexit (atexit_handler), 0); OK > … > TEST_VERIFY (rseq_thread_registered ()); That would change the behavior from "return 1 on error" to "continue executing on error", which is not what we want here. I think we could use TEST_VERIFY_EXIT to achieve a similar goal here. I'll change do_rseq_main_test to return void rather than int. > > from <support/check.h>. The automatically generated error messages will > provide sufficient context, I think. OK > >> + return 0; >> +} >> + >> +static void >> +cancel_routine (void *arg) >> +{ >> + if (!rseq_thread_registered ()) >> + { >> + printf ("rseq not registered in cancel routine\n"); >> + support_record_failure (); >> + } >> +} > > Please add "error: " to the error message, to make it clearer that it is > an error. OK > >> + >> +static int cancel_thread_ready; >> + >> +static void >> +test_cancel_thread (void) >> +{ >> + pthread_cleanup_push (cancel_routine, NULL); >> + atomic_store_release (&cancel_thread_ready, 1); >> + for (;;) >> + usleep (100); >> + pthread_cleanup_pop (0); >> +} > > This can use a real barrier (pthread_barrier_t), I think. No need for > polling then. OK. > >> +static void * >> +thread_function (void * arg) >> +{ >> + int i = (int) (intptr_t) arg; >> + >> + if (raise (SIGUSR1)) >> + FAIL_EXIT1 ("error raising signal"); > > This can use xraise. OK > >> + if (i == 0) >> + test_cancel_thread (); >> + if (pthread_setspecific (rseq_test_key, (void *) 1l)) >> + FAIL_EXIT1 ("error in pthread_setspecific"); > > Could use TEST_COMPARE. OK > >> + return rseq_thread_registered () ? NULL : (void *) 1l; >> +} >> + >> +static void >> +sighandler (int sig) >> +{ >> + if (!rseq_thread_registered ()) >> + { >> + printf ("rseq not registered in signal handler\n"); >> + support_record_failure (); >> + } >> +} > > Please add "error: ". OK > >> +static void >> +setup_signals (void) >> +{ >> + struct sigaction sa; >> + >> + sigemptyset (&sa.sa_mask); >> + sigaddset (&sa.sa_mask, SIGUSR1); >> + sa.sa_flags = 0; >> + sa.sa_handler = sighandler; >> + if (sigaction (SIGUSR1, &sa, NULL) != 0) >> + { >> + FAIL_EXIT1 ("sigaction failure: %s", strerror (errno)); >> + } >> +} > > This could use xsigaction. OK > >> +#define N 7 >> +static const int t[N] = { 1, 2, 6, 5, 4, 3, 50 }; > > Should these definitions be local to do_rseq_test below? You can avoid > defining N by using array_length from <array_length.h>. OK > >> +static int >> +do_rseq_threads_test (int nr_threads) >> +{ >> + pthread_t th[nr_threads]; >> + int i; >> + int result = 0; >> + >> + cancel_thread_ready = 0; >> + for (i = 0; i < nr_threads; ++i) >> + if (pthread_create (&th[i], NULL, thread_function, >> + (void *) (intptr_t) i) != 0) >> + { >> + FAIL_EXIT1 ("creation of thread %d failed", i); >> + } > > Could use xpthread_create. OK > >> + while (!atomic_load_acquire (&cancel_thread_ready)) >> + usleep (100); > > See above, could use xpthread_barrier_wait. OK > > The present code does not wait until all threads have entered their > cancellation region, so I'm not sure if the test object is actually met > here. We're only cancelling the first thread in the test, which is the intent. In terms of barrier, it's a barrier involving only 2 threads. > >> + if (pthread_cancel (th[0])) >> + FAIL_EXIT1 ("error in pthread_cancel"); > > Could use xpthread_cancel. OK > >> + >> + for (i = 0; i < nr_threads; ++i) >> + { >> + void *v; >> + if (pthread_join (th[i], &v) != 0) >> + { >> + printf ("join of thread %d failed\n", i); >> + result = 1; >> + } >> + else if (i != 0 && v != NULL) >> + { >> + printf ("join %d successful, but child failed\n", i); >> + result = 1; >> + } >> + else if (i == 0 && v == NULL) >> + { >> + printf ("join %d successful, child did not fail as expected\n", i); >> + result = 1; >> + } > > Could use xpthread_join. OK > >> +static int >> +sys_rseq (struct rseq *rseq_abi, uint32_t rseq_len, int flags, uint32_t sig) >> +{ >> + return syscall (__NR_rseq, rseq_abi, rseq_len, flags, sig); >> +} > > (This is the only remaining place where internal headers are potentially > required, I think.) OK > >> +static int >> +rseq_available (void) >> +{ >> + int rc; >> + >> + rc = sys_rseq (NULL, 0, 0, 0); >> + if (rc != -1) >> + FAIL_EXIT1 ("Unexpected rseq return value %d", rc); >> + switch (errno) >> + { >> + case ENOSYS: >> + return 0; >> + case EINVAL: >> + return 1; >> + default: >> + FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno)); >> + } >> +} > > Maybe add a comment to explain what EINVAL means in this context? For instance: /* rseq is implemented, but detected an invalid parameter. */ > >> +static int >> +do_rseq_fork_test (void) >> +{ >> + int status; >> + pid_t pid, retpid; >> + >> + pid = fork (); >> + switch (pid) >> + { >> + case 0: >> + exit (do_rseq_main_test ()); >> + case -1: >> + FAIL_EXIT1 ("Unexpected fork error %s", strerror (errno)); >> + } > > Could use xfork. OK > >> + retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)); >> + if (retpid != pid) >> + { >> + FAIL_EXIT1 ("waitpid returned %ld, expected %ld", >> + (long int) retpid, (long int) pid); >> + } > > Hmm. Is the TEMP_FAILURE_RETRY really needed? Our xwaitpid does not > have this. Then how does it deal with a signal interrupting the system call performing the waitpid (EINTR) ? I do not see WNOHANG being used. > >> + if (WEXITSTATUS (status)) >> + { >> + printf ("rseq not registered in child\n"); >> + return 1; >> + } > > This whole thing looks a lot lie support_isolate_in_subprocess from > <support/namespace.h>. I'll wait until we reach an agreement on TEMP_FAILURE_RETRY before chaning the waitpid-related code. But indeed, it looks very similar. > >> +static int >> +do_rseq_test (void) >> +{ >> + int i, result = 0; >> + >> + if (!rseq_available ()) >> + { >> + FAIL_UNSUPPORTED ("kernel does not support rseq, skipping test"); >> + } > > Braces are not needed here. OK > >> + setup_signals (); >> + if (raise (SIGUSR1)) >> + FAIL_EXIT1 ("error raising signal"); > > Could use xraise. OK > >> + if (do_rseq_main_test ()) >> + result = 1; >> + for (i = 0; i < N; i++) >> + { >> + if (do_rseq_threads_test (t[i])) >> + result = 1; >> + } > > Braces are unnecessary. OK > >> +/* Test C++ destructor called at thread and process exit. */ >> +void >> +__call_tls_dtors (void) >> +{ >> + /* Cannot use deferred failure reporting after main () returns. */ >> + if (!rseq_thread_registered ()) >> + FAIL_EXIT1 ("rseq not registered in C++ thread/process exit destructor"); >> +} > > Uhm, what is this supposed to accomplish, under the __call_tls_dtors > name in particular? I don't think this gets ever called. > > It may make sense to have a separate, smaller C++ test to cover this > (perhaps as a separate patch). Hrm, the intent was to implement __call_tls_dtors locally so it would be invoked by libc on thread/process exit, but looking deeper into stdlib/cxa_thread_atexit_impl.c I suspect the hidden _call_tls_dtors defined there will be used. Indeed, a separate C++ test for this would be better. Could be done in a follow up patch later perhaps ? > >> +#else > > This needs a comment on the same line. I think it corresponds to the > earlier “#ifdef RSEQ_SIG”. OK > >> +static int >> +do_rseq_test (void) >> +{ >> +#ifndef RSEQ_SIG >> + FAIL_UNSUPPORTED ("glibc does not define RSEQ_SIG, skipping test"); >> +#endif > > And with the comment, it should be obvious that the #ifndef is not > needed here. 8-) Indeed! :) > >> + return 0; >> +} >> +#endif > > Likewise: Add comment. OK > >> diff --git a/sysdeps/unix/sysv/linux/tst-rseq.c >> b/sysdeps/unix/sysv/linux/tst-rseq.c >> new file mode 100644 >> index 0000000000..48a61f9998 >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/tst-rseq.c >> @@ -0,0 +1,108 @@ >> +/* Restartable Sequences single-threaded tests. >> + >> + Copyright (C) 2020 Free Software Foundation, Inc. > > Perhaps remove the empty line? OK > >> +#include <sys/syscall.h> >> +#include <unistd.h> >> +#include <stdio.h> >> +#include <support/check.h> >> +#include <sys/rseq.h> >> + >> +#ifdef RSEQ_SIG >> +#include <syscall.h> > > Duplicate <syscall.h>. OK, and fixing indentation. > >> +#include <stdlib.h> >> +#include <error.h> >> +#include <errno.h> >> +#include <stdint.h> >> +#include <string.h> >> +#include <atomic.h> >> + >> +static int >> +rseq_thread_registered (void) >> +{ >> + return (int32_t) atomic_load_relaxed (&__rseq_abi.cpu_id) >= 0; >> +} > > (See above.) OK > >> +static int >> +do_rseq_main_test (void) >> +{ >> + if (!rseq_thread_registered ()) >> + { >> + FAIL_RET ("rseq not registered in main thread"); >> + } >> + return 0; >> +} > > Additional braces. Will use TEST_VERIFY_EXIT (rseq_thread_registered ()); instead. > >> +static int >> +sys_rseq (struct rseq *rseq_abi, uint32_t rseq_len, int flags, uint32_t sig) >> +{ >> + return syscall (__NR_rseq, rseq_abi, rseq_len, flags, sig); >> +} >> + >> +static int >> +rseq_available (void) >> +{ >> + int rc; >> + >> + rc = sys_rseq (NULL, 0, 0, 0); >> + if (rc != -1) >> + FAIL_EXIT1 ("Unexpected rseq return value %d", rc); >> + switch (errno) >> + { >> + case ENOSYS: >> + return 0; >> + case EINVAL: >> + return 1; >> + default: >> + FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno)); >> + } >> +} > > It looks these three functions could be moved in to a tst-rseq.h header > file (just create the file, no Makefile updates needed). OK > >> +static int >> +do_rseq_test (void) >> +{ >> + int result = 0; >> + >> + if (!rseq_available ()) >> + { >> + FAIL_UNSUPPORTED ("kernel does not support rseq, skipping test"); >> + } >> + if (do_rseq_main_test ()) >> + result = 1; >> + return result; >> +} >> +#else >> +static int >> +do_rseq_test (void) >> +{ >> +#ifndef RSEQ_SIG >> + FAIL_UNSUPPORTED ("glibc does not define RSEQ_SIG, skipping test"); >> +#endif >> + return 0; >> +} >> +#endif > > Comments on #else and #endif, see above. OK > > C++ test could exercise the thread exit path via thread_local, without > linking against libpthread. Should we keep this for a future patch ? Thanks, Mathieu > > Thanks, > Florian -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 3/3] rseq registration tests (v10) 2020-05-25 17:07 ` Mathieu Desnoyers via Libc-alpha @ 2020-05-26 12:47 ` Florian Weimer via Libc-alpha 2020-05-26 14:43 ` Mathieu Desnoyers via Libc-alpha 0 siblings, 1 reply; 27+ messages in thread From: Florian Weimer via Libc-alpha @ 2020-05-26 12:47 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: libc-alpha, Joseph Myers * Mathieu Desnoyers: >> The present code does not wait until all threads have entered their >> cancellation region, so I'm not sure if the test object is actually met >> here. > > We're only cancelling the first thread in the test, which is the intent. > In terms of barrier, it's a barrier involving only 2 threads. Huh. I need to look at the version with the real barrier. >>> +static int >>> +rseq_available (void) >>> +{ >>> + int rc; >>> + >>> + rc = sys_rseq (NULL, 0, 0, 0); >>> + if (rc != -1) >>> + FAIL_EXIT1 ("Unexpected rseq return value %d", rc); >>> + switch (errno) >>> + { >>> + case ENOSYS: >>> + return 0; >>> + case EINVAL: >>> + return 1; >>> + default: >>> + FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno)); >>> + } >>> +} >> >> Maybe add a comment to explain what EINVAL means in this context? > > For instance: > > /* rseq is implemented, but detected an invalid parameter. */ Ah, so 0 is an invalid operation? >>> + retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)); >>> + if (retpid != pid) >>> + { >>> + FAIL_EXIT1 ("waitpid returned %ld, expected %ld", >>> + (long int) retpid, (long int) pid); >>> + } >> >> Hmm. Is the TEMP_FAILURE_RETRY really needed? Our xwaitpid does not >> have this. > > Then how does it deal with a signal interrupting the system call performing > the waitpid (EINTR) ? I do not see WNOHANG being used. It obscures spurious signals. In most test cases, if an unexpected signal is delivered, something is quite wrong indeed. This is why we don't generally hide EINTR errors. >>> +/* Test C++ destructor called at thread and process exit. */ >>> +void >>> +__call_tls_dtors (void) >>> +{ >>> + /* Cannot use deferred failure reporting after main () returns. */ >>> + if (!rseq_thread_registered ()) >>> + FAIL_EXIT1 ("rseq not registered in C++ thread/process exit destructor"); >>> +} >> >> Uhm, what is this supposed to accomplish, under the __call_tls_dtors >> name in particular? I don't think this gets ever called. >> >> It may make sense to have a separate, smaller C++ test to cover this >> (perhaps as a separate patch). > > Hrm, the intent was to implement __call_tls_dtors locally so it would > be invoked by libc on thread/process exit, but looking deeper into > stdlib/cxa_thread_atexit_impl.c I suspect the hidden _call_tls_dtors > defined there will be used. Right, it's not an interposable symbol. > Indeed, a separate C++ test for this would be better. Could be done in a > follow up patch later perhaps ? Yes, let's remove this and add a real C++ test later. >> C++ test could exercise the thread exit path via thread_local, without >> linking against libpthread. > > Should we keep this for a future patch ? Yes, please. Thanks, Florian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 3/3] rseq registration tests (v10) 2020-05-26 12:47 ` Florian Weimer via Libc-alpha @ 2020-05-26 14:43 ` Mathieu Desnoyers via Libc-alpha 2020-05-27 15:05 ` Mathieu Desnoyers via Libc-alpha 2020-05-27 15:12 ` Florian Weimer via Libc-alpha 0 siblings, 2 replies; 27+ messages in thread From: Mathieu Desnoyers via Libc-alpha @ 2020-05-26 14:43 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, Joseph Myers ----- On May 26, 2020, at 8:47 AM, Florian Weimer fweimer@redhat.com wrote: > * Mathieu Desnoyers: > >>> The present code does not wait until all threads have entered their >>> cancellation region, so I'm not sure if the test object is actually met >>> here. >> >> We're only cancelling the first thread in the test, which is the intent. >> In terms of barrier, it's a barrier involving only 2 threads. > > Huh. I need to look at the version with the real barrier. Useful bits: static void cancel_routine (void *arg) { if (!rseq_thread_registered ()) { printf ("error: rseq not registered in cancel routine\n"); support_record_failure (); } } static pthread_barrier_t cancel_thread_barrier; static pthread_cond_t cancel_thread_cond = PTHREAD_COND_INITIALIZER; static pthread_mutex_t cancel_thread_mutex = PTHREAD_MUTEX_INITIALIZER; static void test_cancel_thread (void) { pthread_cleanup_push (cancel_routine, NULL); (void) xpthread_barrier_wait (&cancel_thread_barrier); /* Wait forever until cancellation. */ xpthread_cond_wait (&cancel_thread_cond, &cancel_thread_mutex); pthread_cleanup_pop (0); } static void * thread_function (void * arg) { int i = (int) (intptr_t) arg; xraise (SIGUSR1); if (i == 0) test_cancel_thread (); TEST_COMPARE (pthread_setspecific (rseq_test_key, (void *) 1l), 0); return rseq_thread_registered () ? NULL : (void *) 1l; } static int do_rseq_threads_test (int nr_threads) { pthread_t th[nr_threads]; int i; int result = 0; xpthread_barrier_init (&cancel_thread_barrier, NULL, 2); for (i = 0; i < nr_threads; ++i) th[i] = xpthread_create (NULL, thread_function, (void *) (intptr_t) i); (void) xpthread_barrier_wait (&cancel_thread_barrier); xpthread_cancel (th[0]); for (i = 0; i < nr_threads; ++i) { void *v; v = xpthread_join (th[i]); if (i != 0 && v != NULL) { printf ("error: join %d successful, but child failed\n", i); result = 1; } else if (i == 0 && v == NULL) { printf ("error: join %d successful, child did not fail as expected\n", i); result = 1; } } xpthread_barrier_destroy (&cancel_thread_barrier); return result; } > >>>> +static int >>>> +rseq_available (void) >>>> +{ >>>> + int rc; >>>> + >>>> + rc = sys_rseq (NULL, 0, 0, 0); >>>> + if (rc != -1) >>>> + FAIL_EXIT1 ("Unexpected rseq return value %d", rc); >>>> + switch (errno) >>>> + { >>>> + case ENOSYS: >>>> + return 0; >>>> + case EINVAL: >>>> + return 1; >>>> + default: >>>> + FAIL_EXIT1 ("Unexpected rseq error %s", strerror (errno)); >>>> + } >>>> +} >>> >>> Maybe add a comment to explain what EINVAL means in this context? >> >> For instance: >> >> /* rseq is implemented, but detected an invalid parameter. */ > > Ah, so 0 is an invalid operation? So the @flags parameter is 0, which is fine. It's the @rseq_len parameter being 0 (which differs from sizeof(struct rseq)) which returns -EINVAL. I will clarify this in the comment: /* rseq is implemented, but detected an invalid rseq_len parameter. */ > >>>> + retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)); >>>> + if (retpid != pid) >>>> + { >>>> + FAIL_EXIT1 ("waitpid returned %ld, expected %ld", >>>> + (long int) retpid, (long int) pid); >>>> + } >>> >>> Hmm. Is the TEMP_FAILURE_RETRY really needed? Our xwaitpid does not >>> have this. >> >> Then how does it deal with a signal interrupting the system call performing >> the waitpid (EINTR) ? I do not see WNOHANG being used. > > It obscures spurious signals. In most test cases, if an unexpected > signal is delivered, something is quite wrong indeed. This is why we > don't generally hide EINTR errors. So it means you may have trouble using tools like strace and gdb on those tests ? AFAIU those are heavy users of SIGSTOP and SIGCONT. Similarly for profilers, those usually rely on a timer-driven signal. > >>>> +/* Test C++ destructor called at thread and process exit. */ >>>> +void >>>> +__call_tls_dtors (void) >>>> +{ >>>> + /* Cannot use deferred failure reporting after main () returns. */ >>>> + if (!rseq_thread_registered ()) >>>> + FAIL_EXIT1 ("rseq not registered in C++ thread/process exit destructor"); >>>> +} >>> >>> Uhm, what is this supposed to accomplish, under the __call_tls_dtors >>> name in particular? I don't think this gets ever called. >>> >>> It may make sense to have a separate, smaller C++ test to cover this >>> (perhaps as a separate patch). >> >> Hrm, the intent was to implement __call_tls_dtors locally so it would >> be invoked by libc on thread/process exit, but looking deeper into >> stdlib/cxa_thread_atexit_impl.c I suspect the hidden _call_tls_dtors >> defined there will be used. > > Right, it's not an interposable symbol. > >> Indeed, a separate C++ test for this would be better. Could be done in a >> follow up patch later perhaps ? > > Yes, let's remove this and add a real C++ test later. OK > >>> C++ test could exercise the thread exit path via thread_local, without >>> linking against libpthread. >> >> Should we keep this for a future patch ? > > Yes, please. OK, thanks! Mathieu > > Thanks, > Florian -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 3/3] rseq registration tests (v10) 2020-05-26 14:43 ` Mathieu Desnoyers via Libc-alpha @ 2020-05-27 15:05 ` Mathieu Desnoyers via Libc-alpha 2020-05-27 15:12 ` Florian Weimer via Libc-alpha 1 sibling, 0 replies; 27+ messages in thread From: Mathieu Desnoyers via Libc-alpha @ 2020-05-27 15:05 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, Joseph Myers ----- On May 26, 2020, at 10:43 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote: > ----- On May 26, 2020, at 8:47 AM, Florian Weimer fweimer@redhat.com wrote: > >> * Mathieu Desnoyers: [...] The one question below is the last thing we need to complete discussing before I can re-spin another version of the patches: >> >>>>> + retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)); >>>>> + if (retpid != pid) >>>>> + { >>>>> + FAIL_EXIT1 ("waitpid returned %ld, expected %ld", >>>>> + (long int) retpid, (long int) pid); >>>>> + } >>>> >>>> Hmm. Is the TEMP_FAILURE_RETRY really needed? Our xwaitpid does not >>>> have this. >>> >>> Then how does it deal with a signal interrupting the system call performing >>> the waitpid (EINTR) ? I do not see WNOHANG being used. >> >> It obscures spurious signals. In most test cases, if an unexpected >> signal is delivered, something is quite wrong indeed. This is why we >> don't generally hide EINTR errors. > > So it means you may have trouble using tools like strace and gdb on those > tests ? AFAIU those are heavy users of SIGSTOP and SIGCONT. Similarly for > profilers, those usually rely on a timer-driven signal. > Should I use xwaitpid instead, and should xwaitpid be fixed to use TEMP_FAILURE_RETRY ? Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 3/3] rseq registration tests (v10) 2020-05-26 14:43 ` Mathieu Desnoyers via Libc-alpha 2020-05-27 15:05 ` Mathieu Desnoyers via Libc-alpha @ 2020-05-27 15:12 ` Florian Weimer via Libc-alpha 2020-05-27 15:17 ` Mathieu Desnoyers via Libc-alpha 1 sibling, 1 reply; 27+ messages in thread From: Florian Weimer via Libc-alpha @ 2020-05-27 15:12 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: libc-alpha, Joseph Myers * Mathieu Desnoyers: >>>>> + retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)); >>>>> + if (retpid != pid) >>>>> + { >>>>> + FAIL_EXIT1 ("waitpid returned %ld, expected %ld", >>>>> + (long int) retpid, (long int) pid); >>>>> + } >>>> >>>> Hmm. Is the TEMP_FAILURE_RETRY really needed? Our xwaitpid does not >>>> have this. >>> >>> Then how does it deal with a signal interrupting the system call performing >>> the waitpid (EINTR) ? I do not see WNOHANG being used. >> >> It obscures spurious signals. In most test cases, if an unexpected >> signal is delivered, something is quite wrong indeed. This is why we >> don't generally hide EINTR errors. > > So it means you may have trouble using tools like strace and gdb on those > tests ? AFAIU those are heavy users of SIGSTOP and SIGCONT. Similarly for > profilers, those usually rely on a timer-driven signal. I have never seen any problems with strace due to this. ptrace has become a bit more transparent to the tracee since the early days, I think. I haven't seen problems under GDB, either, but then tests that fork can be rather annoying to debug anyway. Thanks, Florian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 3/3] rseq registration tests (v10) 2020-05-27 15:12 ` Florian Weimer via Libc-alpha @ 2020-05-27 15:17 ` Mathieu Desnoyers via Libc-alpha 2020-05-27 15:21 ` Florian Weimer via Libc-alpha 0 siblings, 1 reply; 27+ messages in thread From: Mathieu Desnoyers via Libc-alpha @ 2020-05-27 15:17 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, Joseph Myers ----- On May 27, 2020, at 11:12 AM, Florian Weimer fweimer@redhat.com wrote: > * Mathieu Desnoyers: > >>>>>> + retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)); >>>>>> + if (retpid != pid) >>>>>> + { >>>>>> + FAIL_EXIT1 ("waitpid returned %ld, expected %ld", >>>>>> + (long int) retpid, (long int) pid); >>>>>> + } >>>>> >>>>> Hmm. Is the TEMP_FAILURE_RETRY really needed? Our xwaitpid does not >>>>> have this. >>>> >>>> Then how does it deal with a signal interrupting the system call performing >>>> the waitpid (EINTR) ? I do not see WNOHANG being used. >>> >>> It obscures spurious signals. In most test cases, if an unexpected >>> signal is delivered, something is quite wrong indeed. This is why we >>> don't generally hide EINTR errors. >> >> So it means you may have trouble using tools like strace and gdb on those >> tests ? AFAIU those are heavy users of SIGSTOP and SIGCONT. Similarly for >> profilers, those usually rely on a timer-driven signal. > > I have never seen any problems with strace due to this. ptrace has > become a bit more transparent to the tracee since the early days, I > think. > > I haven't seen problems under GDB, either, but then tests that fork can > be rather annoying to debug anyway. OK so I'll use the xwaitpid wrapper and let this be someone else's problem. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 3/3] rseq registration tests (v10) 2020-05-27 15:17 ` Mathieu Desnoyers via Libc-alpha @ 2020-05-27 15:21 ` Florian Weimer via Libc-alpha 2020-05-27 15:30 ` Mathieu Desnoyers via Libc-alpha 0 siblings, 1 reply; 27+ messages in thread From: Florian Weimer via Libc-alpha @ 2020-05-27 15:21 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: libc-alpha, Joseph Myers * Mathieu Desnoyers: > ----- On May 27, 2020, at 11:12 AM, Florian Weimer fweimer@redhat.com wrote: > >> * Mathieu Desnoyers: >> >>>>>>> + retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)); >>>>>>> + if (retpid != pid) >>>>>>> + { >>>>>>> + FAIL_EXIT1 ("waitpid returned %ld, expected %ld", >>>>>>> + (long int) retpid, (long int) pid); >>>>>>> + } >>>>>> >>>>>> Hmm. Is the TEMP_FAILURE_RETRY really needed? Our xwaitpid does not >>>>>> have this. >>>>> >>>>> Then how does it deal with a signal interrupting the system call performing >>>>> the waitpid (EINTR) ? I do not see WNOHANG being used. >>>> >>>> It obscures spurious signals. In most test cases, if an unexpected >>>> signal is delivered, something is quite wrong indeed. This is why we >>>> don't generally hide EINTR errors. >>> >>> So it means you may have trouble using tools like strace and gdb on those >>> tests ? AFAIU those are heavy users of SIGSTOP and SIGCONT. Similarly for >>> profilers, those usually rely on a timer-driven signal. >> >> I have never seen any problems with strace due to this. ptrace has >> become a bit more transparent to the tracee since the early days, I >> think. >> >> I haven't seen problems under GDB, either, but then tests that fork can >> be rather annoying to debug anyway. > > OK so I'll use the xwaitpid wrapper and let this be someone else's problem. Yes please. Or support_isolate_in_subprocess. Thanks, Florian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 3/3] rseq registration tests (v10) 2020-05-27 15:21 ` Florian Weimer via Libc-alpha @ 2020-05-27 15:30 ` Mathieu Desnoyers via Libc-alpha 0 siblings, 0 replies; 27+ messages in thread From: Mathieu Desnoyers via Libc-alpha @ 2020-05-27 15:30 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, Joseph Myers ----- On May 27, 2020, at 11:21 AM, Florian Weimer fweimer@redhat.com wrote: > * Mathieu Desnoyers: > >> ----- On May 27, 2020, at 11:12 AM, Florian Weimer fweimer@redhat.com wrote: >> >>> * Mathieu Desnoyers: >>> >>>>>>>> + retpid = TEMP_FAILURE_RETRY (waitpid (pid, &status, 0)); >>>>>>>> + if (retpid != pid) >>>>>>>> + { >>>>>>>> + FAIL_EXIT1 ("waitpid returned %ld, expected %ld", >>>>>>>> + (long int) retpid, (long int) pid); >>>>>>>> + } >>>>>>> >>>>>>> Hmm. Is the TEMP_FAILURE_RETRY really needed? Our xwaitpid does not >>>>>>> have this. >>>>>> >>>>>> Then how does it deal with a signal interrupting the system call performing >>>>>> the waitpid (EINTR) ? I do not see WNOHANG being used. >>>>> >>>>> It obscures spurious signals. In most test cases, if an unexpected >>>>> signal is delivered, something is quite wrong indeed. This is why we >>>>> don't generally hide EINTR errors. >>>> >>>> So it means you may have trouble using tools like strace and gdb on those >>>> tests ? AFAIU those are heavy users of SIGSTOP and SIGCONT. Similarly for >>>> profilers, those usually rely on a timer-driven signal. >>> >>> I have never seen any problems with strace due to this. ptrace has >>> become a bit more transparent to the tracee since the early days, I >>> think. >>> >>> I haven't seen problems under GDB, either, but then tests that fork can >>> be rather annoying to debug anyway. >> >> OK so I'll use the xwaitpid wrapper and let this be someone else's problem. > > Yes please. Or support_isolate_in_subprocess. Right, even better, thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 0/3] Restartable Sequences enablement 2020-05-01 2:14 [PATCH glibc 0/3] Restartable Sequences enablement Mathieu Desnoyers via Libc-alpha ` (2 preceding siblings ...) 2020-05-01 2:14 ` [PATCH glibc 3/3] rseq registration tests (v10) Mathieu Desnoyers via Libc-alpha @ 2020-05-20 11:44 ` Florian Weimer via Libc-alpha 2020-05-25 13:52 ` Mathieu Desnoyers via Libc-alpha 3 siblings, 1 reply; 27+ messages in thread From: Florian Weimer via Libc-alpha @ 2020-05-20 11:44 UTC (permalink / raw) To: Mathieu Desnoyers via Libc-alpha; +Cc: Mathieu Desnoyers, Joseph Myers * Mathieu Desnoyers via Libc-alpha: > Florian, if you find the patches OK and possibly with a bit of > tweaking of the commit messages, can you please commit them > to glibc ? I had some final review comments, and I found a real bug: the uptr changes in <sys/rseq.h> are problematic. What do others think about the level of testing and documentation in these patches? Thanks, Florian ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 0/3] Restartable Sequences enablement 2020-05-20 11:44 ` [PATCH glibc 0/3] Restartable Sequences enablement Florian Weimer via Libc-alpha @ 2020-05-25 13:52 ` Mathieu Desnoyers via Libc-alpha 2020-05-25 14:28 ` Florian Weimer via Libc-alpha 0 siblings, 1 reply; 27+ messages in thread From: Mathieu Desnoyers via Libc-alpha @ 2020-05-25 13:52 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, Joseph Myers ----- On May 20, 2020, at 7:44 AM, Florian Weimer fweimer@redhat.com wrote: > * Mathieu Desnoyers via Libc-alpha: > >> Florian, if you find the patches OK and possibly with a bit of >> tweaking of the commit messages, can you please commit them >> to glibc ? > > I had some final review comments, and I found a real bug: the uptr > changes in <sys/rseq.h> are problematic. Hi Florian, Can you share what issue you have found with the uptr changes ? Thanks, Mathieu > > What do others think about the level of testing and documentation in > these patches? > > Thanks, > Florian -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH glibc 0/3] Restartable Sequences enablement 2020-05-25 13:52 ` Mathieu Desnoyers via Libc-alpha @ 2020-05-25 14:28 ` Florian Weimer via Libc-alpha 0 siblings, 0 replies; 27+ messages in thread From: Florian Weimer via Libc-alpha @ 2020-05-25 14:28 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: libc-alpha, Joseph Myers * Mathieu Desnoyers: > ----- On May 20, 2020, at 7:44 AM, Florian Weimer fweimer@redhat.com wrote: > >> * Mathieu Desnoyers via Libc-alpha: >> >>> Florian, if you find the patches OK and possibly with a bit of >>> tweaking of the commit messages, can you please commit them >>> to glibc ? >> >> I had some final review comments, and I found a real bug: the uptr >> changes in <sys/rseq.h> are problematic. > > Hi Florian, > > Can you share what issue you have found with the uptr changes ? I thought I had written about something here: <https://sourceware.org/pipermail/libc-alpha/2020-May/114142.html> But it's not very explicit. I must have deleted it before sending it. The problem is that you made substantial changes to the fallback definitions compared to the kernel version. But those will never be used if the user has the appropriate kernel headers installed. The fallback part really has to be an almost-verbatim copy of the kernel definitions. Thanks, Florian ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2020-05-27 15:30 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-01 2:14 [PATCH glibc 0/3] Restartable Sequences enablement Mathieu Desnoyers via Libc-alpha 2020-05-01 2:14 ` [PATCH glibc 1/3] glibc: Perform rseq registration at C startup and thread creation (v19) Mathieu Desnoyers via Libc-alpha 2020-05-20 11:40 ` Florian Weimer via Libc-alpha 2020-05-25 14:51 ` Mathieu Desnoyers via Libc-alpha 2020-05-25 15:20 ` Florian Weimer via Libc-alpha 2020-05-25 17:36 ` Mathieu Desnoyers via Libc-alpha 2020-05-26 12:41 ` Florian Weimer via Libc-alpha 2020-05-26 14:32 ` Mathieu Desnoyers via Libc-alpha 2020-05-26 14:38 ` Florian Weimer via Libc-alpha 2020-05-26 14:53 ` Mathieu Desnoyers via Libc-alpha 2020-05-26 14:57 ` Florian Weimer via Libc-alpha 2020-05-26 15:22 ` Mathieu Desnoyers via Libc-alpha 2020-05-01 2:14 ` [PATCH glibc 2/3] glibc: sched_getcpu(): use rseq cpu_id TLS on Linux (v7) Mathieu Desnoyers via Libc-alpha 2020-05-20 10:14 ` Florian Weimer via Libc-alpha 2020-05-01 2:14 ` [PATCH glibc 3/3] rseq registration tests (v10) Mathieu Desnoyers via Libc-alpha 2020-05-20 10:52 ` Florian Weimer via Libc-alpha 2020-05-25 17:07 ` Mathieu Desnoyers via Libc-alpha 2020-05-26 12:47 ` Florian Weimer via Libc-alpha 2020-05-26 14:43 ` Mathieu Desnoyers via Libc-alpha 2020-05-27 15:05 ` Mathieu Desnoyers via Libc-alpha 2020-05-27 15:12 ` Florian Weimer via Libc-alpha 2020-05-27 15:17 ` Mathieu Desnoyers via Libc-alpha 2020-05-27 15:21 ` Florian Weimer via Libc-alpha 2020-05-27 15:30 ` Mathieu Desnoyers via Libc-alpha 2020-05-20 11:44 ` [PATCH glibc 0/3] Restartable Sequences enablement Florian Weimer via Libc-alpha 2020-05-25 13:52 ` Mathieu Desnoyers via Libc-alpha 2020-05-25 14:28 ` Florian Weimer via Libc-alpha
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).