[PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed

[PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed

[Martin Sebor]

[-- 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." } */
+}

Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed

[Jeff Law]

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

Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed

[Martin Sebor]

[-- 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);
+}

Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed

[Marek Polacek]

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

Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed

[Martin Sebor]

[-- 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);
+}

Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed

[Marek Polacek]

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

Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed

[Martin Sebor]

[-- 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);
+}

Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed

[Marek Polacek]

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

Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed

[Martin Sebor]

>> 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
>

Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed

[Mike Stump]

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

Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed

[Marek Polacek]

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

Re: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed

[Martin Sebor]

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

[PR c/68966] Restore atomic builtins usage in libstdc++-v3 (was: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed)

[Thomas Schwinge]

[-- 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 --]

Re: [PR c/68966] Restore atomic builtins usage in libstdc++-v3 (was: [PATCH] c/68966 - atomic_fetch_* on atomic_bool not diagnosed)

[Jonathan Wakely]

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.

Re: [PR c/68966] Restore atomic builtins usage in libstdc++-v3

[Thomas Schwinge]

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

Re: [PR c/68966] Restore atomic builtins usage in libstdc++-v3

[Jonathan Wakely]

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.

Re: [PR c/68966] Restore atomic builtins usage in libstdc++-v3

[Jonathan Wakely]

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

Re: [PR c/68966] Restore atomic builtins usage in libstdc++-v3

[Jonathan Wakely]

[-- 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);