diff mbox series

Fix atomic builtins on arrays (PR target/82112)

Message ID 20170907084344.GT2323@tucnak
State New
Headers show
Series Fix atomic builtins on arrays (PR target/82112) | expand

Commit Message

Jakub Jelinek Sept. 7, 2017, 8:43 a.m. UTC 
Hi!

The powerpc patch I've just posted led me to try __atomic_* builtins
on arrays as in the testcase below.  While it works fine if the
array is just on the first argument or in C, in C++ for arrays in 2nd
or 3rd argument the atomics are rejected (complaining that the argument
is not a pointer), while we should really have performed array-to-pointer
conversion first.

Fixed thusly, bootstrapped/regtested on powerpc64-linux, ok for trunk?

2017-09-07  Jakub Jelinek  <jakub@redhat.com>

	PR target/82112
	* c-common.c (sync_resolve_size): Formatting fix.
	(get_atomic_generic_size): Likewise.  Before testing if parameter
	has pointer type, if it has array type, call for C++
	default_conversion to perform array-to-pointer conversion.

	* c-c++-common/pr82112.c: New test.


	Jakub

Comments

Joseph Myers Sept. 7, 2017, 3:35 p.m. UTC | #1 
On Thu, 7 Sep 2017, Jakub Jelinek wrote:

> Hi!
> 
> The powerpc patch I've just posted led me to try __atomic_* builtins
> on arrays as in the testcase below.  While it works fine if the
> array is just on the first argument or in C, in C++ for arrays in 2nd
> or 3rd argument the atomics are rejected (complaining that the argument
> is not a pointer), while we should really have performed array-to-pointer
> conversion first.
> 
> Fixed thusly, bootstrapped/regtested on powerpc64-linux, ok for trunk?

I don't think either the existing or new assertion that arrays only occur 
in C++ are correct.  E.g. build the following with -std=gnu90 so 
non-lvalue arrays don't decay to pointers.  (ICEs on such an assertion in 
get_atomic_generic_size with a trunk build as of a few days ago.)

struct s { int c[10]; };
struct s f (void);
void
g (void)
{
  __atomic_load ( f().c, f().c, __ATOMIC_ACQUIRE);
}

(I think C90 non-lvalue arrays there are most appropriately an error, 
rather than attempting to make them decay to pointers after all.)
Jakub Jelinek Sept. 7, 2017, 9:09 p.m. UTC | #2 
On Thu, Sep 07, 2017 at 03:35:00PM +0000, Joseph Myers wrote:
> > The powerpc patch I've just posted led me to try __atomic_* builtins
> > on arrays as in the testcase below.  While it works fine if the
> > array is just on the first argument or in C, in C++ for arrays in 2nd
> > or 3rd argument the atomics are rejected (complaining that the argument
> > is not a pointer), while we should really have performed array-to-pointer
> > conversion first.
> > 
> > Fixed thusly, bootstrapped/regtested on powerpc64-linux, ok for trunk?
> 
> I don't think either the existing or new assertion that arrays only occur 
> in C++ are correct.  E.g. build the following with -std=gnu90 so 
> non-lvalue arrays don't decay to pointers.  (ICEs on such an assertion in 
> get_atomic_generic_size with a trunk build as of a few days ago.)
> 
> struct s { int c[10]; };
> struct s f (void);
> void
> g (void)
> {
>   __atomic_load ( f().c, f().c, __ATOMIC_ACQUIRE);
> }
> 
> (I think C90 non-lvalue arrays there are most appropriately an error, 
> rather than attempting to make them decay to pointers after all.)

Ah, ok, so like this instead?

2017-09-07  Jakub Jelinek  <jakub@redhat.com>

	PR target/82112
	* c-common.c (sync_resolve_size): Instead of c_dialect_cxx ()
	assertion check that in the condition.
	(get_atomic_generic_size): Likewise.  Before testing if parameter
	has pointer type, if it has array type, call for C++
	default_conversion to perform array-to-pointer conversion.

	* c-c++-common/pr82112.c: New test.
	* gcc.dg/pr82112.c: New test.

--- gcc/c-family/c-common.c.jj	2017-09-06 16:23:25.285903973 +0200
+++ gcc/c-family/c-common.c	2017-09-07 22:55:18.836731301 +0200
@@ -6478,10 +6478,9 @@ sync_resolve_size (tree function, vec<tr
     }
 
   argtype = type = TREE_TYPE ((*params)[0]);
