diff mbox series

[v3,1/7] Fix __libc_signal_block_all on sparc64

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

Commit Message

Adhemerval Zanella Netto Dec. 10, 2019, 6:32 p.m. UTC
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

Comments

Andreas Schwab Dec. 11, 2019, 8:51 a.m. UTC | #1
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."
Adhemerval Zanella Netto Dec. 11, 2019, 12:50 p.m. UTC | #2
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).
Andreas Schwab Dec. 11, 2019, 2:06 p.m. UTC | #3
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."
Adhemerval Zanella Netto Dec. 12, 2019, 4:15 p.m. UTC | #4
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).
Andreas Schwab Dec. 12, 2019, 4:30 p.m. UTC | #5
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."
Adhemerval Zanella Netto Dec. 12, 2019, 4:38 p.m. UTC | #6
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?
Andreas Schwab Dec. 12, 2019, 4:53 p.m. UTC | #7
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."
Adhemerval Zanella Netto Dec. 12, 2019, 4:57 p.m. UTC | #8
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?
Andreas Schwab Dec. 12, 2019, 5 p.m. UTC | #9
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."
Adhemerval Zanella Netto Dec. 12, 2019, 5:01 p.m. UTC | #10
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.
Joseph Myers Dec. 12, 2019, 6 p.m. UTC | #11
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 mbox series

Patch

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.  */