Hi Thomas,
On Fri, 2025-08-01 at 17:09 +0200, Thomas Weißschuh wrote:
> On 2025-07-31 22:12:25+0200, Benjamin Berg wrote:
> > From: Benjamin Berg <[email protected]>
> >
> > Add support for sigaction() using the rt_sigaction syscall and implement
> > the normal sa_mask helpers.
> >
> > For the uapi definitions, everything is copied into nolibc. This avoids
> > issues with kernel architecture headers that are not usable with the
> > rt_sigaction syscall.
> >
> > Signed-off-by: Benjamin Berg <[email protected]>
> >
> > ---
> >
> > v3:
> > - put everything into signal.h and the new asm-signal.h
>
> Hm, did we decide on that? We don't want the per-architecture include
> dance, but static overrides should still be fine I think.
> Keeping the architecture ifdeffery inside the respective arch header.
> And all the generic stuff in a shared header.
I probably just didn't really understand what you meant :-)
You are right, we can have the common definitions in signal.h and just
skip them if the architecture header did already define them.
I think I'll also drop asm-signal.h again, see below.
> > - split out sigset_t tests
> > - actually mark signal_check static
> > - remove unused string.h include
> > - fix SIGUSR2 reset
> > - Use integer for signal_check as the signals are emitted from the
> > syscall context.
>
> I don't understand this point, isn't it a signal handler?
My reasoning is, that the signal emission by the kernel happens from
the kill syscall or function return. Both cases implicitly act as a
memory barrier. So in this specific case we do not actually need an
atomic variable.
> > v2:
> > - Use newly added macros to check signal emission order
> > - Add tests for sigset handling
> > - Restore the default handler after signal test
> > - make signal_check variable static
> >
> > v1:
> > - Update architecture support (adding sh)
> > - Move sparc sys_rt_sigaction logic into its header
> > - Add sig_atomic_t
> > - Use new BITSET_* macros
> > - Move test into syscall suite
> > - Various other small changes
> > ---
> > tools/include/nolibc/Makefile | 1 +
> > tools/include/nolibc/arch-s390.h | 4 +-
> > tools/include/nolibc/asm-signal.h | 237 +++++++++++++++++++
> > tools/include/nolibc/signal.h | 179 ++++++++++++++
> > tools/include/nolibc/sys.h | 2 +-
> > tools/include/nolibc/sys/wait.h | 1 +
> > tools/include/nolibc/time.h | 2 +-
> > tools/include/nolibc/types.h | 9 +
> > tools/testing/selftests/nolibc/nolibc-test.c | 134 +++++++++++
> > 9 files changed, 566 insertions(+), 3 deletions(-)
> > create mode 100644 tools/include/nolibc/asm-signal.h
>
> (...)
>
> > diff --git a/tools/include/nolibc/sys.h b/tools/include/nolibc/sys.h
> > index 295e71d34aba..a790e816565b 100644
> > --- a/tools/include/nolibc/sys.h
> > +++ b/tools/include/nolibc/sys.h
> > @@ -14,7 +14,6 @@
> >
> > /* system includes */
> > #include <linux/unistd.h>
> > -#include <linux/signal.h> /* for SIGCHLD */
> > #include <linux/termios.h>
> > #include <linux/mman.h>
> > #include <linux/fs.h>
> > @@ -28,6 +27,7 @@
> > #include "errno.h"
> > #include "stdarg.h"
> > #include "types.h"
> > +#include "asm-signal.h" /* for SIGCHLD */
>
> #include "signal.h"
Right, this and asm-signal.h happened because signal.h uses sys_kill()
for raise(), resulting in a circular dependency.
The simplest solution is probably to avoid the circular include by
implementing raise() as:
int raise(int signal);
__attribute__((weak,unused,section(".text.nolibc_raise")))
int raise(int signal)
{
return my_syscall2(__NR_kill, my_syscall0(__NR_getpid), signal);
}
> > /* Syscall return helper: takes the syscall value in argument and checks
> > for an
> > diff --git a/tools/include/nolibc/sys/wait.h
> > b/tools/include/nolibc/sys/wait.h
> > index 56ddb806da7f..e2aa90cc3cf3 100644
> > --- a/tools/include/nolibc/sys/wait.h
> > +++ b/tools/include/nolibc/sys/wait.h
> > @@ -10,6 +10,7 @@
> > #ifndef _NOLIBC_SYS_WAIT_H
> > #define _NOLIBC_SYS_WAIT_H
> >
> > +#include <asm/siginfo.h>
>
> #include "signal.h"
>
> The asm/ usage should be hidden.
>
> > #include "../arch.h"
> > #include "../std.h"
> > #include "../types.h"
>
> (...)
>
> > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c
> > b/tools/testing/selftests/nolibc/nolibc-test.c
> > index 180f0436127a..75b96eaa4c65 100644
> > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > @@ -1269,6 +1269,138 @@ int test_namespace(void)
> > return ret;
> > }
> >
> > +int test_sigset_t(int test_idx)
> > +{
> > + int llen;
> > + int ret = 0;
> > +
> > +#ifdef NOLIBC
> > + if (is_nolibc) {
>
> This looks unnecessary. The #ifdef should be sufficient.
>
> > + sigset_t sigset;
> > +
>
> (...)
>
>
> Looks nice, thanks!
Great, thanks for the reviews!
Benjamin
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