-  if (TREE_CODE (type) == ARRAY_TYPE)
+  if (TREE_CODE (type) == ARRAY_TYPE && c_dialect_cxx ())
     {
       /* Force array-to-pointer decay for C++.  */
-      gcc_assert (c_dialect_cxx());
       (*params)[0] = default_conversion ((*params)[0]);
       type = TREE_TYPE ((*params)[0]);
     }
@@ -6646,10 +6645,9 @@ get_atomic_generic_size (location_t loc,
 
   /* Get type of first parameter, and determine its size.  */
   type_0 = TREE_TYPE ((*params)[0]);
-  if (TREE_CODE (type_0) == ARRAY_TYPE)
+  if (TREE_CODE (type_0) == ARRAY_TYPE && c_dialect_cxx ())
     {
       /* Force array-to-pointer decay for C++.  */
-      gcc_assert (c_dialect_cxx());
       (*params)[0] = default_conversion ((*params)[0]);
       type_0 = TREE_TYPE ((*params)[0]);
     }
@@ -6688,6 +6686,12 @@ get_atomic_generic_size (location_t loc,
       /* __atomic_compare_exchange has a bool in the 4th position, skip it.  */
       if (n_param == 6 && x == 3)
         continue;
+      if (TREE_CODE (type) == ARRAY_TYPE && c_dialect_cxx ())
+	{
+	  /* Force array-to-pointer decay for C++.  */
+	  (*params)[x] = default_conversion ((*params)[x]);
+	  type = TREE_TYPE ((*params)[x]);
+	}
       if (!POINTER_TYPE_P (type))
 	{
 	  error_at (loc, "argument %d of %qE must be a pointer type", x + 1,
--- gcc/testsuite/c-c++-common/pr82112.c.jj	2017-09-07 22:54:39.755185278 +0200
+++ gcc/testsuite/c-c++-common/pr82112.c	2017-09-07 22:54:39.755185278 +0200
@@ -0,0 +1,13 @@
+/* PR target/82112 */
+/* { dg-do compile } */
+
+int c[10], d[10], e[10], f[10], g[10], h[10], i[10], j[10], k[10], l[10];
+
+void
+foo (void)
+{
+  __atomic_load (c, d, __ATOMIC_ACQUIRE);
+  __atomic_store (e, f, __ATOMIC_SEQ_CST);
+  __atomic_exchange (g, h, i, __ATOMIC_RELAXED);
+  __atomic_compare_exchange (j, k, l, 1, __ATOMIC_RELAXED, __ATOMIC_RELAXED);
+}
--- gcc/testsuite/gcc.dg/pr82112.c.jj	2017-09-07 22:55:59.917254104 +0200
+++ gcc/testsuite/gcc.dg/pr82112.c	2017-09-07 23:07:27.635458611 +0200
@@ -0,0 +1,21 @@
+/* PR target/82112 */
+/* { dg-do compile } */
+/* { dg-options "-std=gnu90" } */
+
+struct S { int a[10]; } bar (void);
+int b, c;
+
+void
+foo (void)
+{
+  __atomic_load (bar ().a, &b, __ATOMIC_ACQUIRE);	/* { dg-error "argument 1 of .__atomic_load. must be a non-void pointer type" } */
+  __atomic_load (&b, bar ().a, __ATOMIC_ACQUIRE);	/* { dg-error "argument 2 of .__atomic_load. must be a pointer type" } */
+  __atomic_store (bar ().a, &b, __ATOMIC_SEQ_CST);	/* { dg-error "argument 1 of .__atomic_store. must be a non-void pointer type" } */
+  __atomic_store (&b, bar ().a, __ATOMIC_SEQ_CST);	/* { dg-error "argument 2 of .__atomic_store. must be a pointer type" } */
+  __atomic_exchange (bar ().a, &b, &c, __ATOMIC_RELAXED);	/* { dg-error "argument 1 of .__atomic_exchange. must be a non-void pointer type" } */
+  __atomic_exchange (&b, bar ().a, &c, __ATOMIC_RELAXED);	/* { dg-error "argument 2 of .__atomic_exchange. must be a pointer type" } */
+  __atomic_exchange (&b, &c, bar ().a, __ATOMIC_RELAXED);	/* { dg-error "argument 3 of .__atomic_exchange. must be a pointer type" } */
+  __atomic_compare_exchange (bar ().a, &b, &c, 1, __ATOMIC_RELAXED, __ATOMIC_RELAXED);	/* { dg-error "argument 1 of .__atomic_compare_exchange. must be a non-void pointer type" } */
+  __atomic_compare_exchange (&b, bar ().a, &c, 1, __ATOMIC_RELAXED, __ATOMIC_RELAXED);	/* { dg-error "argument 2 of .__atomic_compare_exchange. must be a pointer type" } */
+  __atomic_compare_exchange (&b, &c, bar ().a, 1, __ATOMIC_RELAXED, __ATOMIC_RELAXED);	/* { dg-error "argument 3 of .__atomic_compare_exchange. must be a pointer type" } */
+}


	Jakub
Joseph Myers Sept. 7, 2017, 9:38 p.m. UTC | #3 
On Thu, 7 Sep 2017, Jakub Jelinek wrote:

> Ah, ok, so like this instead?
> 
> 2017-09-07  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/82112
> 	* c-common.c (sync_resolve_size): Instead of c_dialect_cxx ()
> 	assertion check that in the condition.
> 	(get_atomic_generic_size): Likewise.  Before testing if parameter
> 	has pointer type, if it has array type, call for C++
> 	default_conversion to perform array-to-pointer conversion.
> 
> 	* c-c++-common/pr82112.c: New test.
> 	* gcc.dg/pr82112.c: New test.

This version is OK.
Jason Merrill Sept. 8, 2017, 4:39 p.m. UTC | #4 
On Thu, Sep 7, 2017 at 11:38 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Thu, 7 Sep 2017, Jakub Jelinek wrote:
>
>> Ah, ok, so like this instead?
>>
>> 2017-09-07  Jakub Jelinek  <jakub@redhat.com>
>>
>>       PR target/82112
>>       * c-common.c (sync_resolve_size): Instead of c_dialect_cxx ()
>>       assertion check that in the condition.
>>       (get_atomic_generic_size): Likewise.  Before testing if parameter
>>       has pointer type, if it has array type, call for C++
>>       default_conversion to perform array-to-pointer conversion.
>>
>>       * c-c++-common/pr82112.c: New test.
>>       * gcc.dg/pr82112.c: New test.
>
> This version is OK.

OK with me as well.

Jason
diff mbox series

Patch

--- gcc/c-family/c-common.c.jj	2017-09-01 09:25:35.000000000 +0200
+++ gcc/c-family/c-common.c	2017-09-06 14:47:11.781523252 +0200
@@ -6481,7 +6481,7 @@  sync_resolve_size (tree function, vec<tr
   if (TREE_CODE (type) == ARRAY_TYPE)
     {
       /* Force array-to-pointer decay for C++.  */
-      gcc_assert (c_dialect_cxx());
+      gcc_assert (c_dialect_cxx ());
       (*params)[0] = default_conversion ((*params)[0]);
       type = TREE_TYPE ((*params)[0]);
     }
@@ -6649,7 +6649,7 @@  get_atomic_generic_size (location_t loc,
   if (TREE_CODE (type_0) == ARRAY_TYPE)
     {
       /* Force array-to-pointer decay for C++.  */
-      gcc_assert (c_dialect_cxx());
+      gcc_assert (c_dialect_cxx ());
       (*params)[0] = default_conversion ((*params)[0]);
       type_0 = TREE_TYPE ((*params)[0]);
     }
@@ -6688,6 +6688,13 @@  get_atomic_generic_size (location_t loc,
       /* __atomic_compare_exchange has a bool in the 4th position, skip it.  */
       if (n_param == 6 && x == 3)
         continue;
+      if (TREE_CODE (type) == ARRAY_TYPE)
+	{
+	  /* Force array-to-pointer decay for C++.  */
+	  gcc_assert (c_dialect_cxx ());
+	  (*params)[x] = default_conversion ((*params)[x]);
+	  type = TREE_TYPE ((*params)[x]);
+	}
       if (!POINTER_TYPE_P (type))
 	{
 	  error_at (loc, "argument %d of %qE must be a pointer type", x + 1,
--- gcc/testsuite/c-c++-common/pr82112.c.jj	2017-09-06 15:21:06.720336134 +0200
+++ gcc/testsuite/c-c++-common/pr82112.c	2017-09-06 15:21:25.138116835 +0200
@@ -0,0 +1,13 @@ 
+/* PR target/82112 */
+/* { dg-do compile } */
+
+int c[10], d[10], e[10], f[10], g[10], h[10], i[10], j[10], k[10], l[10];
+
+void
+foo (void)
+{
+  __atomic_load (c, d, __ATOMIC_ACQUIRE);
+  __atomic_store (e, f, __ATOMIC_SEQ_CST);
+  __atomic_exchange (g, h, i, __ATOMIC_RELAXED);
+  __atomic_compare_exchange (j, k, l, 1, __ATOMIC_RELAXED, __ATOMIC_RELAXED);
+}