From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 53805 invoked by alias); 5 Jan 2016 01:18:41 -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 52751 invoked by uid 89); 5 Jan 2016 01:18:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.7 required=5.0 tests=AWL,BAYES_50,FREEMAIL_FROM,KAM_ASCII_DIVIDERS,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=no version=3.3.2 spammy=automation, nand, resolving, params X-HELO: mail-qk0-f196.google.com Received: from mail-qk0-f196.google.com (HELO mail-qk0-f196.google.com) (209.85.220.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 05 Jan 2016 01:18:38 +0000 Received: by mail-qk0-f196.google.com with SMTP id f10so7124172qke.1 for ; Mon, 04 Jan 2016 17:18:37 -0800 (PST) X-Received: by 10.55.78.75 with SMTP id c72mr117957899qkb.97.1451956716029; Mon, 04 Jan 2016 17:18:36 -0800 (PST) Received: from [192.168.0.26] (71-212-229-169.hlrn.qwest.net. [71.212.229.169]) by smtp.gmail.com with ESMTPSA id l38sm41056989qgd.27.2016.01.04.17.18.33 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 04 Jan 2016 17:18:34 -0800 (PST) Message-ID: <568B19E8.1020709@gmail.com> Date: Tue, 05 Jan 2016 01:18:00 -0000 From: Martin Sebor User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Marek Polacek CC: Jeff Law , Gcc Patch List , "Joseph S. Myers" Subject: Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed References: <567A271B.2090403@gmail.com> <56877F8B.9010000@redhat.com> <5689E0F8.5050209@gmail.com> <20160104152239.GB31604@redhat.com> In-Reply-To: <20160104152239.GB31604@redhat.com> Content-Type: multipart/mixed; boundary="------------030800070003010908090402" X-IsSubscribed: yes X-SW-Source: 2016-01/txt/msg00117.txt.bz2 This is a multi-part message in MIME format. --------------030800070003010908090402 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Content-length: 854 On 01/04/2016 08:22 AM, Marek Polacek wrote: > Hi Martin, > ... Thanks for the careful review! I've fixed the problems you pointed out in the attached patch. The typos are my bad. As for the whitespace, I have to confess I'm finding all the rules tedious to follow without some sort of automation. Jason suggested some option to git but I don't use git to commit (too many other problems). I'm also not sure the option makes Git replace 8 spaces with TABs. I tried to have my Emacs automatically strip trailing whitespace for me but that was causing spurious changes on otherwise untouched lines that already contain it (clearly, I'm not the only who struggles with whitespace). I don't suppose everyone is voluntarily subjecting themselves to this torture so there must be a way to make it less onerous and painful. What's your secret? Martin --------------030800070003010908090402 Content-Type: text/x-patch; name="gcc-68966.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="gcc-68966.patch" Content-length: 15473 gcc/ChangeLog: 2016-01-04 Martin Sebor PR c/68966 * doc/extend.texi (__atomic Builtins, __sync Builtins): Document constraint on the type of arguments. gcc/c-family/ChangeLog: 2016-01-04 Martin Sebor PR c/68966 * c-common.c (sync_resolve_size): Reject first argument when it's a pointer to _Bool. gcc/testsuite/ChangeLog: 2016-01-04 Martin Sebor PR c/68966 * gcc.dg/atomic-fetch-bool.c: New test. * gcc.dg/sync-fetch-bool.c: Same. Index: gcc/doc/extend.texi =================================================================== --- gcc/doc/extend.texi (revision 232047) +++ gcc/doc/extend.texi (working copy) @@ -9238,6 +9238,9 @@ @{ tmp = *ptr; *ptr = ~(tmp & value); return tmp; @} // nand @end smallexample +The object pointed to by the first argument must be of integer or pointer +type. It must not be a Boolean type. + @emph{Note:} GCC 4.4 and later implement @code{__sync_fetch_and_nand} as @code{*ptr = ~(tmp & value)} instead of @code{*ptr = ~tmp & value}. @@ -9261,6 +9264,9 @@ @{ *ptr = ~(*ptr & value); return *ptr; @} // nand @end smallexample +The same constraints on arguments apply as for the corresponding +@code{__sync_op_and_fetch} built-in functions. + @emph{Note:} GCC 4.4 and later implement @code{__sync_nand_and_fetch} as @code{*ptr = ~(*ptr & value)} instead of @code{*ptr = ~*ptr & value}. @@ -9507,13 +9513,14 @@ @deftypefnx {Built-in Function} @var{type} __atomic_or_fetch (@var{type} *ptr, @var{type} val, int memorder) @deftypefnx {Built-in Function} @var{type} __atomic_nand_fetch (@var{type} *ptr, @var{type} val, int memorder) These built-in functions perform the operation suggested by the name, and -return the result of the operation. That is, +return the result of the operation. That is, @smallexample @{ *ptr @var{op}= val; return *ptr; @} @end smallexample -All memory orders are valid. +The object pointed to by the first argument must be of integer or pointer +type. It must not be a Boolean type. All memory orders are valid. @end deftypefn @@ -9530,7 +9537,8 @@ @{ tmp = *ptr; *ptr @var{op}= val; return tmp; @} @end smallexample -All memory orders are valid. +The same constraints on arguments apply as for the corresponding +@code{__atomic_op_fetch} built-in functions. All memory orders are valid. @end deftypefn Index: gcc/c-family/c-common.c =================================================================== --- gcc/c-family/c-common.c (revision 232047) +++ gcc/c-family/c-common.c (working copy) @@ -7804,7 +7804,7 @@ else if (TYPE_P (*node)) type = node, is_type = 1; - if ((i = check_user_alignment (align_expr, false)) == -1 + if ((i = check_user_alignment (align_expr, true)) == -1 || !check_cxx_fundamental_alignment_constraints (*node, i, flags)) *no_add_attrs = true; else if (is_type) @@ -10657,11 +10657,16 @@ /* A helper function for resolve_overloaded_builtin in resolving the overloaded __sync_ builtins. Returns a positive power of 2 if the first operand of PARAMS is a pointer to a supported data type. - Returns 0 if an error is encountered. */ + Returns 0 if an error is encountered. + FETCH is true when FUNCTION is one of the _FETCH_OP_ or _OP_FETCH_ + built-ins. */ static int -sync_resolve_size (tree function, vec *params) +sync_resolve_size (tree function, vec *params, bool fetch) { + /* Type of the argument. */ + tree argtype; + /* Type the argument points to. */ tree type; int size; @@ -10671,7 +10676,7 @@ return 0; } - type = TREE_TYPE ((*params)[0]); + argtype = type = TREE_TYPE ((*params)[0]); if (TREE_CODE (type) == ARRAY_TYPE) { /* Force array-to-pointer decay for C++. */ @@ -10686,12 +10691,16 @@ if (!INTEGRAL_TYPE_P (type) && !POINTER_TYPE_P (type)) goto incompatible; + if (fetch && TREE_CODE (type) == BOOLEAN_TYPE) + goto incompatible; + size = tree_to_uhwi (TYPE_SIZE_UNIT (type)); if (size == 1 || size == 2 || size == 4 || size == 8 || size == 16) return size; incompatible: - error ("incompatible type for argument %d of %qE", 1, function); + error ("operand type %qT is incompatible with argument %d of %qE", + argtype, 1, function); return 0; } @@ -11250,6 +11259,11 @@ vec *params) { enum built_in_function orig_code = DECL_FUNCTION_CODE (function); + + /* Is function one of the _FETCH_OP_ or _OP_FETCH_ built-ins? + Those are not valid to call with a pointer to _Bool (or C++ bool) + and so must be rejected. */ + bool fetch_op = true; bool orig_format = true; tree new_return = NULL_TREE; @@ -11325,6 +11339,10 @@ case BUILT_IN_ATOMIC_COMPARE_EXCHANGE_N: case BUILT_IN_ATOMIC_LOAD_N: case BUILT_IN_ATOMIC_STORE_N: + { + fetch_op = false; + /* Fallthrough to further processing. */ + } case BUILT_IN_ATOMIC_ADD_FETCH_N: case BUILT_IN_ATOMIC_SUB_FETCH_N: case BUILT_IN_ATOMIC_AND_FETCH_N: @@ -11358,7 +11376,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); + + int n = sync_resolve_size (function, params, fetch_op); tree new_function, first_param, result; enum built_in_function fncode; 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 "-pedantic-errors -std=c11" } */ + + +void test_atomic_bool (_Atomic _Bool *a) +{ + enum { SEQ_CST = __ATOMIC_SEQ_CST }; + + __atomic_fetch_add (a, 1, SEQ_CST); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__atomic_fetch_add." } */ + __atomic_fetch_sub (a, 1, SEQ_CST); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__atomic_fetch_sub." } */ + __atomic_fetch_and (a, 1, SEQ_CST); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__atomic_fetch_and." } */ + __atomic_fetch_xor (a, 1, SEQ_CST); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__atomic_fetch_xor." } */ + __atomic_fetch_or (a, 1, SEQ_CST); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__atomic_fetch_or." } */ + __atomic_fetch_nand (a, 1, SEQ_CST); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__atomic_fetch_nand." } */ + + __atomic_add_fetch (a, 1, SEQ_CST); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__atomic_add_fetch." } */ + __atomic_sub_fetch (a, 1, SEQ_CST); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__atomic_sub_fetch." } */ + __atomic_and_fetch (a, 1, SEQ_CST); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__atomic_and_fetch." } */ + __atomic_xor_fetch (a, 1, SEQ_CST); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__atomic_xor_fetch." } */ + __atomic_or_fetch (a, 1, SEQ_CST); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__atomic_or_fetch." } */ + __atomic_nand_fetch (a, 1, SEQ_CST); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__atomic_nand_fetch." } */ + + /* The following are valid and must be accepted. */ + _Bool val = 0, ret = 0; + __atomic_exchange (a, &val, &ret, SEQ_CST); + __atomic_exchange_n (a, val, SEQ_CST); + __atomic_compare_exchange (a, &val, &ret, !1, SEQ_CST, SEQ_CST); + __atomic_compare_exchange_n (a, &val, ret, !1, SEQ_CST, SEQ_CST); + __atomic_test_and_set (a, SEQ_CST); + __atomic_clear (a, SEQ_CST); +} + +void test_bool (_Bool *b) +{ + enum { SEQ_CST = __ATOMIC_SEQ_CST }; + + __atomic_fetch_add (b, 1, SEQ_CST); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__atomic_fetch_add." } */ + __atomic_fetch_sub (b, 1, SEQ_CST); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__atomic_fetch_sub." } */ + __atomic_fetch_and (b, 1, SEQ_CST); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__atomic_fetch_and." } */ + __atomic_fetch_xor (b, 1, SEQ_CST); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__atomic_fetch_xor." } */ + __atomic_fetch_or (b, 1, SEQ_CST); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__atomic_fetch_or." } */ + __atomic_fetch_nand (b, 1, SEQ_CST); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__atomic_fetch_nand." } */ + + __atomic_add_fetch (b, 1, SEQ_CST); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__atomic_add_fetch." } */ + __atomic_sub_fetch (b, 1, SEQ_CST); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__atomic_sub_fetch." } */ + __atomic_and_fetch (b, 1, SEQ_CST); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__atomic_and_fetch." } */ + __atomic_xor_fetch (b, 1, SEQ_CST); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__atomic_xor_fetch." } */ + __atomic_or_fetch (b, 1, SEQ_CST); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__atomic_or_fetch." } */ + __atomic_nand_fetch (b, 1, SEQ_CST); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__atomic_nand_fetch." } */ + + /* The following are valid and must be accepted. */ + _Bool val = 0, ret = 0; + __atomic_exchange (b, &val, &ret, SEQ_CST); + __atomic_exchange_n (b, val, SEQ_CST); + __atomic_compare_exchange (b, &val, &ret, !1, SEQ_CST, SEQ_CST); + __atomic_compare_exchange_n (b, &val, ret, !1, SEQ_CST, SEQ_CST); + __atomic_test_and_set (b, SEQ_CST); + __atomic_clear (b, SEQ_CST); +} 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 "-pedantic-errors -std=c11" } */ + + +void test_sync_atomic_bool (_Atomic _Bool *a) +{ + __sync_fetch_and_add (a, 1); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__sync_fetch_and_add." } */ + __sync_fetch_and_sub (a, 1); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__sync_fetch_and_sub." } */ + __sync_fetch_and_and (a, 1); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__sync_fetch_and_and." } */ + __sync_fetch_and_xor (a, 1); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__sync_fetch_and_xor." } */ + __sync_fetch_and_or (a, 1); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__sync_fetch_and_or." } */ + __sync_fetch_and_nand (a, 1); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__sync_fetch_and_nand." } */ + + __sync_add_and_fetch (a, 1); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__sync_add_and_fetch." } */ + __sync_sub_and_fetch (a, 1); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__sync_sub_and_fetch." } */ + __sync_and_and_fetch (a, 1); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__sync_and_and_fetch." } */ + __sync_xor_and_fetch (a, 1); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__sync_xor_and_fetch." } */ + __sync_or_and_fetch (a, 1); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__sync_or_and_fetch." } */ + __sync_nand_and_fetch (a, 1); /* { dg-error "operand type ._Atomic _Bool \\*. is incompatible with argument 1 of .__sync_nand_and_fetch." } */ + + /* The following are valid and must be accepted. */ + __sync_bool_compare_and_swap (a, 0, 1); + __sync_val_compare_and_swap (a, 0, 1); + __sync_lock_test_and_set (a, 1); + __sync_lock_release (a); +} + +void test_sync_bool (_Bool *b) +{ + __sync_fetch_and_add (b, 1); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__sync_fetch_and_add." } */ + __sync_fetch_and_sub (b, 1); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__sync_fetch_and_sub." } */ + __sync_fetch_and_and (b, 1); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__sync_fetch_and_and." } */ + __sync_fetch_and_xor (b, 1); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__sync_fetch_and_xor." } */ + __sync_fetch_and_or (b, 1); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__sync_fetch_and_or." } */ + __sync_fetch_and_nand (b, 1); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__sync_fetch_and_nand." } */ + + __sync_add_and_fetch (b, 1); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__sync_add_and_fetch." } */ + __sync_sub_and_fetch (b, 1); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__sync_sub_and_fetch." } */ + __sync_and_and_fetch (b, 1); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__sync_and_and_fetch." } */ + __sync_xor_and_fetch (b, 1); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__sync_xor_and_fetch." } */ + __sync_or_and_fetch (b, 1); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__sync_or_and_fetch." } */ + __sync_nand_and_fetch (b, 1); /* { dg-error "operand type ._Bool \\*. is incompatible with argument 1 of .__sync_nand_and_fetch." } */ + + /* The following are valid and must be accepted. */ + __sync_bool_compare_and_swap (b, 0, 1); + __sync_val_compare_and_swap (b, 0, 1); + __sync_lock_test_and_set (b, 1); + __sync_lock_release (b); +} --------------030800070003010908090402--