| Message ID | 877eifgpn7.fsf@oldenburg.str.redhat.com |
|---|---|
| State | Committed |
| Headers |
Received: (qmail 33579 invoked by alias); 18 Oct 2018 16:56:24 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 33567 invoked by uid 89); 18 Oct 2018 16:56:23 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=band X-HELO: mx1.redhat.com From: Florian Weimer <fweimer@redhat.com> To: libc-alpha@sourceware.org Cc: Ilya Yu. Malakhov <malakhov@mcst.ru> Subject: [PATCH] signal: Use correct type for si_band in siginfo_t [BZ #23562] Date: Thu, 18 Oct 2018 18:56:12 +0200 Message-ID: <877eifgpn7.fsf@oldenburg.str.redhat.com> MIME-Version: 1.0 Content-Type: text/plain |
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
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.
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;