Message ID | CAMe9rOqeAtFanFH=Wphf_ksxRpfw2p_WsJTvudpq13b1jhHQkQ@mail.gmail.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 33837 invoked by alias); 25 Jul 2018 16:31:51 -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 33827 invoked by uid 89); 25 Jul 2018 16:31:51 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=H*RU:209.85.218.67, Hx-spam-relays-external:209.85.218.67 X-HELO: mail-oi0-f67.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=B2Zg+QXYDX01jZGeyn5Bn+dbMKcCFQsMZRH/ehjfd4M=; b=VqAQX8SNLcWfVpbJKdj24C/424Rwmh/ZW8OKWCOIddcRaSV21FiG/qRuaaUgPZX7zz AOLP6GX/WqYrD1khSTTmfVu6sv5t/n0fFp9Gw2OF4BnMLF1vohWI7ZUzBW2R6jZRrArp rrwJ3SPbUrMNWA03PgYSpK0gBUJ6iPAOcMLwF3hmOUKGpTL2ghoWFReHgoHdegNRbV1B bYBEmsAbWqPp2Gf7ivdNMboPQ/caJ/UO2yLpmPJm3fb1ssQh7h0RWMhUX6KM1osd2p1L vJCnlr4uyjD30T3ye10kKflRhf/wYyHbGGH4UzuO4VdOhZ3IkOqwaix8kcNkTrTGrMMG TXbg== MIME-Version: 1.0 In-Reply-To: <c4ab08a3-658b-b31c-140f-5a43f4e3f532@redhat.com> References: <20180721142035.21059-1-hjl.tools@gmail.com> <20180721142035.21059-11-hjl.tools@gmail.com> <c4ab08a3-658b-b31c-140f-5a43f4e3f532@redhat.com> From: "H.J. Lu" <hjl.tools@gmail.com> Date: Wed, 25 Jul 2018 09:31:46 -0700 Message-ID: <CAMe9rOqeAtFanFH=Wphf_ksxRpfw2p_WsJTvudpq13b1jhHQkQ@mail.gmail.com> Subject: Re: [PATCH 10/12] Add another test for setcontext To: Florian Weimer <fweimer@redhat.com> Cc: GNU C Library <libc-alpha@sourceware.org>, "Carlos O'Donell" <carlos@redhat.com> Content-Type: multipart/mixed; boundary="00000000000030c0900571d56a4c" |
Commit Message
H.J. Lu
July 25, 2018, 4:31 p.m. UTC
On Wed, Jul 25, 2018 at 9:21 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 07/21/2018 04:20 PM, H.J. Lu wrote: >> >> + /* check sigmask in old context of swapcontext-call */ >> + if (sigismember (&oldctx.uc_sigmask, SIGUSR2) != 1) >> + { >> + puts ("FAIL: SIGUSR2 is not blocked in oldctx.uc_sigmask."); >> + exit (1); >> + } > > > This breaks on ia64 because uc_sigmask does not have the correct type there: > > tst-setcontext4.c: In function ‘do_test’: > tst-setcontext4.c:202:20: error: passing argument 1 of ‘sigismember’ from > incompatible pointer type [-Werror=incompatible-pointer-types] > if (sigismember (&oldctx.uc_sigmask, SIGUSR2) != 1) > In file included from ../include/signal.h:2, > from tst-setcontext4.c:23: > ../signal/signal.h:208:41: note: expected ‘const sigset_t *’ {aka ‘const > struct <anonymous> *’} but argument is of type ‘long unsigned int *’ > extern int sigismember (const sigset_t *__set, int __signo) > ~~~~~~~~~~~~~~~~^~~~~ > > I don't have a quick solution for this. How important is this part of the > test? > I am testing this patch.
Comments
On 07/25/2018 12:31 PM, H.J. Lu wrote: > From 70951c51d03623c10485380fcccd3a384dff20d6 Mon Sep 17 00:00:00 2001 > From: "H.J. Lu" <hjl.tools@gmail.com> > Date: Wed, 25 Jul 2018 09:30:50 -0700 > Subject: [PATCH] ia64: Work around incorrect type of IA64 uc_sigmask > > * sysdeps/unix/sysv/linux/ia64/tst-setcontext4.c: New file. Please file a bug for ia64 about this issue, then add the comment below. Pre-accepted for 2.28 / master. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > --- > .../unix/sysv/linux/ia64/tst-setcontext4.c | 24 +++++++++++++++++++ > 1 file changed, 24 insertions(+) > create mode 100644 sysdeps/unix/sysv/linux/ia64/tst-setcontext4.c > > diff --git a/sysdeps/unix/sysv/linux/ia64/tst-setcontext4.c b/sysdeps/unix/sysv/linux/ia64/tst-setcontext4.c > new file mode 100644 > index 0000000000..58807e78cb > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/ia64/tst-setcontext4.c > @@ -0,0 +1,24 @@ > +/* Work around incorrect type of IA64 uc_sigmask. > + Copyright (C) 2018 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <http://www.gnu.org/licenses/>. */ > + > +#include <signal.h> > + Add comment: /* The uc_sigmask on IA64 has the wrong type and this needs fixing, but until that change is evaluated, we fix this here with a cast. See bug XXX. */ > +#undef sigismember > +#define sigismember(set, signo) sigismember ((const sigset_t *) (set), (signo)) > + > +#include <stdlib/tst-setcontext4.c> > -- 2.17.1
* Carlos O'Donell: > /* The uc_sigmask on IA64 has the wrong type and this needs fixing, > but until that change is evaluated, we fix this here with a cast. > See bug XXX. */ Is this really a bug? After this long, I would consider it just a quirk of the ia64 API/ABI and move on …
On Wed, 25 Jul 2018, Carlos O'Donell wrote: > On 07/25/2018 12:31 PM, H.J. Lu wrote: > > From 70951c51d03623c10485380fcccd3a384dff20d6 Mon Sep 17 00:00:00 2001 > > From: "H.J. Lu" <hjl.tools@gmail.com> > > Date: Wed, 25 Jul 2018 09:30:50 -0700 > > Subject: [PATCH] ia64: Work around incorrect type of IA64 uc_sigmask > > > > * sysdeps/unix/sysv/linux/ia64/tst-setcontext4.c: New file. > > Please file a bug for ia64 about this issue, then add the comment below. It's bug 21634 (filed for XFAILing the relevant conform/ test assertions, not because of any expectation of being fixable while staying ABI-compatible).
On 07/25/2018 04:42 PM, Florian Weimer wrote: > * Carlos O'Donell: > >> /* The uc_sigmask on IA64 has the wrong type and this needs fixing, >> but until that change is evaluated, we fix this here with a cast. >> See bug XXX. */ > > Is this really a bug? After this long, I would consider it just a > quirk of the ia64 API/ABI and move on … You can't use sigismember(); I would consider that a bug? c.
* Carlos O'Donell: > On 07/25/2018 04:42 PM, Florian Weimer wrote: >> * Carlos O'Donell: >> >>> /* The uc_sigmask on IA64 has the wrong type and this needs fixing, >>> but until that change is evaluated, we fix this here with a cast. >>> See bug XXX. */ >> >> Is this really a bug? After this long, I would consider it just a >> quirk of the ia64 API/ABI and move on … > > You can't use sigismember(); I would consider that a bug? Well, if the ABI says the member has the type it has, and that's not compatible with sigismember, then that's how things are. Surely this isn't the only such example. What about the return type of the tsearch function?
On 07/25/2018 05:21 PM, Florian Weimer wrote: > * Carlos O'Donell: > >> On 07/25/2018 04:42 PM, Florian Weimer wrote: >>> * Carlos O'Donell: >>> >>>> /* The uc_sigmask on IA64 has the wrong type and this needs fixing, >>>> but until that change is evaluated, we fix this here with a cast. >>>> See bug XXX. */ >>> >>> Is this really a bug? After this long, I would consider it just a >>> quirk of the ia64 API/ABI and move on … >> >> You can't use sigismember(); I would consider that a bug? > > Well, if the ABI says the member has the type it has, and that's not > compatible with sigismember, then that's how things are. Surely this > isn't the only such example. What about the return type of the > tsearch function? What about tsearch? It returns a void* and so you do have to cast that to something. Are you saying this is a similar issue? The void* creates an expectation of a cast, which I think we don't have here. c.
* Carlos O'Donell: > On 07/25/2018 05:21 PM, Florian Weimer wrote: >> * Carlos O'Donell: >> >>> On 07/25/2018 04:42 PM, Florian Weimer wrote: >>>> * Carlos O'Donell: >>>> >>>>> /* The uc_sigmask on IA64 has the wrong type and this needs fixing, >>>>> but until that change is evaluated, we fix this here with a cast. >>>>> See bug XXX. */ >>>> >>>> Is this really a bug? After this long, I would consider it just a >>>> quirk of the ia64 API/ABI and move on … >>> >>> You can't use sigismember(); I would consider that a bug? >> >> Well, if the ABI says the member has the type it has, and that's not >> compatible with sigismember, then that's how things are. Surely this >> isn't the only such example. What about the return type of the >> tsearch function? > > What about tsearch? It returns a void* and so you do have to cast that > to something. Are you saying this is a similar issue? The void* creates > an expectation of a cast, which I think we don't have here. The return type is a pointer to a const void *. You are not supposed to cast the returned pointer, but that const void * (after checking that the return value is not NULL).
On Jul 25 2018, Florian Weimer <fw@deneb.enyo.de> wrote: > * Carlos O'Donell: > >> /* The uc_sigmask on IA64 has the wrong type and this needs fixing, >> but until that change is evaluated, we fix this here with a cast. >> See bug XXX. */ > > Is this really a bug? After this long, I would consider it just a > quirk of the ia64 API/ABI and move on … It isn't POSIX compatible (when POSIX still supported it). Andreas.
On 25/07/2018 17:45, Joseph Myers wrote: > On Wed, 25 Jul 2018, Carlos O'Donell wrote: > >> On 07/25/2018 12:31 PM, H.J. Lu wrote: >>> From 70951c51d03623c10485380fcccd3a384dff20d6 Mon Sep 17 00:00:00 2001 >>> From: "H.J. Lu" <hjl.tools@gmail.com> >>> Date: Wed, 25 Jul 2018 09:30:50 -0700 >>> Subject: [PATCH] ia64: Work around incorrect type of IA64 uc_sigmask >>> >>> * sysdeps/unix/sysv/linux/ia64/tst-setcontext4.c: New file. >> >> Please file a bug for ia64 about this issue, then add the comment below. > > It's bug 21634 (filed for XFAILing the relevant conform/ test assertions, > not because of any expectation of being fixable while staying > ABI-compatible). > What would require to fix this issue for ia64? The kernel interface already seems to use the expected sigset_t type for sc_mask and I think it would be feasible to use the *context functions as is with only adjusting the size of rt_sigprocmask to be dependent of _NSIG.
On 07/26/2018 08:31 AM, Adhemerval Zanella wrote: > > > On 25/07/2018 17:45, Joseph Myers wrote: >> On Wed, 25 Jul 2018, Carlos O'Donell wrote: >> >>> On 07/25/2018 12:31 PM, H.J. Lu wrote: >>>> From 70951c51d03623c10485380fcccd3a384dff20d6 Mon Sep 17 00:00:00 2001 >>>> From: "H.J. Lu" <hjl.tools@gmail.com> >>>> Date: Wed, 25 Jul 2018 09:30:50 -0700 >>>> Subject: [PATCH] ia64: Work around incorrect type of IA64 uc_sigmask >>>> >>>> * sysdeps/unix/sysv/linux/ia64/tst-setcontext4.c: New file. >>> >>> Please file a bug for ia64 about this issue, then add the comment below. >> >> It's bug 21634 (filed for XFAILing the relevant conform/ test assertions, >> not because of any expectation of being fixable while staying >> ABI-compatible). >> > > What would require to fix this issue for ia64? The kernel interface already > seems to use the expected sigset_t type for sc_mask and I think it would > be feasible to use the *context functions as is with only adjusting the > size of rt_sigprocmask to be dependent of _NSIG. That all sounds reasonable :-) c.
On Thu, 26 Jul 2018, Adhemerval Zanella wrote: > > It's bug 21634 (filed for XFAILing the relevant conform/ test assertions, > > not because of any expectation of being fixable while staying > > ABI-compatible). > > What would require to fix this issue for ia64? The kernel interface already > seems to use the expected sigset_t type for sc_mask and I think it would > be feasible to use the *context functions as is with only adjusting the > size of rt_sigprocmask to be dependent of _NSIG. glibc sigset_t is much larger than kernel sigset_t.
On 26/07/2018 12:04, Joseph Myers wrote: > On Thu, 26 Jul 2018, Adhemerval Zanella wrote: > >>> It's bug 21634 (filed for XFAILing the relevant conform/ test assertions, >>> not because of any expectation of being fixable while staying >>> ABI-compatible). >> >> What would require to fix this issue for ia64? The kernel interface already >> seems to use the expected sigset_t type for sc_mask and I think it would >> be feasible to use the *context functions as is with only adjusting the >> size of rt_sigprocmask to be dependent of _NSIG. > > glibc sigset_t is much larger than kernel sigset_t. > Sigh, indeed 'new' kernel sigset_t has the same size as previous one, it has changed only to use a struct. I still think it would be feasible to change uc_sigmask to glibc sigset_t, the *context functions is already using the expected sizes for __NR_sigprocmask. Do you see anything that need further adjustment?
On Fri, 27 Jul 2018, Adhemerval Zanella wrote: > Sigh, indeed 'new' kernel sigset_t has the same size as previous one, > it has changed only to use a struct. I still think it would be feasible > to change uc_sigmask to glibc sigset_t, the *context functions is > already using the expected sizes for __NR_sigprocmask. Do you see > anything that need further adjustment? Well, there are all the usual risks of any structure size change.
From 70951c51d03623c10485380fcccd3a384dff20d6 Mon Sep 17 00:00:00 2001 From: "H.J. Lu" <hjl.tools@gmail.com> Date: Wed, 25 Jul 2018 09:30:50 -0700 Subject: [PATCH] ia64: Work around incorrect type of IA64 uc_sigmask * sysdeps/unix/sysv/linux/ia64/tst-setcontext4.c: New file. --- .../unix/sysv/linux/ia64/tst-setcontext4.c | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 sysdeps/unix/sysv/linux/ia64/tst-setcontext4.c diff --git a/sysdeps/unix/sysv/linux/ia64/tst-setcontext4.c b/sysdeps/unix/sysv/linux/ia64/tst-setcontext4.c new file mode 100644 index 0000000000..58807e78cb --- /dev/null +++ b/sysdeps/unix/sysv/linux/ia64/tst-setcontext4.c @@ -0,0 +1,24 @@ +/* Work around incorrect type of IA64 uc_sigmask. + Copyright (C) 2018 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#include <signal.h> + +#undef sigismember +#define sigismember(set, signo) sigismember ((const sigset_t *) (set), (signo)) + +#include <stdlib/tst-setcontext4.c> -- 2.17.1