| Message ID | 20191210183221.26912-1-adhemerval.zanella@linaro.org |
|---|---|
| State | Accepted |
| Commit | 1b132d55e2d3a4eb421c0f77f63b67b5022c22e3 |
| Headers | show |
| Series | [v3,1/7] Fix __libc_signal_block_all on sparc64 | expand |
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. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
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. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
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.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
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. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
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. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
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."). -- Joseph S. Myers joseph@codesourcery.com
diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h index 01d8bf0a4c..a496c7174c 100644 --- a/sysdeps/unix/sysv/linux/internal-signals.h +++ b/sysdeps/unix/sysv/linux/internal-signals.h @@ -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. */
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(-) -- 2.17.1