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

[Martin Sebor <msebor at gmail dot com>]

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

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