From owner-freebsd-threads@FreeBSD.ORG Sat Feb 19 19:54:40 2011 Return-Path: Delivered-To: freebsd-threads@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 344AE106564A for ; Sat, 19 Feb 2011 19:54:40 +0000 (UTC) (envelope-from kosaki.motohiro@gmail.com) Received: from mail-wy0-f182.google.com (mail-wy0-f182.google.com [74.125.82.182]) by mx1.freebsd.org (Postfix) with ESMTP id BBEBD8FC1B for ; Sat, 19 Feb 2011 19:54:39 +0000 (UTC) Received: by wyb32 with SMTP id 32so631401wyb.13 for ; Sat, 19 Feb 2011 11:54:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type:content-transfer-encoding; bh=GeCYMSIxb6raP8ZELfvwATHoBllQm+bKeyXv3XcSL+4=; b=q+vtgHHoRiC8mAIekqfFs1kc2mSZny11d7JVyHB+2GWJ+AdFCrcBsxP99fsUvGeqZ9 I/vYBYuV9SRqREPoDq/UR13m0uQaPJ8vKJfyenUhHF41nW7dQyRhY+7Cd6zSuAShTxr3 wCyFKzEcHWIddXnPz5akLCZRzff12XbHaHNYA= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type:content-transfer-encoding; b=nldSVDLRucJupeEVw2rRzRzR7SzEo1uHm0P+MlU4m2aLjzSvyqJHAPOAJGF1VYlQjL t093b3P8KCEMJKcROuV+BJhIeAS2XJ+euLrIYskU1pV9Vrgr3xvxWDhO8RsvlLR3vjsU wiExvO87LiJKblKqYDDf101LcLmxK03L9+7P0= Received: by 10.216.7.8 with SMTP id 8mr740570weo.30.1298143386866; Sat, 19 Feb 2011 11:23:06 -0800 (PST) MIME-Version: 1.0 Received: by 10.216.182.74 with HTTP; Sat, 19 Feb 2011 11:22:46 -0800 (PST) In-Reply-To: <20110219184348.GK78089@deviant.kiev.zoral.com.ua> References: <201102191809.p1JI9rZi063561@red.freebsd.org> <20110219184348.GK78089@deviant.kiev.zoral.com.ua> From: KOSAKI Motohiro Date: Sun, 20 Feb 2011 04:22:46 +0900 Message-ID: To: Kostik Belousov Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: freebsd-threads@freebsd.org Subject: Re: threads/154893: pthread_sigmask don't work if mask and oldmask are passed the same pointer X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 19 Feb 2011 19:54:40 -0000 Hi > Note that POSIX requires the following prototypes for > the sigprocmask and pthread_sigmask: > > int pthread_sigmask(int how, const sigset_t *restrict set, > =A0 =A0 =A0 sigset_t *restrict oset); > int sigprocmask(int how, const sigset_t *restrict set, > =A0 =A0 =A0sigset_t *restrict oset); Ouch. right you are. > The restrict keyword explicitely disallows the case of set =3D=3D oset, > so I think the behaviour is undefined by POSIX, and our implementation > is correct. I agree. > On the other hand, pthread_sigmask(3) manpage needs update to add > restrict, and include/signal.h also ought to be changed. OK, thanks. >> Then, if set =3D=3D oset, set argument was override before use it at (1)= . >> To introduce temporary variable fix this issue easily. > libc_r is unused. Real implementation is in lib/libthr, that already > has a twist that would not allow the override for case of action !=3D > SIG_UNBLOCK. Moreover, the kernel side of sigprocmask(2) implementation > first copies new set into kernel VA, and only then copies old mask out. Hmhm. > Your example does not supply oset =3D=3D set to the pthread_sigmask, > meantime. I really do not see anything wrong with the output of > the program that supposed to illustrate the issue. Oh, I'm sorry. It's cut-n-paste mistake. I did paste old version unintensionally. void* func2(void* arg) { sigset_t old; sigset_t add; int i; sigemptyset(&old); pthread_sigmask(SIG_BLOCK, NULL, &old); printf("before: "); for (i=3D0; i<4; i++) printf(" %08x", old.__bits[i]); printf("\n"); sigemptyset(&add); sigaddset(&add, SIGUSR1); pthread_sigmask(SIG_BLOCK, &add, &add); pthread_sigmask(SIG_BLOCK, NULL, &old); printf("after: "); for (i=3D0; i<4; i++) printf(" %08x", old.__bits[i]); printf("\n"); return 0; } The result is, correct case: before: 00000000 00000000 00000000 00000000 after: 20000000 00000000 00000000 00000000 incorrect case: before: 00000000 00000000 00000000 00000000 after: 00000000 00000000 00000000 00000000 difference between func and func2 are - pthread_sigmask(SIG_BLOCK, &add, NULL); + pthread_sigmask(SIG_BLOCK, &add, &add); That said, To add oset changed sigprocmask() behavior significantly. > The func() adds SIGUSR1 to the mask with second call to > pthread_sigmask(SIG_BLOCK, &add, NULL);. Then, third call > correctly returns SIGUSR1 in the mask. > > On the other hand, func2() sets SIGUSR1 as blocked and retrieves > the previous mask in single atomic action. Since SIGUSR1 was not > blocked before the call, old sigset correctly indicates it as > "not blocked". (snip) > The only change that I see as needed now is the following cosmetics: > > commit 49287c24fc46f342b46db1fae3fe9982bfbf7ed9 > Author: Konstantin Belousov > Date: =A0 Sat Feb 19 20:41:46 2011 +0200 > > =A0 =A0Add restrict keyword to pthread_sigmask prototype and manpage > > diff --git a/include/signal.h b/include/signal.h > index 4a4cd17..52a611c 100644 > --- a/include/signal.h > +++ b/include/signal.h > @@ -69,7 +69,8 @@ int =A0 raise(int); > =A0#if __POSIX_VISIBLE || __XSI_VISIBLE > =A0int =A0 =A0kill(__pid_t, int); > =A0int =A0 =A0pthread_kill(__pthread_t, int); > -int =A0 =A0pthread_sigmask(int, const __sigset_t *, __sigset_t *); > +int =A0 =A0pthread_sigmask(int, const __sigset_t *__restrict, > + =A0 =A0 =A0 =A0 =A0 __sigset_t * __restrict); > =A0int =A0 =A0sigaction(int, const struct sigaction * __restrict, > =A0 =A0 =A0 =A0 =A0 =A0struct sigaction * __restrict); > =A0int =A0 =A0sigaddset(sigset_t *, int); > diff --git a/share/man/man3/pthread_sigmask.3 b/share/man/man3/pthread_si= gmask.3 > index c412543..013ba7c 100644 > --- a/share/man/man3/pthread_sigmask.3 > +++ b/share/man/man3/pthread_sigmask.3 > @@ -26,7 +26,7 @@ > =A0.\" EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > =A0.\" > =A0.\" $FreeBSD$ > -.Dd April 27, 2000 > +.Dd February 19, 2011 > =A0.Dt PTHREAD_SIGMASK 3 > =A0.Os > =A0.Sh NAME > @@ -38,7 +38,8 @@ > =A0.In pthread.h > =A0.In signal.h > =A0.Ft int > -.Fn pthread_sigmask "int how" "const sigset_t *set" "sigset_t *oset" > +.Fn pthread_sigmask "int how" "const sigset_t * restrict set" \ > + =A0 =A0"sigset_t * restrict oset" > =A0.Sh DESCRIPTION > =A0The > =A0.Fn pthread_sigmask Looks good.