signal: Use correct type for si_band in siginfo_t [BZ #23562]
diff mbox series

Message ID 877eifgpn7.fsf@oldenburg.str.redhat.com
State New
Headers show
Series
  • signal: Use correct type for si_band in siginfo_t [BZ #23562]
Related show

Commit Message

Florian Weimer Oct. 18, 2018, 4:56 p.m. UTC
Perhaps I a SPARC maintainer could review this as well?

I tested it on x86-64 and found no regressions.  Based on the sources,
the change should only impact SPARC.

2018-10-18  Ilya Yu. Malakhov  <malakhov@mcst.ru>

	[BZ #23562]
	* sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
	(struct siginfo_t): Use correct type for si_band.

Comments

Adhemerval Zanella Oct. 18, 2018, 6:10 p.m. UTC | #1
On 18/10/2018 13:56, Florian Weimer wrote:
> Perhaps I a SPARC maintainer could review this as well?
> 
> I tested it on x86-64 and found no regressions.  Based on the sources,
> the change should only impact SPARC.
> 
> 2018-10-18  Ilya Yu. Malakhov  <malakhov@mcst.ru>
> 
> 	[BZ #23562]
> 	* sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
> 	(struct siginfo_t): Use correct type for si_band.
> 

LGTM, it seems to match kernel definition:

include/uapi/asm-generic/siginfo.h

120                 /* SIGPOLL */
121                 struct {
122                         __ARCH_SI_BAND_T _band; /* POLL_IN, POLL_OUT, POLL_MSG */
123                         int _fd;
124                 } _sigpoll;
				

arch/sparc/include/uapi/asm/siginfo.h

  8 #define __ARCH_SI_BAND_T int


> diff --git a/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h b/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
> index 33766d1813..43c4e009a4 100644
> --- a/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
> +++ b/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
> @@ -107,7 +107,7 @@ typedef struct
>  	/* SIGPOLL.  */
>  	struct
>  	  {
> -	    long int si_band;	/* Band event for SIGPOLL.  */
> +	    __SI_BAND_TYPE si_band;	/* Band event for SIGPOLL.  */
>  	    int si_fd;
>  	  } _sigpoll;
>  
>
Joseph Myers Oct. 22, 2018, 3:07 p.m. UTC | #2
This has broken the conform/ tests for SPARC, so that you now get:

FAIL: conform/POSIX2008/signal.h/conform
FAIL: conform/POSIX2008/sys/wait.h/conform
FAIL: conform/UNIX98/signal.h/conform
FAIL: conform/UNIX98/sys/wait.h/conform
FAIL: conform/XOPEN2K/signal.h/conform
FAIL: conform/XOPEN2K/sys/wait.h/conform
FAIL: conform/XOPEN2K8/signal.h/conform
FAIL: conform/XOPEN2K8/sys/wait.h/conform
FAIL: conform/XPG42/signal.h/conform
FAIL: conform/XPG42/sys/wait.h/conform

because si_band no longer has the POSIX-specified type.

For 32-bit SPARC you should be able to use long int and so avoid the 
failures, as that won't change the layout.  For 64-bit SPARC this would be 
a case where the kernel having the wrong type means you need an 
appropriately conditioned and commented XFAIL in signal.h-data and 
sys/wait.h-data (and appropriately conditioned and commented 
conformtest-xfail-conds definition in a relevant sysdeps Makefile to 
define the condition that's used in the conform/ data changes).
Florian Weimer Oct. 22, 2018, 3:18 p.m. UTC | #3
* Joseph Myers:

> This has broken the conform/ tests for SPARC, so that you now get:
>
> FAIL: conform/POSIX2008/signal.h/conform
> FAIL: conform/POSIX2008/sys/wait.h/conform
> FAIL: conform/UNIX98/signal.h/conform
> FAIL: conform/UNIX98/sys/wait.h/conform
> FAIL: conform/XOPEN2K/signal.h/conform
> FAIL: conform/XOPEN2K/sys/wait.h/conform
> FAIL: conform/XOPEN2K8/signal.h/conform
> FAIL: conform/XOPEN2K8/sys/wait.h/conform
> FAIL: conform/XPG42/signal.h/conform
> FAIL: conform/XPG42/sys/wait.h/conform
>
> because si_band no longer has the POSIX-specified type.

Sorry about that.

> For 32-bit SPARC you should be able to use long int and so avoid the 
> failures, as that won't change the layout.

Okay, that makes sense.

> For 64-bit SPARC this would be a case where the kernel having the
> wrong type means you need an appropriately conditioned and commented
> XFAIL in signal.h-data and sys/wait.h-data (and appropriately
> conditioned and commented conformtest-xfail-conds definition in a
> relevant sysdeps Makefile to define the condition that's used in the
> conform/ data changes).

Why wasn't this a problem before
sysdeps/unix/sysv/linux/bits/types/siginfo_t.h was split out?  SPARC
clearly had int as the type of si_band back then.

Thanks,
Florian
Joseph Myers Oct. 22, 2018, 3:46 p.m. UTC | #4
On Mon, 22 Oct 2018, Florian Weimer wrote:

> > For 64-bit SPARC this would be a case where the kernel having the
> > wrong type means you need an appropriately conditioned and commented
> > XFAIL in signal.h-data and sys/wait.h-data (and appropriately
> > conditioned and commented conformtest-xfail-conds definition in a
> > relevant sysdeps Makefile to define the condition that's used in the
> > conform/ data changes).
> 
> Why wasn't this a problem before
> sysdeps/unix/sysv/linux/bits/types/siginfo_t.h was split out?  SPARC
> clearly had int as the type of si_band back then.

XFAILs at the Makefile level for the signal.h and sys/wait.h tests for 
most standards weren't removed until August 2017 (commit 
4fa9b3bfe6759c82beb4b043a54a3598ca467289, once the ucontext.h namespace 
fixes were completed), after the accidental SPARC si_band change, and so 
served to hide that architecture-specific problem.
Adhemerval Zanella Oct. 22, 2018, 7:49 p.m. UTC | #5
On 22/10/2018 12:07, Joseph Myers wrote:
> This has broken the conform/ tests for SPARC, so that you now get:
> 
> FAIL: conform/POSIX2008/signal.h/conform
> FAIL: conform/POSIX2008/sys/wait.h/conform
> FAIL: conform/UNIX98/signal.h/conform
> FAIL: conform/UNIX98/sys/wait.h/conform
> FAIL: conform/XOPEN2K/signal.h/conform
> FAIL: conform/XOPEN2K/sys/wait.h/conform
> FAIL: conform/XOPEN2K8/signal.h/conform
> FAIL: conform/XOPEN2K8/sys/wait.h/conform
> FAIL: conform/XPG42/signal.h/conform
> FAIL: conform/XPG42/sys/wait.h/conform
> 
> because si_band no longer has the POSIX-specified type.
> 
> For 32-bit SPARC you should be able to use long int and so avoid the 
> failures, as that won't change the layout.  For 64-bit SPARC this would be 
> a case where the kernel having the wrong type means you need an 
> appropriately conditioned and commented XFAIL in signal.h-data and 
> sys/wait.h-data (and appropriately conditioned and commented 
> conformtest-xfail-conds definition in a relevant sysdeps Makefile to 
> define the condition that's used in the conform/ data changes).
> 

I think we should XFAIL for both 32-bit and 64-bit, siginfo_t is passed
directly by kernel in signal handlers and passed unmodified on sigtimedwait,
sigwait, and sigwaitinfo syscalls.
Joseph Myers Oct. 22, 2018, 9:45 p.m. UTC | #6
On Mon, 22 Oct 2018, Adhemerval Zanella wrote:

> I think we should XFAIL for both 32-bit and 64-bit, siginfo_t is passed

Since long int and int have the same size for 32-bit, I see no need to 
XFAIL there when you can just use the proper POSIX type without affecting 
the layout.
Adhemerval Zanella Oct. 22, 2018, 10:02 p.m. UTC | #7
On 22/10/2018 18:45, Joseph Myers wrote:
> On Mon, 22 Oct 2018, Adhemerval Zanella wrote:
> 
>> I think we should XFAIL for both 32-bit and 64-bit, siginfo_t is passed
> 
> Since long int and int have the same size for 32-bit, I see no need to 
> XFAIL there when you can just use the proper POSIX type without affecting 
> the layout.
> 
I see your point, I would just like to keep documented that for SPARC there
is this small divergence between POSIX and kernel definition.
Joseph Myers Oct. 22, 2018, 10:10 p.m. UTC | #8
On Mon, 22 Oct 2018, Adhemerval Zanella wrote:

> On 22/10/2018 18:45, Joseph Myers wrote:
> > On Mon, 22 Oct 2018, Adhemerval Zanella wrote:
> > 
> >> I think we should XFAIL for both 32-bit and 64-bit, siginfo_t is passed
> > 
> > Since long int and int have the same size for 32-bit, I see no need to 
> > XFAIL there when you can just use the proper POSIX type without affecting 
> > the layout.
> > 
> I see your point, I would just like to keep documented that for SPARC there
> is this small divergence between POSIX and kernel definition.

I think XFAILing for such conform/ test assertions is only for the cases 
where the issue is hard to fix - not where the bad type can be replaced by 
the standard one with no change of layout involved.
Florian Weimer Oct. 25, 2018, 9:56 a.m. UTC | #9
* Joseph Myers:

> On Mon, 22 Oct 2018, Florian Weimer wrote:
>
>> > For 64-bit SPARC this would be a case where the kernel having the
>> > wrong type means you need an appropriately conditioned and commented
>> > XFAIL in signal.h-data and sys/wait.h-data (and appropriately
>> > conditioned and commented conformtest-xfail-conds definition in a
>> > relevant sysdeps Makefile to define the condition that's used in the
>> > conform/ data changes).
>> 
>> Why wasn't this a problem before
>> sysdeps/unix/sysv/linux/bits/types/siginfo_t.h was split out?  SPARC
>> clearly had int as the type of si_band back then.
>
> XFAILs at the Makefile level for the signal.h and sys/wait.h tests for 
> most standards weren't removed until August 2017 (commit 
> 4fa9b3bfe6759c82beb4b043a54a3598ca467289, once the ucontext.h namespace 
> fixes were completed), after the accidental SPARC si_band change, and so 
> served to hide that architecture-specific problem.

This is what I came up with.  I tried it on x86-64, sparcv9, and
sparc64, and the conform tests no longer fail.

conform: XFAIL siginfo_t si_band test on sparc64

We can use long int on sparcv9, but on sparc64, we must match the int
type used by the kernel (and not long int, as in POSIX).

2018-10-25  Florian Weimer  <fweimer@redhat.com>

	[BZ #23562]
	[BZ #23821]
	XFAIL siginfo_t si_band conform test on sparc64.
	* sysdeps/unix/sysv/linux/sparc/bits/siginfo-arch.h
	(__SI_BAND_TYPE): Only override long int default type on sparc64.
	* sysdeps/unix/sysv/linux/sparc/sparc64/Makefile
	(conformtest-xfail-conds): Add sparc64-linux.
	* conform/data/signal.h-data (siginfo_t): XFAIL si_band test on
	sparc64.
	* conform/data/sys/wait.h-data (siginfo_t): Likewise.

diff --git a/conform/data/signal.h-data b/conform/data/signal.h-data
index 11e54adb04..674e5793db 100644
--- a/conform/data/signal.h-data
+++ b/conform/data/signal.h-data
@@ -172,7 +172,8 @@ element siginfo_t pid_t si_pid
 element siginfo_t uid_t si_uid
 element siginfo_t {void*} si_addr
 element siginfo_t int si_status
-element siginfo_t long si_band
+// Bug 23821: si_band has type int on sparc64.
+xfail[sparc64-linux]-element siginfo_t long si_band
 #  endif
 #  ifndef XPG42
 element siginfo_t {union sigval} si_value
diff --git a/conform/data/sys/wait.h-data b/conform/data/sys/wait.h-data
index ed3869b34f..c0761424da 100644
--- a/conform/data/sys/wait.h-data
+++ b/conform/data/sys/wait.h-data
@@ -46,7 +46,8 @@ element siginfo_t pid_t si_pid
 element siginfo_t uid_t si_uid
 element siginfo_t {void*} si_addr
 element siginfo_t int si_status
-element siginfo_t long si_band
+// Bug 23821: si_band has type int on sparc64.
+xfail[sparc64-linux]-element siginfo_t long si_band
 # ifndef XPG42
 element siginfo_t {union sigval} si_value
 # endif
diff --git a/sysdeps/unix/sysv/linux/sparc/bits/siginfo-arch.h b/sysdeps/unix/sysv/linux/sparc/bits/siginfo-arch.h
index 9f79715ebe..4dd35237f6 100644
--- a/sysdeps/unix/sysv/linux/sparc/bits/siginfo-arch.h
+++ b/sysdeps/unix/sysv/linux/sparc/bits/siginfo-arch.h
@@ -2,7 +2,12 @@
 #ifndef _BITS_SIGINFO_ARCH_H
 #define _BITS_SIGINFO_ARCH_H 1
 
-#define __SI_BAND_TYPE int
+/* The kernel uses int instead of long int (as in POSIX).  In 32-bit
+   mode, we can still use long int, but in 64-bit mode, we need to
+   deviate from POSIX.  */
+#if __WORDSIZE == 64
+# define __SI_BAND_TYPE int
+#endif
 
 #define __SI_SIGFAULT_ADDL \
   int _si_trapno;
diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/Makefile b/sysdeps/unix/sysv/linux/sparc/sparc64/Makefile
index 715af3df7b..218c246f16 100644
--- a/sysdeps/unix/sysv/linux/sparc/sparc64/Makefile
+++ b/sysdeps/unix/sysv/linux/sparc/sparc64/Makefile
@@ -7,3 +7,8 @@ LD += -melf64_sparc
 ifeq ($(subdir),stdlib)
 sysdep_routines += __start_context
 endif
+
+ifeq ($(subdir),conform)
+# For bug 23821 (incorrect type of si_band).
+conformtest-xfail-conds += sparc64-linux
+endif
Joseph Myers Oct. 25, 2018, 12:22 p.m. UTC | #10
On Thu, 25 Oct 2018, Florian Weimer wrote:

> This is what I came up with.  I tried it on x86-64, sparcv9, and
> sparc64, and the conform tests no longer fail.
> 
> conform: XFAIL siginfo_t si_band test on sparc64
> 
> We can use long int on sparcv9, but on sparc64, we must match the int
> type used by the kernel (and not long int, as in POSIX).

Thanks, this looks good to me.

Patch
diff mbox series

diff --git a/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h b/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
index 33766d1813..43c4e009a4 100644
--- a/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
+++ b/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
@@ -107,7 +107,7 @@  typedef struct
 	/* SIGPOLL.  */
 	struct
 	  {
-	    long int si_band;	/* Band event for SIGPOLL.  */
+	    __SI_BAND_TYPE si_band;	/* Band event for SIGPOLL.  */
 	    int si_fd;
 	  } _sigpoll;