[v3,1/7] Fix __libc_signal_block_all on sparc64
Commit Message
Changes from previous version:
- Remove unrelated changes.
--
The a2e8aa0d9e shows two regressions on sparc64-linux-gnu:
nptl/tst-cancel-self-canceltype
nptl/tst-cancel5
This is not from the patch itself, but rather from an invalid
__NR_rt_sigprocmask issued by the loader:
rt_sigprocmask(SIG_UNBLOCK, [RTMIN RT_1], NULL, 8) = 0
rt_sigprocmask(0xffd07c60 /* SIG_??? */, ~[], 0x7feffd07d08, 8) = -1 EINVAL (Invalid argument)
Tracking the culprit it really seems a wrong code generation in the
INTERNAL_SYSCALL due the automatic sigset_t used on
__libc_signal_block_all:
return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
set, _NSIG / 8);
Where SIGALL_SET is defined as:
((__sigset_t) { .__val = {[0 ... _SIGSET_NWORDS-1 ] = -1 } })
Building the expanded __libc_signal_block_all on sparc64 with recent
compiler (gcc 8.3.1 and 9.1.1):
#include <signal>
int
_libc_signal_block_all (sigset_t *set)
{
INTERNAL_SYSCALL_DECL (err);
return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
set, _NSIG / 8);
}
It seems that the first argument (SIG_BLOCK) is not correctly set on
'o0' register:
__libc_signal_block_all:
save %sp, -304, %sp
add %fp, 1919, %o0
mov 128, %o2
sethi %hi(.LC0), %o1
call memcpy, 0
or %o1, %lo(.LC0), %o1
add %fp, 1919, %o1
mov %i0, %o2
mov 8, %o3
mov 103, %g1
ta 0x6d;
bcc,pt %xcc, 1f
mov 0, %g1
sub %g0, %o0, %o0
mov 1, %g1
1: sra %o0, 0, %i0
return %i7+8
nop
Where is I define SIGALL_SET outside INTERNAL_SYSCALL macro, gcc
correctly sets the expected kernel argument in correct register:
sethi %hi(.LC0), %o1
call memcpy, 0
or %o1, %lo(.LC0), %o1
-> mov 1, %o0
add %fp, 1919, %o1
This patch fixes it by moving both sigset_t that represent all signals
sets and the application set to constant data objects.
Checked on x86_64-linux-gnu, i686-linux-gnu, and sparc64-linux-gnu.
---
sysdeps/unix/sysv/linux/internal-signals.h | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
Comments
On Dez 10 2019, Adhemerval Zanella wrote:
> @@ -53,15 +54,16 @@ __clear_internal_signals (sigset_t *set)
> __sigdelset (set, SIGSETXID);
> }
>
> -#define SIGALL_SET \
> - ((__sigset_t) { .__val = {[0 ... _SIGSET_NWORDS-1 ] = -1 } })
> +static const sigset_t sigall_set = {
> + .__val = {[0 ... _SIGSET_NWORDS-1 ] = -1 }
> +};
Why do you need that static object? Doesn't it suffice to make the
compound literal const?
Andreas.
On 11/12/2019 05:51, Andreas Schwab wrote:
> On Dez 10 2019, Adhemerval Zanella wrote:
>
>> @@ -53,15 +54,16 @@ __clear_internal_signals (sigset_t *set)
>> __sigdelset (set, SIGSETXID);
>> }
>>
>> -#define SIGALL_SET \
>> - ((__sigset_t) { .__val = {[0 ... _SIGSET_NWORDS-1 ] = -1 } })
>> +static const sigset_t sigall_set = {
>> + .__val = {[0 ... _SIGSET_NWORDS-1 ] = -1 }
>> +};
>
> Why do you need that static object? Doesn't it suffice to make the
> compound literal const?
It is a suggestion from Florian to use less stack usage since the
gcc with compound literal materialize the object on the stack; and
slight compat code on some architecture (where coping the compiler
create compound object might incur in a memcpy call).
On Dez 11 2019, Adhemerval Zanella wrote:
> It is a suggestion from Florian to use less stack usage since the
> gcc with compound literal materialize the object on the stack; and
> slight compat code on some architecture (where coping the compiler
> create compound object might incur in a memcpy call).
It shouldn't do that for a const literal.
Andreas.
On 11/12/2019 11:06, Andreas Schwab wrote:
> On Dez 11 2019, Adhemerval Zanella wrote:
>
>> It is a suggestion from Florian to use less stack usage since the
>> gcc with compound literal materialize the object on the stack; and
>> slight compat code on some architecture (where coping the compiler
>> create compound object might incur in a memcpy call).
>
> It shouldn't do that for a const literal.
Well I am seeing such behaviour with gcc 9.2.1 on x86_64 at least:
$ cat sigmask.c
#include <signal.h>
#ifdef COMPOUND
#define sigall \
((const __sigset_t) { .__val = {[0 ... _SIGSET_NWORDS-1 ] = -1 } })
#else
static const sigset_t sigall = {
.__val = {[0 ... _SIGSET_NWORDS-1 ] = -1 }
};
#endif
int foo (void)
{
return sigprocmask (SIG_BLOCK, &sigall, 0);
}
$ x86_64-glibc-linux-gnu-gcc -O2 -std=gnu11 sigmask.c -S -o -
[...]
foo:
.LFB0:
.cfi_startproc
xorl %edx, %edx
movl $sigall, %esi
xorl %edi, %edi
jmp sigprocmask
[...]
$ x86_64-glibc-linux-gnu-gcc -O2 -std=gnu11 sigmask.c -S -o - -DCOMPOUND
[...]foo:
.LFB0:
.cfi_startproc
subq $136, %rsp
.cfi_def_cfa_offset 144
xorl %edx, %edx
xorl %edi, %edi
movq %rsp, %rsi
movq $-1, (%rsp)
[...]
call sigprocmask
addq $136, %rsp
[...]
Do you consider this a blocker? Should we use the compound literal? The
advantage of the static global is it slighter easier to define different
mask for the different ABI (64-bit, 32-bit, and mips with its outlier
number of signals).
On Dez 12 2019, Adhemerval Zanella wrote:
> Well I am seeing such behaviour with gcc 9.2.1 on x86_64 at least:
That's a missed optimisation bug in gcc then. There should not be a
difference between a const compound literal and a static const object,
if only constant expressions are used for initialisation.
Andreas.
On 12/12/2019 13:30, Andreas Schwab wrote:
> On Dez 12 2019, Adhemerval Zanella wrote:
>
>> Well I am seeing such behaviour with gcc 9.2.1 on x86_64 at least:
>
> That's a missed optimisation bug in gcc then. There should not be a
> difference between a const compound literal and a static const object,
> if only constant expressions are used for initialisation.
>
> Andreas.
>
Yes, but it does not answer my previous question: is this a block for
the patch?
On Dez 12 2019, Adhemerval Zanella wrote:
> Yes, but it does not answer my previous question: is this a block for
> the patch?
If that doesn't help fixing the bug then be it. But a gcc bug would be
in order anyway.
Andreas.
On 12/12/2019 13:53, Andreas Schwab wrote:
> On Dez 12 2019, Adhemerval Zanella wrote:
>
>> Yes, but it does not answer my previous question: is this a block for
>> the patch?
>
> If that doesn't help fixing the bug then be it. But a gcc bug would be
> in order anyway.
I will open a gcc bug to track it, is this patch ok with an associated
bug report on gcc?
Perhaps add a comment why a compound literal cannot be used. Ok with
that change.
Andreas.
On 12/12/2019 14:00, Andreas Schwab wrote:
> Perhaps add a comment why a compound literal cannot be used. Ok with
> that change.
>
Ack, I will do it.
On Thu, 12 Dec 2019, Andreas Schwab wrote:
> On Dez 12 2019, Adhemerval Zanella wrote:
>
> > Well I am seeing such behaviour with gcc 9.2.1 on x86_64 at least:
>
> That's a missed optimisation bug in gcc then. There should not be a
> difference between a const compound literal and a static const object,
> if only constant expressions are used for initialisation.
There's a note at the bottom of https://gcc.gnu.org/c99status.html
specifically about this ("const-qualified compound literals could share
storage with each other and with string literals, but currently don't.").
@@ -22,6 +22,7 @@
#include <signal.h>
#include <sigsetops.h>
#include <stdbool.h>
+#include <limits.h>
#include <sysdep.h>
/* The signal used for asynchronous cancelation. */
@@ -53,15 +54,16 @@ __clear_internal_signals (sigset_t *set)
__sigdelset (set, SIGSETXID);
}
-#define SIGALL_SET \
- ((__sigset_t) { .__val = {[0 ... _SIGSET_NWORDS-1 ] = -1 } })
+static const sigset_t sigall_set = {
+ .__val = {[0 ... _SIGSET_NWORDS-1 ] = -1 }
+};
/* Block all signals, including internal glibc ones. */
static inline int
__libc_signal_block_all (sigset_t *set)
{
INTERNAL_SYSCALL_DECL (err);
- return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET,
+ return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &sigall_set,
set, _NSIG / 8);
}
@@ -69,11 +71,11 @@ __libc_signal_block_all (sigset_t *set)
static inline int
__libc_signal_block_app (sigset_t *set)
{
- sigset_t allset = SIGALL_SET;
+ sigset_t allset = sigall_set;
__clear_internal_signals (&allset);
INTERNAL_SYSCALL_DECL (err);
- return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &allset, set,
- _NSIG / 8);
+ return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &allset,
+ set, _NSIG / 8);
}
/* Restore current process signal mask. */