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] | expand |
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; > >
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).
* 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
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.
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.
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.
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.
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.
* 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
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.
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;