* [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed @ 2015-12-23 4:46 Martin Sebor 2016-01-02 7:43 ` Jeff Law 2016-03-21 12:35 ` [PR c/68966] Restore atomic builtins usage in libstdc++-v3 (was: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed) Thomas Schwinge 0 siblings, 2 replies; 18+ messages in thread From: Martin Sebor @ 2015-12-23 4:46 UTC (permalink / raw) To: Gcc Patch List, Marek Polacek, Joseph S. Myers [-- Attachment #1: Type: text/plain, Size: 164 bytes --] The attached patch rejects invocations of atomic fetch_op intrinsics on objects of _Bool type as discussed in the context of PR c/68908. Tested on x86_64. Martin [-- Attachment #2: gcc-68966.patch --] [-- Type: text/x-patch, Size: 10032 bytes --] gcc/testsuite/ChangeLog: 2015-12-22 Martin Sebor <msebor@redhat.com> PR c/68966 * gcc.dg/atomic-fetch-bool.c: New test. * gcc.dg/sync-fetch-bool.c: Same. gcc/ChangeLog: 2015-12-22 Martin Sebor <msebor@redhat.com> PR c/68966 * doc/extend.texi (__atomic Builtins, __sync Builtins): Document constraint on the type of arguments. gcc/c-family/ChangeLog: 2015-12-22 Martin Sebor <msebor@redhat.com> PR c/68966 * c-common.c (sync_resolve_size): Reject first argument when it's a pointer to _Bool. Index: gcc/c-family/c-common.c =================================================================== --- gcc/c-family/c-common.c (revision 231903) +++ gcc/c-family/c-common.c (working copy) @@ -7667,6 +7667,6 @@ if (error_operand_p (align)) return -1; if (TREE_CODE (align) != INTEGER_CST || !INTEGRAL_TYPE_P (TREE_TYPE (align))) { @@ -10657,11 +10658,15 @@ /* 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_ built-ins. */ static int -sync_resolve_size (tree function, vec<tree, va_gc> *params) +sync_resolve_size (tree function, vec<tree, va_gc> *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,9 @@ vec<tree, va_gc> *params) { enum built_in_function orig_code = DECL_FUNCTION_CODE (function); + + /* Is function is one of the _FETCH_ built-ins? */ + bool fetch_op = true; bool orig_format = true; tree new_return = NULL_TREE; @@ -11325,6 +11337,9 @@ case BUILT_IN_ATOMIC_COMPARE_EXCHANGE_N: case BUILT_IN_ATOMIC_LOAD_N: case BUILT_IN_ATOMIC_STORE_N: + { + fetch_op = false; + } case BUILT_IN_ATOMIC_ADD_FETCH_N: case BUILT_IN_ATOMIC_SUB_FETCH_N: case BUILT_IN_ATOMIC_AND_FETCH_N: @@ -11358,7 +11373,7 @@ case BUILT_IN_SYNC_LOCK_TEST_AND_SET_N: case BUILT_IN_SYNC_LOCK_RELEASE_N: { - int n = sync_resolve_size (function, params); + int n = sync_resolve_size (function, params, fetch_op); tree new_function, first_param, result; enum built_in_function fncode; Index: gcc/doc/extend.texi =================================================================== --- gcc/doc/extend.texi (revision 231903) +++ gcc/doc/extend.texi (working copy) @@ -9246,6 +9246,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. + @emph{Note:} GCC 4.4 and later implement @code{__sync_fetch_and_nand} as @code{*ptr = ~(tmp & value)} instead of @code{*ptr = ~tmp & value}. @@ -9269,6 +9271,8 @@ @{ *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}. @@ -9515,13 +9519,13 @@ @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 of integer or pointer type. It must not be a Boolean type. All memory orders are valid. @end deftypefn @@ -9538,7 +9542,7 @@ @{ 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/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,26 @@ +/* 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" } */ + + +void test_atomics (_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." } */ +} 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,24 @@ +/* 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" } */ + + +void test_sync (_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." } */ +} ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed 2015-12-23 4:46 [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed Martin Sebor @ 2016-01-02 7:43 ` Jeff Law 2016-01-04 3:03 ` Martin Sebor 2016-03-21 12:35 ` [PR c/68966] Restore atomic builtins usage in libstdc++-v3 (was: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed) Thomas Schwinge 1 sibling, 1 reply; 18+ messages in thread From: Jeff Law @ 2016-01-02 7:43 UTC (permalink / raw) To: Martin Sebor, Gcc Patch List, Marek Polacek, Joseph S. Myers On 12/22/2015 09:46 PM, Martin Sebor wrote: > The attached patch rejects invocations of atomic fetch_op intrinsics > on objects of _Bool type as discussed in the context of PR c/68908. > > Tested on x86_64. > > Martin > > gcc-68966.patch > > > gcc/testsuite/ChangeLog: > 2015-12-22 Martin Sebor<msebor@redhat.com> > > PR c/68966 > * gcc.dg/atomic-fetch-bool.c: New test. > * gcc.dg/sync-fetch-bool.c: Same. > > gcc/ChangeLog: > 2015-12-22 Martin Sebor<msebor@redhat.com> > > PR c/68966 > * doc/extend.texi (__atomic Builtins, __sync Builtins): Document > constraint on the type of arguments. > > gcc/c-family/ChangeLog: > 2015-12-22 Martin Sebor<msebor@redhat.com> > > PR c/68966 > * c-common.c (sync_resolve_size): Reject first argument when it's > a pointer to _Bool. > > Index: gcc/c-family/c-common.c > =================================================================== > --- gcc/c-family/c-common.c (revision 231903) > +++ gcc/c-family/c-common.c (working copy) > @@ -7667,6 +7667,6 @@ > > if (error_operand_p (align)) > return -1; > if (TREE_CODE (align) != INTEGER_CST > || !INTEGRAL_TYPE_P (TREE_TYPE (align))) > { No changes in this hunk. I assume you hand-edited the patch to remove something that you really didn't intend to submit and left the above useless hunk. The patch itself is fine for the trunk. Thanks, Jeff ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed 2016-01-02 7:43 ` Jeff Law @ 2016-01-04 3:03 ` Martin Sebor 2016-01-04 15:22 ` Marek Polacek 0 siblings, 1 reply; 18+ messages in thread From: Martin Sebor @ 2016-01-04 3:03 UTC (permalink / raw) To: Jeff Law, Gcc Patch List, Marek Polacek, Joseph S. Myers [-- Attachment #1: Type: text/plain, Size: 1121 bytes --] ... >> Index: gcc/c-family/c-common.c >> =================================================================== >> --- gcc/c-family/c-common.c (revision 231903) >> +++ gcc/c-family/c-common.c (working copy) >> @@ -7667,6 +7667,6 @@ >> >> if (error_operand_p (align)) >> return -1; >> if (TREE_CODE (align) != INTEGER_CST >> || !INTEGRAL_TYPE_P (TREE_TYPE (align))) >> { > No changes in this hunk. I assume you hand-edited the patch to remove > something that you really didn't intend to submit and left the above > useless hunk. That's right. It doesn't look like Emacs Diff mode is smart enough to handle these types of edits. I've removed the hunk from the updated patch. > The patch itself is fine for the trunk. Thanks. Unfortunately, I had missed a set of important use cases that are apparently not exercised by the test suite: __sync_bool_compare_and_swap, __sync_val_compare_and_swap, __sync_lock_test_and_set, and __sync_lock_release are valid for _Bool and must not be rejected. I've corrected the handling of those in the attached patch and added tests for them. Martin [-- Attachment #2: gcc-68966.patch --] [-- Type: text/x-patch, Size: 15065 bytes --] gcc/ChangeLog: 2016-01-03 Martin Sebor <msebor@redhat.com> PR c/68966 * doc/extend.texi (__atomic Builtins, __sync Builtins): Document constraint on the type of arguments. gcc/c-family/ChangeLog: 2016-01-03 Martin Sebor <msebor@redhat.com> PR c/68966 * c-common.c (sync_resolve_size): Reject first argument when it's a pointer to _Bool. gcc/testsuite/ChangeLog: 2016-01-03 Martin Sebor <msebor@redhat.com> 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,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. + @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 +9263,8 @@ @{ *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 +9511,13 @@ @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 of integer or pointer type. It must not be a Boolean type. All memory orders are valid. @end deftypefn @@ -9530,7 +9534,7 @@ @{ 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) @@ -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<tree, va_gc> *params) +sync_resolve_size (tree function, vec<tree, va_gc> *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<tree, va_gc> *params) { enum built_in_function orig_code = DECL_FUNCTION_CODE (function); + + /* Is function is 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,9 @@ case BUILT_IN_ATOMIC_COMPARE_EXCHANGE_N: case BUILT_IN_ATOMIC_LOAD_N: case BUILT_IN_ATOMIC_STORE_N: + { + fetch_op = false; + } case BUILT_IN_ATOMIC_ADD_FETCH_N: case BUILT_IN_ATOMIC_SUB_FETCH_N: case BUILT_IN_ATOMIC_AND_FETCH_N: @@ -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; + + 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 "-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 "-std=c99" } */ + + +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); +} ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed 2016-01-04 3:03 ` Martin Sebor @ 2016-01-04 15:22 ` Marek Polacek 2016-01-05 1:18 ` Martin Sebor 0 siblings, 1 reply; 18+ messages in thread From: Marek Polacek @ 2016-01-04 15:22 UTC (permalink / raw) To: Martin Sebor; +Cc: Jeff Law, Gcc Patch List, Joseph S. Myers 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<tree, va_gc> *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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed 2016-01-04 15:22 ` Marek Polacek @ 2016-01-05 1:18 ` Martin Sebor 2016-01-05 10:51 ` Marek Polacek 0 siblings, 1 reply; 18+ messages in thread From: Martin Sebor @ 2016-01-05 1:18 UTC (permalink / raw) To: Marek Polacek; +Cc: Jeff Law, Gcc Patch List, Joseph S. Myers [-- Attachment #1: Type: text/plain, Size: 854 bytes --] 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 [-- Attachment #2: gcc-68966.patch --] [-- Type: text/x-patch, Size: 15473 bytes --] gcc/ChangeLog: 2016-01-04 Martin Sebor <msebor@redhat.com> 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 <msebor@redhat.com> 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 <msebor@redhat.com> 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<tree, va_gc> *params) +sync_resolve_size (tree function, vec<tree, va_gc> *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<tree, va_gc> *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); +} ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed 2016-01-05 1:18 ` Martin Sebor @ 2016-01-05 10:51 ` Marek Polacek 2016-01-05 18:47 ` Martin Sebor 0 siblings, 1 reply; 18+ messages in thread From: Marek Polacek @ 2016-01-05 10:51 UTC (permalink / raw) To: Martin Sebor; +Cc: Jeff Law, Gcc Patch List, Joseph S. Myers On Mon, Jan 04, 2016 at 06:18:32PM -0700, Martin Sebor wrote: > 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? I agree that the rules are sometimes tedious to follow (and rebasing patches just to fix some formatting issues isn't exactly fun). I don't use git to commit either. My "secret" is to enable highlighting of trailing whitespaces in vim ("let c_space_errors=1"), but that's of no use to you I guess. But there's this contrib/check_GNU_style.sh script that you can use, which points out e.g. 8 spaces -> tabs. Before posting a patch, I always do 'check_GNU_style z.patch'. That might help. > 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) This change is unrelated. Marek ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed 2016-01-05 10:51 ` Marek Polacek @ 2016-01-05 18:47 ` Martin Sebor 2016-01-06 11:50 ` Marek Polacek 0 siblings, 1 reply; 18+ messages in thread From: Martin Sebor @ 2016-01-05 18:47 UTC (permalink / raw) To: Marek Polacek; +Cc: Jeff Law, Gcc Patch List, Joseph S. Myers [-- Attachment #1: Type: text/plain, Size: 1870 bytes --] > I agree that the rules are sometimes tedious to follow (and rebasing > patches just to fix some formatting issues isn't exactly fun). I don't > use git to commit either. My "secret" is to enable highlighting of trailing > whitespaces in vim ("let c_space_errors=1"), but that's of no use to you > I guess. Thanks. I just downloaded and installed an Emacs equivalent called redspace. It does the same thing. Unfortunately, it also has the effect of highlighting in red the one blank inserted by diff at the beginning of empty lines. I guess the way to deal with that is to avoid inserting blank spaces into changes to begin with rather than trying to remove them from the final patch. > But there's this contrib/check_GNU_style.sh script that you can use, > which points out e.g. 8 spaces -> tabs. Before posting a patch, I always do > 'check_GNU_style z.patch'. > That might help. I use the script on bigger patches. I just haven't trained myself to use it consistently, for all of them, and each time I post a new revision of a patch for review. (The script also complains about tests where following the convention isn't always possible -- e.g., lines over 80 characters are unavoidable due to dg-error directives, and periods can't be preceded by a space when they are part of regular expressions in dg- directives..) But what I'm hoping to find is a fully automated way of dealing with this so I don't have to think about and waste other people's time pointing it out. (Ideally, it would be automated for everyone so that none of us have to worry about it. Perhaps a post-commit hook could be put in place for this.) > >> Index: gcc/c-family/c-common.c ... > > This change is unrelated. Yes, sorry about that. It's yet another weakness in my workflow (using the same local copy to test multiple unrelated changes). A new patch is attached. Martin [-- Attachment #2: gcc-68966.patch --] [-- Type: text/x-patch, Size: 15149 bytes --] gcc/ChangeLog: 2016-01-04 Martin Sebor <msebor@redhat.com> 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 <msebor@redhat.com> 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 <msebor@redhat.com> 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 232069) +++ 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 232069) +++ gcc/c-family/c-common.c (working copy) @@ -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<tree, va_gc> *params) +sync_resolve_size (tree function, vec<tree, va_gc> *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<tree, va_gc> *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); +} ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed 2016-01-05 18:47 ` Martin Sebor @ 2016-01-06 11:50 ` Marek Polacek 2016-01-06 23:38 ` Martin Sebor 0 siblings, 1 reply; 18+ messages in thread From: Marek Polacek @ 2016-01-06 11:50 UTC (permalink / raw) To: Martin Sebor; +Cc: Jeff Law, Gcc Patch List, Joseph S. Myers On Tue, Jan 05, 2016 at 11:46:52AM -0700, Martin Sebor wrote: > I just downloaded and installed an Emacs equivalent called redspace. > It does the same thing. Unfortunately, it also has the effect of > highlighting in red the one blank inserted by diff at the beginning > of empty lines. I guess the way to deal with that is to avoid > inserting blank spaces into changes to begin with rather than trying > to remove them from the final patch. Maybe some Emacs user can offer some advice (but we're slightly off topic here ;). > I use the script on bigger patches. I just haven't trained myself > to use it consistently, for all of them, and each time I post a new > revision of a patch for review. (The script also complains about I have a script called "P" that generates a patch, runs an ancient version of mklog, adds "PR c/#" etc. and finally it runs check_GNU_style so that I won't forget. > tests where following the convention isn't always possible -- e.g., > lines over 80 characters are unavoidable due to dg-error directives, > and periods can't be preceded by a space when they are part of > regular expressions in dg- directives..) Yes, you need to mentally filter out the output for tests. > gcc/testsuite/ChangeLog: > 2016-01-04 Martin Sebor <msebor@redhat.com> > > PR c/68966 > * gcc.dg/atomic-fetch-bool.c: New test. > * gcc.dg/sync-fetch-bool.c: Same. So the tradition is to repeat "New test." rather than to say "Same." Otherwise ok, thanks. Marek ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed 2016-01-06 11:50 ` Marek Polacek @ 2016-01-06 23:38 ` Martin Sebor 2016-01-07 0:08 ` Mike Stump 2016-01-07 12:07 ` Marek Polacek 0 siblings, 2 replies; 18+ messages in thread From: Martin Sebor @ 2016-01-06 23:38 UTC (permalink / raw) To: Marek Polacek; +Cc: Jeff Law, Gcc Patch List, Joseph S. Myers >> gcc/testsuite/ChangeLog: >> 2016-01-04 Martin Sebor <msebor@redhat.com> >> >> PR c/68966 >> * gcc.dg/atomic-fetch-bool.c: New test. >> * gcc.dg/sync-fetch-bool.c: Same. > > So the tradition is to repeat "New test." rather than to say "Same." Can we try not to make the rules any more rigid than they need to be? As we just discussed, following the required formatting rules is error-prone and time-consuming enough. GCC's own Coding Conventions doesn't even require ChangeLog entries for new tests (https://gcc.gnu.org/codingconventions.html#ChangeLogs). People have been adding them and that's just fine with me, but I can't discern any established convention when it comes to the comment when a new test is being added. I see examples of "New test" or "New file" followed by "Likewise" or "Ditto" as well as "New test" followed by "Same". I see no point in adding yet another hoop for people to have to remember to jump through. Thanks Martin > > Otherwise ok, thanks. > > Marek > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed 2016-01-06 23:38 ` Martin Sebor @ 2016-01-07 0:08 ` Mike Stump 2016-01-07 12:07 ` Marek Polacek 1 sibling, 0 replies; 18+ messages in thread From: Mike Stump @ 2016-01-07 0:08 UTC (permalink / raw) To: Martin Sebor; +Cc: Marek Polacek, Jeff Law, Gcc Patch List, Joseph S. Myers On Jan 6, 2016, at 3:38 PM, Martin Sebor <msebor@gmail.com> wrote: >>> gcc/testsuite/ChangeLog: >>> 2016-01-04 Martin Sebor <msebor@redhat.com> >>> >>> PR c/68966 >>> * gcc.dg/atomic-fetch-bool.c: New test. >>> * gcc.dg/sync-fetch-bool.c: Same. >> >> So the tradition is to repeat "New test." rather than to say "Same." > > Can we try not to make the rules any more rigid than they need > to be? As we just discussed, following the required formatting > rules is error-prone and time-consuming enough. GCC's own > Coding Conventions doesn't even require ChangeLog entries for > new tests (https://gcc.gnu.org/codingconventions.html#ChangeLogs). > People have been adding them and that's just fine with me, but > I can't discern any established convention when it comes to the > comment when a new test is being added. I see examples of "New > test" or "New file" followed by "Likewise" or "Ditto" as well > as "New test" followed by "Same". I see no point in adding yet > another hoop for people to have to remember to jump through. And I thought it was Likewise. :-) $ grep Same *ChangeLog* | wc -l 3938 $ grep Likewise *ChangeLog* | wc -l 69444 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed 2016-01-06 23:38 ` Martin Sebor 2016-01-07 0:08 ` Mike Stump @ 2016-01-07 12:07 ` Marek Polacek 2016-01-08 1:00 ` Martin Sebor 1 sibling, 1 reply; 18+ messages in thread From: Marek Polacek @ 2016-01-07 12:07 UTC (permalink / raw) To: Martin Sebor; +Cc: Jeff Law, Gcc Patch List, Joseph S. Myers On Wed, Jan 06, 2016 at 04:38:20PM -0700, Martin Sebor wrote: > >>gcc/testsuite/ChangeLog: > >>2016-01-04 Martin Sebor <msebor@redhat.com> > >> > >> PR c/68966 > >> * gcc.dg/atomic-fetch-bool.c: New test. > >> * gcc.dg/sync-fetch-bool.c: Same. > > > >So the tradition is to repeat "New test." rather than to say "Same." > > Can we try not to make the rules any more rigid than they need > to be? As we just discussed, following the required formatting > rules is error-prone and time-consuming enough. GCC's own > Coding Conventions doesn't even require ChangeLog entries for > new tests (https://gcc.gnu.org/codingconventions.html#ChangeLogs). > People have been adding them and that's just fine with me, but > I can't discern any established convention when it comes to the > comment when a new test is being added. I see examples of "New > test" or "New file" followed by "Likewise" or "Ditto" as well > as "New test" followed by "Same". I see no point in adding yet > another hoop for people to have to remember to jump through. Sorry, I didn't mean to nitpick; I just thought it's something worth pointing out, mostly for the future. But it's not like I actually care as long as there's something reasonable in the CL entry :). And, as you noticed, you don't have to bother with testsuite CLs at all, and that's fine with me as well. Marek ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed 2016-01-07 12:07 ` Marek Polacek @ 2016-01-08 1:00 ` Martin Sebor 0 siblings, 0 replies; 18+ messages in thread From: Martin Sebor @ 2016-01-08 1:00 UTC (permalink / raw) To: Marek Polacek; +Cc: Jeff Law, Gcc Patch List, Joseph S. Myers On 01/07/2016 05:07 AM, Marek Polacek wrote: > On Wed, Jan 06, 2016 at 04:38:20PM -0700, Martin Sebor wrote: >>>> gcc/testsuite/ChangeLog: >>>> 2016-01-04 Martin Sebor <msebor@redhat.com> >>>> >>>> PR c/68966 >>>> * gcc.dg/atomic-fetch-bool.c: New test. >>>> * gcc.dg/sync-fetch-bool.c: Same. >>> >>> So the tradition is to repeat "New test." rather than to say "Same." >> >> Can we try not to make the rules any more rigid than they need >> to be? As we just discussed, following the required formatting >> rules is error-prone and time-consuming enough. GCC's own >> Coding Conventions doesn't even require ChangeLog entries for >> new tests (https://gcc.gnu.org/codingconventions.html#ChangeLogs). >> People have been adding them and that's just fine with me, but >> I can't discern any established convention when it comes to the >> comment when a new test is being added. I see examples of "New >> test" or "New file" followed by "Likewise" or "Ditto" as well >> as "New test" followed by "Same". I see no point in adding yet >> another hoop for people to have to remember to jump through. > > Sorry, I didn't mean to nitpick; I just thought it's something worth > pointing out, mostly for the future. No problem. It's easy enough to automate so I've modified my ChangeLog script to print "New test." for each newly added test. (I can't promise to use it consistently, though :) Martin ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PR c/68966] Restore atomic builtins usage in libstdc++-v3 (was: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed) 2015-12-23 4:46 [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed Martin Sebor 2016-01-02 7:43 ` Jeff Law @ 2016-03-21 12:35 ` Thomas Schwinge 2016-03-21 15:12 ` Jonathan Wakely 1 sibling, 1 reply; 18+ messages in thread From: Thomas Schwinge @ 2016-03-21 12:35 UTC (permalink / raw) To: Martin Sebor, libstdc++, gcc-patches Cc: Marek Polacek, Joseph S. Myers, Jeff Law [-- Attachment #1: Type: text/plain, Size: 8115 bytes --] Hi! On Tue, 22 Dec 2015 21:46:19 -0700, Martin Sebor <msebor@gmail.com> wrote: > The attached patch rejects invocations of atomic fetch_op intrinsics > on objects of _Bool type as discussed in the context of PR c/68908. > > Tested on x86_64. I'd noticed something "strange". ;-) For reference: > --- gcc/c-family/c-common.c (revision 231903) > +++ gcc/c-family/c-common.c (working copy) > 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; > } When comparing GCC build logs before/after your change got committed (r232147), libstdc++-v3 shows the following configure-time differences: -checking for atomic builtins for bool... yes +checking for atomic builtins for bool... no checking for atomic builtins for short... yes checking for atomic builtins for int... yes checking for atomic builtins for long long... yes -ln -s [...]/source-gcc/libstdc++-v3/config/cpu/generic/atomicity_builtins/atomicity.h- ./atomicity.cc || true +ln -s [...]/source-gcc/libstdc++-v3/config/cpu/i486/atomicity.h ./atomicity.cc || true -ATOMICITY_SRCDIR = config/cpu/generic/atomicity_builtins +ATOMICITY_SRCDIR = config/cpu/i486 /* Define if the compiler supports C++11 atomics. */ -#define _GLIBCXX_ATOMIC_BUILTINS 1 +/* #undef _GLIBCXX_ATOMIC_BUILTINS */ Per the new BOOLEAN_TYPE checking cited above, the following test now is -- expectedly -- failing: configure:15211: checking for atomic builtins for bool configure:15240: [...]/build-gcc/./gcc/xgcc -shared-libgcc -B[...]/build-gcc/./gcc -nostdinc++ -L[...]/build-gcc/x86_64-pc-linux-gnu/libstdc++-v3/src -L[...]/build-gcc/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs -L[...]/build-gcc/x86_64-pc-linux-gnu/libstdc++-v3/libsupc++/.libs -B/x86_64-pc-linux-gnu/bin/ -B/x86_64-pc-linux-gnu/lib/ -isystem /x86_64-pc-linux-gnu/include -isystem /x86_64-pc-linux-gnu/sys-include -o conftest -g -O2 -D_GNU_SOURCE -fno-exceptions conftest.cpp >&5 conftest.cpp: In function 'int main()': conftest.cpp:31:52: error: operand type 'atomic_type* {aka bool*}' is incompatible with argument 1 of '__atomic_fetch_add' __atomic_fetch_add(&c1, c2, __ATOMIC_RELAXED); ^ configure:15240: $? = 1 configure: failed program was: | [...] | int | main () | { | typedef bool atomic_type; | atomic_type c1; | atomic_type c2; | atomic_type c3(0); | __atomic_fetch_add(&c1, c2, __ATOMIC_RELAXED); | __atomic_compare_exchange_n(&c1, &c2, c3, true, __ATOMIC_ACQ_REL, | __ATOMIC_RELAXED); | __atomic_test_and_set(&c1, __ATOMIC_RELAXED); | __atomic_load_n(&c1, __ATOMIC_RELAXED); | | ; | return 0; | } configure:15250: result: no The other data types still work fine: configure:15253: checking for atomic builtins for short configure:15282: [...]/build-gcc/./gcc/xgcc -shared-libgcc -B[...]/build-gcc/./gcc -nostdinc++ -L[...]/build-gcc/x86_64-pc-linux-gnu/libstdc++-v3/src -L[...]/build-gcc/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs -L[...]/build-gcc/x86_64-pc-linux-gnu/libstdc++-v3/libsupc++/.libs -B/x86_64-pc-linux-gnu/bin/ -B/x86_64-pc-linux-gnu/lib/ -isystem /x86_64-pc-linux-gnu/include -isystem /x86_64-pc-linux-gnu/sys-include -o conftest -g -O2 -D_GNU_SOURCE -fno-exceptions conftest.cpp >&5 configure:15282: $? = 0 configure:15292: result: yes [same for int and long long] Per my (admittedly, not in-depth) reading of libstdc++-v3 source code, the _GLIBCXX_ATOMIC_BUILTINS conditional is only used in combination with the _Atomic_word data type, which in libstdc++-v3/doc/xml/manual/concurrency_extensions.xml is described as "a signed integral type" (so, matching the semantics as clarified by your patch). That makes sense: it's used to keep reference counts, for example. So, it seems sound to just remove the bool atomics check. That said, I have not looked into why the libstdc++-v3 configure script checks short, int, and long long atomics, but not long. But then, only the short and int results are used to decide on _GLIBCXX_ATOMIC_BUILTINS, and on the other hand there actually are "typedef long _Atomic_word" definitions, but long atomics are not explicitly checked for. OK to commit to following? commit 134784da2f0274b194bfd53264af173d5c5920d4 Author: Thomas Schwinge <thomas@codesourcery.com> Date: Mon Mar 21 11:59:03 2016 +0100 [PR c/68966] Restore atomic builtins usage in libstdc++-v3 libstdc++-v3/ PR c/68966 * acinclude.m4 (GLIBCXX_ENABLE_ATOMIC_BUILTINS) <bool>: Remove checks. * configure: Regenerate. --- libstdc++-v3/acinclude.m4 | 51 +------------------------- libstdc++-v3/configure | 92 ++++------------------------------------------- 2 files changed, 8 insertions(+), 135 deletions(-) diff --git libstdc++-v3/acinclude.m4 libstdc++-v3/acinclude.m4 index 95df24a..145d0f9 100644 --- libstdc++-v3/acinclude.m4 +++ libstdc++-v3/acinclude.m4 @@ -3282,25 +3282,6 @@ AC_DEFUN([GLIBCXX_ENABLE_ATOMIC_BUILTINS], [ CXXFLAGS="$CXXFLAGS -fno-exceptions" - AC_MSG_CHECKING([for atomic builtins for bool]) - AC_CACHE_VAL(glibcxx_cv_atomic_bool, [ - AC_TRY_LINK( - [ ], - [typedef bool atomic_type; - atomic_type c1; - atomic_type c2; - atomic_type c3(0); - __atomic_fetch_add(&c1, c2, __ATOMIC_RELAXED); - __atomic_compare_exchange_n(&c1, &c2, c3, true, __ATOMIC_ACQ_REL, - __ATOMIC_RELAXED); - __atomic_test_and_set(&c1, __ATOMIC_RELAXED); - __atomic_load_n(&c1, __ATOMIC_RELAXED); - ], - [glibcxx_cv_atomic_bool=yes], - [glibcxx_cv_atomic_bool=no]) - ]) - AC_MSG_RESULT($glibcxx_cv_atomic_bool) - AC_MSG_CHECKING([for atomic builtins for short]) AC_CACHE_VAL(glibcxx_cv_atomic_short, [ AC_TRY_LINK( @@ -3371,35 +3352,6 @@ AC_DEFUN([GLIBCXX_ENABLE_ATOMIC_BUILTINS], [ [#]line __oline__ "configure" int main() { - typedef bool atomic_type; - atomic_type c1; - atomic_type c2; - atomic_type c3(0); - __atomic_fetch_add(&c1, c2, __ATOMIC_RELAXED); - __atomic_compare_exchange_n(&c1, &c2, c3, true, __ATOMIC_ACQ_REL, - __ATOMIC_RELAXED); - __atomic_test_and_set(&c1, __ATOMIC_RELAXED); - __atomic_load_n(&c1, __ATOMIC_RELAXED); - - return 0; -} -EOF - - AC_MSG_CHECKING([for atomic builtins for bool]) - if AC_TRY_EVAL(ac_compile); then - if grep __atomic_ conftest.s >/dev/null 2>&1 ; then - glibcxx_cv_atomic_bool=no - else - glibcxx_cv_atomic_bool=yes - fi - fi - AC_MSG_RESULT($glibcxx_cv_atomic_bool) - rm -f conftest* - - cat > conftest.$ac_ext << EOF -[#]line __oline__ "configure" -int main() -{ typedef short atomic_type; atomic_type c1; atomic_type c2; @@ -3490,8 +3442,7 @@ EOF AC_LANG_RESTORE # Set atomicity_dir to builtins if all but the long long test above passes. - if test "$glibcxx_cv_atomic_bool" = yes \ - && test "$glibcxx_cv_atomic_short" = yes \ + if test "$glibcxx_cv_atomic_short" = yes \ && test "$glibcxx_cv_atomic_int" = yes; then AC_DEFINE(_GLIBCXX_ATOMIC_BUILTINS, 1, [Define if the compiler supports C++11 atomics.]) diff --git libstdc++-v3/configure libstdc++-v3/configure index acbc6a6..6f6f1d3 100755 --- libstdc++-v3/configure +++ libstdc++-v3/configure [...] Grüße Thomas [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 472 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PR c/68966] Restore atomic builtins usage in libstdc++-v3 (was: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed) 2016-03-21 12:35 ` [PR c/68966] Restore atomic builtins usage in libstdc++-v3 (was: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed) Thomas Schwinge @ 2016-03-21 15:12 ` Jonathan Wakely 2016-03-21 16:04 ` [PR c/68966] Restore atomic builtins usage in libstdc++-v3 Thomas Schwinge 0 siblings, 1 reply; 18+ messages in thread From: Jonathan Wakely @ 2016-03-21 15:12 UTC (permalink / raw) To: Thomas Schwinge Cc: Martin Sebor, libstdc++, gcc-patches, Marek Polacek, Joseph S. Myers, Jeff Law On 21/03/16 13:08 +0100, Thomas Schwinge wrote: >Per my (admittedly, not in-depth) reading of libstdc++-v3 source code, >the _GLIBCXX_ATOMIC_BUILTINS conditional is only used in combination with >the _Atomic_word data type, which in >libstdc++-v3/doc/xml/manual/concurrency_extensions.xml is described as "a >signed integral type" (so, matching the semantics as clarified by your >patch). That makes sense: it's used to keep reference counts, for >example. So, it seems sound to just remove the bool atomics check. I agree that it doesn't make any sense to check whether atomics work for bool when we only care about them for _Atomic_word, however ... This would change the value of _GLIBCXX_ATOMIC_BUILTINS for any target which was already failing the check for bool but passing it for the other types. We would now switch to using atomic builtins where we previously didn't use them, which could be a problem. I don't know if there are any targets that would be affected, and if it would cause an actual problem. Would leaving the bool check in place, but just removing the __atomic_fetch_add() part be better? It should still fix the regression, but is less likely to change behaviour for targets that were never using the builtins. >That said, I have not looked into why the libstdc++-v3 configure script >checks short, int, and long long atomics, but not long. But then, only >the short and int results are used to decide on _GLIBCXX_ATOMIC_BUILTINS, >and on the other hand there actually are "typedef long _Atomic_word" >definitions, but long atomics are not explicitly checked for. Ugh. Those checks have a long and messy history. If we're now only using _GLIBCXX_ATOMIC_BUILTINS to decide how to operate on _Atomic_word then it should really only be testing whether atomics are supported for that type. I'm not brave enough to change that now though. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PR c/68966] Restore atomic builtins usage in libstdc++-v3 2016-03-21 15:12 ` Jonathan Wakely @ 2016-03-21 16:04 ` Thomas Schwinge 2016-04-05 11:01 ` Jonathan Wakely 0 siblings, 1 reply; 18+ messages in thread From: Thomas Schwinge @ 2016-03-21 16:04 UTC (permalink / raw) To: Jonathan Wakely Cc: Martin Sebor, libstdc++, gcc-patches, Marek Polacek, Joseph S. Myers, Jeff Law Hi! On Mon, 21 Mar 2016 15:01:49 +0000, Jonathan Wakely <jwakely@redhat.com> wrote: > On 21/03/16 13:08 +0100, Thomas Schwinge wrote: > >Per my (admittedly, not in-depth) reading of libstdc++-v3 source code, > >the _GLIBCXX_ATOMIC_BUILTINS conditional is only used in combination with > >the _Atomic_word data type, which in > >libstdc++-v3/doc/xml/manual/concurrency_extensions.xml is described as "a > >signed integral type" (so, matching the semantics as clarified by your > >patch). That makes sense: it's used to keep reference counts, for > >example. So, it seems sound to just remove the bool atomics check. > > I agree that it doesn't make any sense to check whether atomics work > for bool when we only care about them for _Atomic_word, however ... (Please review that it really is used only for that; I have only done a quick scan of the libstdc++-v3 sources.) > This would change the value of _GLIBCXX_ATOMIC_BUILTINS for any target > which was already failing the check for bool but passing it for the > other types. We would now switch to using atomic builtins where we > previously didn't use them, which could be a problem. I don't know if > there are any targets that would be affected, and if it would cause an > actual problem. Assuming there are no other reasons that could have caused the bool atomics checks to fail (under the condition that the short and int ones did and still do succeed), my patch just restores the state of a few months ago, before Martin's bool atomics warning patch got committed. So, I think it is safe to commit. > Would leaving the bool check in place, but just removing the > __atomic_fetch_add() part be better? It should still fix the > regression, but is less likely to change behaviour for targets that > were never using the builtins. Yes, we could do that, but while I have not verified this, I assume that it's very unlikely that there exists a configuration where the bool atomics checks already used to fail but the short and int ones did and still do succeed. Anyway, that's not my decision to make. ;-) > >That said, I have not looked into why the libstdc++-v3 configure script > >checks short, int, and long long atomics, but not long. But then, only > >the short and int results are used to decide on _GLIBCXX_ATOMIC_BUILTINS, > >and on the other hand there actually are "typedef long _Atomic_word" > >definitions, but long atomics are not explicitly checked for. > > Ugh. Those checks have a long and messy history. > > If we're now only using _GLIBCXX_ATOMIC_BUILTINS to decide how to > operate on _Atomic_word then it should really only be testing whether > atomics are supported for that type. I'm not brave enough to change > that now though. "Don't shoot the messenger", who quickly runs away to stay out of this mess. ;-) Grüße Thomas ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PR c/68966] Restore atomic builtins usage in libstdc++-v3 2016-03-21 16:04 ` [PR c/68966] Restore atomic builtins usage in libstdc++-v3 Thomas Schwinge @ 2016-04-05 11:01 ` Jonathan Wakely 2016-04-05 18:08 ` Jonathan Wakely 0 siblings, 1 reply; 18+ messages in thread From: Jonathan Wakely @ 2016-04-05 11:01 UTC (permalink / raw) To: Thomas Schwinge Cc: Martin Sebor, libstdc++, gcc-patches, Marek Polacek, Joseph S. Myers, Jeff Law On 21/03/16 17:01 +0100, Thomas Schwinge wrote: >Hi! > >On Mon, 21 Mar 2016 15:01:49 +0000, Jonathan Wakely <jwakely@redhat.com> wrote: >> On 21/03/16 13:08 +0100, Thomas Schwinge wrote: >> >Per my (admittedly, not in-depth) reading of libstdc++-v3 source code, >> >the _GLIBCXX_ATOMIC_BUILTINS conditional is only used in combination with >> >the _Atomic_word data type, which in >> >libstdc++-v3/doc/xml/manual/concurrency_extensions.xml is described as "a >> >signed integral type" (so, matching the semantics as clarified by your >> >patch). That makes sense: it's used to keep reference counts, for >> >example. So, it seems sound to just remove the bool atomics check. >> >> I agree that it doesn't make any sense to check whether atomics work >> for bool when we only care about them for _Atomic_word, however ... > >(Please review that it really is used only for that; I have only done a >quick scan of the libstdc++-v3 sources.) My own checking agreed. >> This would change the value of _GLIBCXX_ATOMIC_BUILTINS for any target >> which was already failing the check for bool but passing it for the >> other types. We would now switch to using atomic builtins where we >> previously didn't use them, which could be a problem. I don't know if >> there are any targets that would be affected, and if it would cause an >> actual problem. > >Assuming there are no other reasons that could have caused the bool >atomics checks to fail A target without 1-byte atomics might fail the bool checks, but pass the int and short ones. >(under the condition that the short and int ones >did and still do succeed), my patch just restores the state of a few >months ago, before Martin's bool atomics warning patch got committed. >So, I think it is safe to commit. > >> Would leaving the bool check in place, but just removing the >> __atomic_fetch_add() part be better? It should still fix the >> regression, but is less likely to change behaviour for targets that >> were never using the builtins. > >Yes, we could do that, but while I have not verified this, I assume that >it's very unlikely that there exists a configuration where the bool >atomics checks already used to fail but the short and int ones did and >still do succeed. Anyway, that's not my decision to make. ;-) Well I guess it's mine, and this is a fairly serious regression (is it tracked in Bugzilla anywhere?) so the patch is OK for trunk. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PR c/68966] Restore atomic builtins usage in libstdc++-v3 2016-04-05 11:01 ` Jonathan Wakely @ 2016-04-05 18:08 ` Jonathan Wakely 2016-04-05 19:04 ` Jonathan Wakely 0 siblings, 1 reply; 18+ messages in thread From: Jonathan Wakely @ 2016-04-05 18:08 UTC (permalink / raw) To: Thomas Schwinge Cc: Martin Sebor, libstdc++, gcc-patches, Marek Polacek, Joseph S. Myers, Jeff Law On 05/04/16 12:01 +0100, Jonathan Wakely wrote: >Well I guess it's mine, and this is a fairly serious regression (is it >tracked in Bugzilla anywhere?) so the patch is OK for trunk. This is now https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70554 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PR c/68966] Restore atomic builtins usage in libstdc++-v3 2016-04-05 18:08 ` Jonathan Wakely @ 2016-04-05 19:04 ` Jonathan Wakely 0 siblings, 0 replies; 18+ messages in thread From: Jonathan Wakely @ 2016-04-05 19:04 UTC (permalink / raw) To: Thomas Schwinge Cc: Martin Sebor, libstdc++, gcc-patches, Marek Polacek, Joseph S. Myers, Jeff Law [-- Attachment #1: Type: text/plain, Size: 625 bytes --] On 05/04/16 19:08 +0100, Jonathan Wakely wrote: >On 05/04/16 12:01 +0100, Jonathan Wakely wrote: >>Well I guess it's mine, and this is a fairly serious regression (is it >>tracked in Bugzilla anywhere?) so the patch is OK for trunk. > >This is now https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70554 I've committed this smaller change, which still tests atomics for bool, but simply removes the uses of __atomic_fetch_add for bool. During stage 1 we can revisit whether we want to do something more invasive and stop testing for atomics that we don't actually need. Tested ppc64le-linux, x86_64-linux, comitted to trunk. [-- Attachment #2: patch.txt --] [-- Type: text/x-patch, Size: 1263 bytes --] commit 7591475c21a34bbcce6d225ea7640d35022fb8ff Author: Jonathan Wakely <jwakely@redhat.com> Date: Tue Apr 5 19:16:05 2016 +0100 Restore atomic builtins usage in libstdc++-v3 PR libstdc++/70554 * acinclude.m4 (GLIBCXX_ENABLE_ATOMIC_BUILTINS): Don't test __atomic_fetch_add for bool. * configure: Regenerate. diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4 index 95df24a..b0f88cb 100644 --- a/libstdc++-v3/acinclude.m4 +++ b/libstdc++-v3/acinclude.m4 @@ -3290,7 +3290,7 @@ AC_DEFUN([GLIBCXX_ENABLE_ATOMIC_BUILTINS], [ atomic_type c1; atomic_type c2; atomic_type c3(0); - __atomic_fetch_add(&c1, c2, __ATOMIC_RELAXED); + // N.B. __atomic_fetch_add is not supported for bool. __atomic_compare_exchange_n(&c1, &c2, c3, true, __ATOMIC_ACQ_REL, __ATOMIC_RELAXED); __atomic_test_and_set(&c1, __ATOMIC_RELAXED); @@ -3375,7 +3375,7 @@ int main() atomic_type c1; atomic_type c2; atomic_type c3(0); - __atomic_fetch_add(&c1, c2, __ATOMIC_RELAXED); + // N.B. __atomic_fetch_add is not supported for bool. __atomic_compare_exchange_n(&c1, &c2, c3, true, __ATOMIC_ACQ_REL, __ATOMIC_RELAXED); __atomic_test_and_set(&c1, __ATOMIC_RELAXED); ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2016-04-05 19:04 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-23 4:46 [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed Martin Sebor 2016-01-02 7:43 ` Jeff Law 2016-01-04 3:03 ` Martin Sebor 2016-01-04 15:22 ` Marek Polacek 2016-01-05 1:18 ` Martin Sebor 2016-01-05 10:51 ` Marek Polacek 2016-01-05 18:47 ` Martin Sebor 2016-01-06 11:50 ` Marek Polacek 2016-01-06 23:38 ` Martin Sebor 2016-01-07 0:08 ` Mike Stump 2016-01-07 12:07 ` Marek Polacek 2016-01-08 1:00 ` Martin Sebor 2016-03-21 12:35 ` [PR c/68966] Restore atomic builtins usage in libstdc++-v3 (was: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed) Thomas Schwinge 2016-03-21 15:12 ` Jonathan Wakely 2016-03-21 16:04 ` [PR c/68966] Restore atomic builtins usage in libstdc++-v3 Thomas Schwinge 2016-04-05 11:01 ` Jonathan Wakely 2016-04-05 18:08 ` Jonathan Wakely 2016-04-05 19:04 ` Jonathan Wakely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).