From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 49952 invoked by alias); 4 Jan 2016 15:22:48 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 49942 invoked by uid 89); 4 Jan 2016 15:22:48 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.5 required=5.0 tests=AWL,BAYES_50,KAM_ASCII_DIVIDERS,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no version=3.3.2 spammy=nand, smallexample, pointed, params X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 04 Jan 2016 15:22:47 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id EF772C10909E; Mon, 4 Jan 2016 15:22:45 +0000 (UTC) Received: from redhat.com (ovpn-204-22.brq.redhat.com [10.40.204.22]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u04FMeEB030397 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Mon, 4 Jan 2016 10:22:44 -0500 Date: Mon, 04 Jan 2016 15:22:00 -0000 From: Marek Polacek To: Martin Sebor Cc: Jeff Law , Gcc Patch List , "Joseph S. Myers" Subject: Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed Message-ID: <20160104152239.GB31604@redhat.com> References: <567A271B.2090403@gmail.com> <56877F8B.9010000@redhat.com> <5689E0F8.5050209@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5689E0F8.5050209@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-SW-Source: 2016-01/txt/msg00090.txt.bz2 Hi Martin, On Sun, Jan 03, 2016 at 08:03:20PM -0700, Martin Sebor wrote: > Index: gcc/doc/extend.texi > =================================================================== > --- gcc/doc/extend.texi (revision 232047) > +++ gcc/doc/extend.texi (working copy) > @@ -9238,6 +9238,8 @@ > @{ tmp = *ptr; *ptr = ~(tmp & value); return tmp; @} // nand > @end smallexample > > +The object pointed to by the first argument must of integer or pointer type. It must not be a Boolean type. Too long line and missing "be " after "must"? > +The same constraints on arguments apply as for the corresponding @code{__sync_op_and_fetch} built-in functions. > + Too long line. > -All memory orders are valid. > +The object pointed to by the first argument must of integer or pointer type. It must not be a Boolean type. All memory orders are valid. Too long line and missing "be " after "must"? > +The same constraints on arguments apply as for the corresponding @code{__atomic_op_fetch} built-in functions. All memory orders are valid. Too long line. > @@ -10686,12 +10691,16 @@ > if (!INTEGRAL_TYPE_P (type) && !POINTER_TYPE_P (type)) > goto incompatible; > > + if (fetch && TREE_CODE (type) == BOOLEAN_TYPE) > + goto incompatible; This goto is indented two more spaces than it should be. > @@ -11250,6 +11259,11 @@ > vec *params) > { > enum built_in_function orig_code = DECL_FUNCTION_CODE (function); > + > + /* Is function is one of the _FETCH_OP_ or _OP_FETCH_ built-ins? I think drop the second "is". > @@ -11325,6 +11339,9 @@ > case BUILT_IN_ATOMIC_COMPARE_EXCHANGE_N: > case BUILT_IN_ATOMIC_LOAD_N: > case BUILT_IN_ATOMIC_STORE_N: > + { > + fetch_op = false; > + } Let's either remove those {} or add a fallthrough comment as done above. > @@ -11358,7 +11375,16 @@ > case BUILT_IN_SYNC_LOCK_TEST_AND_SET_N: > case BUILT_IN_SYNC_LOCK_RELEASE_N: > { > - int n = sync_resolve_size (function, params); > + /* The following are not _FETCH_OPs and must be accepted with > + pointers to _Bool (or C++ bool). */ > + if (fetch_op) > + fetch_op = > + orig_code != BUILT_IN_SYNC_BOOL_COMPARE_AND_SWAP_N > + && orig_code != BUILT_IN_SYNC_VAL_COMPARE_AND_SWAP_N > + && orig_code != BUILT_IN_SYNC_LOCK_TEST_AND_SET_N > + && orig_code != BUILT_IN_SYNC_LOCK_RELEASE_N; > + Trailing whitespaces on this line. And I think add () around the RHS of the assignment to fetch_op. > Index: gcc/testsuite/gcc.dg/atomic-fetch-bool.c > =================================================================== > --- gcc/testsuite/gcc.dg/atomic-fetch-bool.c (revision 0) > +++ gcc/testsuite/gcc.dg/atomic-fetch-bool.c (working copy) > @@ -0,0 +1,64 @@ > +/* PR c/68966 - atomic_fetch_* on atomic_bool not diagnosed > + Test to verify that calls to __atomic_fetch_op funcions with a _Bool > + argument are rejected. This is necessary because GCC expects that > + all initialized _Bool objects have a specific representation and > + allowing atomic operations to change it would break the invariant. */ > +/* { dg-do compile } */ > +/* { dg-options "-std=c11" } */ Doesn't matter here, but probably add -pedantic-errors. > Index: gcc/testsuite/gcc.dg/sync-fetch-bool.c > =================================================================== > --- gcc/testsuite/gcc.dg/sync-fetch-bool.c (revision 0) > +++ gcc/testsuite/gcc.dg/sync-fetch-bool.c (working copy) > @@ -0,0 +1,54 @@ > +/* PR c/68966 - atomic_fetch_* on atomic_bool not diagnosed > + Test to verify that calls to __sync_fetch_op funcions with a _Bool > + argument are rejected. This is necessary because GCC expects that > + all initialized _Bool objects have a specific representation and > + allowing atomic operations to change it would break the invariant. */ > +/* { dg-do compile } */ > +/* { dg-options "-std=c99" } */ As the testcase uses _Atomic, I wonder why there's -std=c99. I'd use -std=c11 -pedantic-errors. Thanks, Marek