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

[Marek Polacek <polacek at redhat dot com>]

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