Re: [PATCH] Fix ICE with __atomic_{always,is}_lock_free (PR middle-end/77624)

2016-09-20 Thread Richard Biener
On Tue, 20 Sep 2016, Jakub Jelinek wrote:

> Hi!
> 
> fold_builtin_atomic_always_lock_free has an assertion that if the 2nd
> argument isn't INTEGER_CST, it has POINTER_TYPE_P, but additionally for
> casts to void * it looks through the cast.  That means though for invalid
> code if an integral expression etc. is converted to pointer, we violate the
> assertion.  This patch instead doesn't look through such casts, so
> type_align is effectively alignment of void and thus it considers objects
> insufficiently aligned, but doesn't ICE.
> 
> Another approach (incomplete) is what I've attached to the PR - instead of
> prototyping __atomic_{always,is}_lock_free it uses ... and only checks that
> it has 2 arguments and that the last one is a pointer - that way we don't
> have to try to undo implicit cast to void * and know the conversion in there
> is the user conversion.  Unfortunately the C++ FE isn't prepared for the
> check_builtin_function_arguments to change the arguments (unlike C).
> 
> I've bootstrapped/regtested on x86_64-linux and i686-linux the following
> fix, ok for trunk, or should I invest more time into the other approach?

Ok for trunk.  I think the other solution would be to somehow mark the
builtin to not get the (void *) cast applied in the first place ...
(make it varargs?)

Thanks,
Richard.

> 2016-09-19  Jakub Jelinek  
> 
>   PR middle-end/77624
>   * builtins.c (fold_builtin_atomic_always_lock_free): Only look through
>   cast to void * if the cast is from some other pointer type.
> 
>   * c-c++-common/pr77624-1.c: New test.
>   * c-c++-common/pr77624-2.c: New test.
> 
> --- gcc/builtins.c.jj 2016-09-16 22:19:38.0 +0200
> +++ gcc/builtins.c2016-09-19 15:28:41.100412521 +0200
> @@ -5575,8 +5575,10 @@ fold_builtin_atomic_always_lock_free (tr
>end before anything else has a chance to look at it.  The pointer
>parameter at this point is usually cast to a void *, so check for that
>and look past the cast.  */
> -  if (CONVERT_EXPR_P (arg1) && POINTER_TYPE_P (ttype)
> -   && VOID_TYPE_P (TREE_TYPE (ttype)))
> +  if (CONVERT_EXPR_P (arg1)
> +   && POINTER_TYPE_P (ttype)
> +   && VOID_TYPE_P (TREE_TYPE (ttype))
> +   && POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (arg1, 0
>   arg1 = TREE_OPERAND (arg1, 0);
>  
>ttype = TREE_TYPE (arg1);
> --- gcc/testsuite/c-c++-common/pr77624-1.c.jj 2016-09-19 15:18:09.049394093 
> +0200
> +++ gcc/testsuite/c-c++-common/pr77624-1.c2016-09-19 15:41:09.972949880 
> +0200
> @@ -0,0 +1,14 @@
> +/* PR middle-end/77624 */
> +/* { dg-do compile } */
> +
> +int
> +foo (int a)
> +{
> +  return __atomic_is_lock_free (2, a);   /* { dg-warning 
> "pointer from integer" "" { target c } } */
> +}/* { dg-error "invalid 
> conversion" "" { target c++ } 7 } */
> +
> +int
> +bar (int a)
> +{
> +  return __atomic_always_lock_free (2, a);   /* { dg-warning "pointer from 
> integer" "" { target c } } */
> +}/* { dg-error "invalid 
> conversion" "" { target c++ } 13 } */
> --- gcc/testsuite/c-c++-common/pr77624-2.c.jj 2016-09-19 15:18:12.280353292 
> +0200
> +++ gcc/testsuite/c-c++-common/pr77624-2.c2016-09-19 15:42:37.441844581 
> +0200
> @@ -0,0 +1,26 @@
> +/* PR middle-end/77624 */
> +/* { dg-do compile } */
> +
> +void
> +foo (int *a)
> +{
> +  double b = 0;
> +  __atomic_is_lock_free (2, a, 2);   /* { dg-error "too many arguments" } */
> +  __atomic_is_lock_free (2); /* { dg-error "too few arguments" } */
> +  __atomic_is_lock_free (2, b);  /* { dg-error "incompatible 
> type" "" { target c } } */
> + /* { dg-message "expected" "" { target 
> c } 10 } */
> + /* { dg-error "convert" "" { target c++ 
> } 10 } */
> +  __atomic_is_lock_free (2, 0);
> +}
> +
> +void
> +bar (int *a)
> +{
> +  double b = 0;
> +  __atomic_always_lock_free (2, a, 2);   /* { dg-error "too many 
> arguments" } */
> +  __atomic_always_lock_free (2); /* { dg-error "too few arguments" } */
> +  __atomic_always_lock_free (2, b);  /* { dg-error "incompatible type" "" { 
> target c } } */
> + /* { dg-message "expected" "" { target 
> c } 22 } */
> + /* { dg-error "convert" "" { target c++ 
> } 22 } */
> +  __atomic_always_lock_free (2, 0);
> +}
> 
>   Jakub
> 
> 

-- 
Richard Biener 
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 
21284 (AG Nuernberg)


[PATCH] Fix ICE with __atomic_{always,is}_lock_free (PR middle-end/77624)

2016-09-19 Thread Jakub Jelinek
Hi!

fold_builtin_atomic_always_lock_free has an assertion that if the 2nd
argument isn't INTEGER_CST, it has POINTER_TYPE_P, but additionally for
casts to void * it looks through the cast.  That means though for invalid
code if an integral expression etc. is converted to pointer, we violate the
assertion.  This patch instead doesn't look through such casts, so
type_align is effectively alignment of void and thus it considers objects
insufficiently aligned, but doesn't ICE.

Another approach (incomplete) is what I've attached to the PR - instead of
prototyping __atomic_{always,is}_lock_free it uses ... and only checks that
it has 2 arguments and that the last one is a pointer - that way we don't
have to try to undo implicit cast to void * and know the conversion in there
is the user conversion.  Unfortunately the C++ FE isn't prepared for the
check_builtin_function_arguments to change the arguments (unlike C).

I've bootstrapped/regtested on x86_64-linux and i686-linux the following
fix, ok for trunk, or should I invest more time into the other approach?

2016-09-19  Jakub Jelinek  

PR middle-end/77624
* builtins.c (fold_builtin_atomic_always_lock_free): Only look through
cast to void * if the cast is from some other pointer type.

* c-c++-common/pr77624-1.c: New test.
* c-c++-common/pr77624-2.c: New test.

--- gcc/builtins.c.jj   2016-09-16 22:19:38.0 +0200
+++ gcc/builtins.c  2016-09-19 15:28:41.100412521 +0200
@@ -5575,8 +5575,10 @@ fold_builtin_atomic_always_lock_free (tr
 end before anything else has a chance to look at it.  The pointer
 parameter at this point is usually cast to a void *, so check for that
 and look past the cast.  */
-  if (CONVERT_EXPR_P (arg1) && POINTER_TYPE_P (ttype)
- && VOID_TYPE_P (TREE_TYPE (ttype)))
+  if (CONVERT_EXPR_P (arg1)
+ && POINTER_TYPE_P (ttype)
+ && VOID_TYPE_P (TREE_TYPE (ttype))
+ && POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (arg1, 0
arg1 = TREE_OPERAND (arg1, 0);
 
   ttype = TREE_TYPE (arg1);
--- gcc/testsuite/c-c++-common/pr77624-1.c.jj   2016-09-19 15:18:09.049394093 
+0200
+++ gcc/testsuite/c-c++-common/pr77624-1.c  2016-09-19 15:41:09.972949880 
+0200
@@ -0,0 +1,14 @@
+/* PR middle-end/77624 */
+/* { dg-do compile } */
+
+int
+foo (int a)
+{
+  return __atomic_is_lock_free (2, a); /* { dg-warning "pointer from 
integer" "" { target c } } */
+}  /* { dg-error "invalid 
conversion" "" { target c++ } 7 } */
+
+int
+bar (int a)
+{
+  return __atomic_always_lock_free (2, a); /* { dg-warning "pointer from 
integer" "" { target c } } */
+}  /* { dg-error "invalid 
conversion" "" { target c++ } 13 } */
--- gcc/testsuite/c-c++-common/pr77624-2.c.jj   2016-09-19 15:18:12.280353292 
+0200
+++ gcc/testsuite/c-c++-common/pr77624-2.c  2016-09-19 15:42:37.441844581 
+0200
@@ -0,0 +1,26 @@
+/* PR middle-end/77624 */
+/* { dg-do compile } */
+
+void
+foo (int *a)
+{
+  double b = 0;
+  __atomic_is_lock_free (2, a, 2); /* { dg-error "too many arguments" } */
+  __atomic_is_lock_free (2);   /* { dg-error "too few arguments" } */
+  __atomic_is_lock_free (2, b);/* { dg-error "incompatible 
type" "" { target c } } */
+   /* { dg-message "expected" "" { target 
c } 10 } */
+   /* { dg-error "convert" "" { target c++ 
} 10 } */
+  __atomic_is_lock_free (2, 0);
+}
+
+void
+bar (int *a)
+{
+  double b = 0;
+  __atomic_always_lock_free (2, a, 2); /* { dg-error "too many arguments" } */
+  __atomic_always_lock_free (2);   /* { dg-error "too few arguments" } */
+  __atomic_always_lock_free (2, b);/* { dg-error "incompatible type" "" { 
target c } } */
+   /* { dg-message "expected" "" { target 
c } 22 } */
+   /* { dg-error "convert" "" { target c++ 
} 22 } */
+  __atomic_always_lock_free (2, 0);
+}

Jakub