Re: C PATCH for c/70093 (ICE with nested-function returning VM type)

2016-03-19 Thread Jakub Jelinek 
On Wed, Mar 16, 2016 at 04:11:56PM +0100, Marek Polacek wrote:
> > 2016-03-09  Marek Polacek  
> > 
> > PR c/70093
> > * c-typeck.c (build_function_call_vec): Create a TARGET_EXPR for
> > nested functions returning VM types.
> > 
> > * cgraphunit.c (cgraph_node::expand_thunk): Also build call to the
> > function being thunked if the result type doesn't have fixed size.
> > * gimplify.c (gimplify_modify_expr): Also set LHS if the result type
> > doesn't have fixed size.
> > 
> > * gcc.dg/nested-func-10.c: New test.
> > * gcc.dg/nested-func-9.c: New test.

Ok, thanks.

Jakub

Re: C PATCH for c/70093 (ICE with nested-function returning VM type)

2016-03-19 Thread Marek Polacek 
Ping.

On Wed, Mar 09, 2016 at 04:55:23PM +0100, Marek Polacek wrote:
> On Wed, Mar 09, 2016 at 04:31:37PM +0100, Jakub Jelinek wrote:
> > No, I meant:
> >   switch (n)
> > {
> >   struct S x;
> > case 1:
> >   fn ();
> >   break;
> > case 2:
> >   fn2 ();
> >   break;
> > case 3:
> >   x = fn ();
> >   if (x.a[0] != 42)
> > __builtin_abort ();
> >   break;
> > case 4:
> >   if (fn ().a[0] != 42)
> > __builtin_abort ();
> >   break;
> > ...
> > 
> > The reason is that anything after a noreturn call can be optimized away
> > shortly afterwards.  Perhaps you want __attribute__((noinline, noclone)) on
> > the function too just in case (I know you haven't included -O*).
>  
> Aha.  I couldn't do exactly this because of 
> error: switch jumps into scope of identifier with variably modified type
> so I moved the decl out of the switch.
> 
> > Otherwise LGTM.
> 
> Thanks.
> 
> Bootstrapped/regtested on x86_64-linux.
> 
> 2016-03-09  Marek Polacek  
> 
>   PR c/70093
>   * c-typeck.c (build_function_call_vec): Create a TARGET_EXPR for
>   nested functions returning VM types.
> 
>   * cgraphunit.c (cgraph_node::expand_thunk): Also build call to the
>   function being thunked if the result type doesn't have fixed size.
>   * gimplify.c (gimplify_modify_expr): Also set LHS if the result type
>   doesn't have fixed size.
> 
>   * gcc.dg/nested-func-10.c: New test.
>   * gcc.dg/nested-func-9.c: New test.
> 
> diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
> index 6aa0f03..de9d465 100644
> --- gcc/c/c-typeck.c
> +++ gcc/c/c-typeck.c
> @@ -3068,6 +3068,16 @@ build_function_call_vec (location_t loc, 
> vec arg_loc,
>  result = build_call_array_loc (loc, TREE_TYPE (fntype),
>  function, nargs, argarray);
>  
> +  /* In this improbable scenario, a nested function returns a VM type.
> + Create a TARGET_EXPR so that the call always has a LHS, much as
> + what the C++ FE does for functions returning non-PODs.  */
> +  if (variably_modified_type_p (TREE_TYPE (fntype), NULL_TREE))
> +{
> +  tree tmp = create_tmp_var_raw (TREE_TYPE (fntype));
> +  result = build4 (TARGET_EXPR, TREE_TYPE (fntype), tmp, result,
> +NULL_TREE, NULL_TREE);
> +}
> +
>if (VOID_TYPE_P (TREE_TYPE (result)))
>  {
>if (TYPE_QUALS (TREE_TYPE (result)) != TYPE_UNQUALIFIED)
> diff --git gcc/cgraphunit.c gcc/cgraphunit.c
> index 8b3fddc..4351ae4 100644
> --- gcc/cgraphunit.c
> +++ gcc/cgraphunit.c
> @@ -1708,7 +1708,9 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool 
> force_gimple_thunk)
>  
>/* Build call to the function being thunked.  */
>if (!VOID_TYPE_P (restype)
> -   && (!alias_is_noreturn || TREE_ADDRESSABLE (restype)))
> +   && (!alias_is_noreturn
> +   || TREE_ADDRESSABLE (restype)
> +   || TREE_CODE (TYPE_SIZE_UNIT (restype)) != INTEGER_CST))
>   {
> if (DECL_BY_REFERENCE (resdecl))
>   {
> diff --git gcc/gimplify.c gcc/gimplify.c
> index b331e41..692d168 100644
> --- gcc/gimplify.c
> +++ gcc/gimplify.c
> @@ -4838,7 +4838,8 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, 
> gimple_seq *post_p,
>   }
>notice_special_calls (call_stmt);
>if (!gimple_call_noreturn_p (call_stmt)
> -   || TREE_ADDRESSABLE (TREE_TYPE (*to_p)))
> +   || TREE_ADDRESSABLE (TREE_TYPE (*to_p))
> +   || TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (*to_p))) != INTEGER_CST)
>   gimple_call_set_lhs (call_stmt, *to_p);
>assign = call_stmt;
>  }
> diff --git gcc/testsuite/gcc.dg/nested-func-10.c 
> gcc/testsuite/gcc.dg/nested-func-10.c
> index e69de29..ac6f76f 100644
> --- gcc/testsuite/gcc.dg/nested-func-10.c
> +++ gcc/testsuite/gcc.dg/nested-func-10.c
> @@ -0,0 +1,56 @@
> +/* PR c/70093 */
> +/* { dg-do compile } */
> +/* { dg-options "" } */
> +
> +void __attribute__((noinline, noclone))
> +foo (int n)
> +{
> +  struct S { int a[n]; };
> +
> +  struct S __attribute__((noreturn))
> +  fn (void)
> +  {
> +__builtin_abort ();
> +  }
> +
> +  auto struct S __attribute__((noreturn))
> +  fn2 (void)
> +  {
> +__builtin_abort ();
> +  }
> +
> +  struct S x;
> +  __typeof__ (fn ()) *p = 
> +  switch (n)
> +{
> +case 1:
> +  fn ();
> +  break;
> +case 2:
> +  fn2 ();
> +  break;
> +case 3:
> +  x = fn ();
> +  if (x.a[0] != 42)
> + __builtin_abort ();
> +  break;
> +case 4:
> +  if (fn ().a[0] != 42)
> + __builtin_abort ();
> +  break;
> +case 5:
> +  if (p->a[0] != 42)
> + __builtin_abort ();
> +  break;
> +case 6:
> +  if (fn2 ().a[0] != 42)
> + __builtin_abort ();
> +  break;
> +}
> +}
> +
> +int
> +main (void)
> +{
> +  foo (1);
> +}
> diff --git gcc/testsuite/gcc.dg/nested-func-9.c 
> gcc/testsuite/gcc.dg/nested-func-9.c
> index e69de29..902c258

Re: C PATCH for c/70093 (ICE with nested-function returning VM type)

2016-03-09 Thread Marek Polacek 
On Wed, Mar 09, 2016 at 04:31:37PM +0100, Jakub Jelinek wrote:
> No, I meant:
>   switch (n)
> {
>   struct S x;
> case 1:
>   fn ();
>   break;
> case 2:
>   fn2 ();
>   break;
> case 3:
>   x = fn ();
>   if (x.a[0] != 42)
>   __builtin_abort ();
>   break;
> case 4:
>   if (fn ().a[0] != 42)
>   __builtin_abort ();
>   break;
> ...
> 
> The reason is that anything after a noreturn call can be optimized away
> shortly afterwards.  Perhaps you want __attribute__((noinline, noclone)) on
> the function too just in case (I know you haven't included -O*).
 
Aha.  I couldn't do exactly this because of 
error: switch jumps into scope of identifier with variably modified type
so I moved the decl out of the switch.

> Otherwise LGTM.

Thanks.

Bootstrapped/regtested on x86_64-linux.

2016-03-09  Marek Polacek  

PR c/70093
* c-typeck.c (build_function_call_vec): Create a TARGET_EXPR for
nested functions returning VM types.

* cgraphunit.c (cgraph_node::expand_thunk): Also build call to the
function being thunked if the result type doesn't have fixed size.
* gimplify.c (gimplify_modify_expr): Also set LHS if the result type
doesn't have fixed size.

* gcc.dg/nested-func-10.c: New test.
* gcc.dg/nested-func-9.c: New test.

diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index 6aa0f03..de9d465 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -3068,6 +3068,16 @@ build_function_call_vec (location_t loc, vec 
arg_loc,
 result = build_call_array_loc (loc, TREE_TYPE (fntype),
   function, nargs, argarray);
 
+  /* In this improbable scenario, a nested function returns a VM type.
+ Create a TARGET_EXPR so that the call always has a LHS, much as
+ what the C++ FE does for functions returning non-PODs.  */
+  if (variably_modified_type_p (TREE_TYPE (fntype), NULL_TREE))
+{
+  tree tmp = create_tmp_var_raw (TREE_TYPE (fntype));
+  result = build4 (TARGET_EXPR, TREE_TYPE (fntype), tmp, result,
+  NULL_TREE, NULL_TREE);
+}
+
   if (VOID_TYPE_P (TREE_TYPE (result)))
 {
   if (TYPE_QUALS (TREE_TYPE (result)) != TYPE_UNQUALIFIED)
diff --git gcc/cgraphunit.c gcc/cgraphunit.c
index 8b3fddc..4351ae4 100644
--- gcc/cgraphunit.c
+++ gcc/cgraphunit.c
@@ -1708,7 +1708,9 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool 
force_gimple_thunk)
 
   /* Build call to the function being thunked.  */
   if (!VOID_TYPE_P (restype)
- && (!alias_is_noreturn || TREE_ADDRESSABLE (restype)))
+ && (!alias_is_noreturn
+ || TREE_ADDRESSABLE (restype)
+ || TREE_CODE (TYPE_SIZE_UNIT (restype)) != INTEGER_CST))
{
  if (DECL_BY_REFERENCE (resdecl))
{
diff --git gcc/gimplify.c gcc/gimplify.c
index b331e41..692d168 100644
--- gcc/gimplify.c
+++ gcc/gimplify.c
@@ -4838,7 +4838,8 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, 
gimple_seq *post_p,
}
   notice_special_calls (call_stmt);
   if (!gimple_call_noreturn_p (call_stmt)
- || TREE_ADDRESSABLE (TREE_TYPE (*to_p)))
+ || TREE_ADDRESSABLE (TREE_TYPE (*to_p))
+ || TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (*to_p))) != INTEGER_CST)
gimple_call_set_lhs (call_stmt, *to_p);
   assign = call_stmt;
 }
diff --git gcc/testsuite/gcc.dg/nested-func-10.c 
gcc/testsuite/gcc.dg/nested-func-10.c
index e69de29..ac6f76f 100644
--- gcc/testsuite/gcc.dg/nested-func-10.c
+++ gcc/testsuite/gcc.dg/nested-func-10.c
@@ -0,0 +1,56 @@
+/* PR c/70093 */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+void __attribute__((noinline, noclone))
+foo (int n)
+{
+  struct S { int a[n]; };
+
+  struct S __attribute__((noreturn))
+  fn (void)
+  {
+__builtin_abort ();
+  }
+
+  auto struct S __attribute__((noreturn))
+  fn2 (void)
+  {
+__builtin_abort ();
+  }
+
+  struct S x;
+  __typeof__ (fn ()) *p = 
+  switch (n)
+{
+case 1:
+  fn ();
+  break;
+case 2:
+  fn2 ();
+  break;
+case 3:
+  x = fn ();
+  if (x.a[0] != 42)
+   __builtin_abort ();
+  break;
+case 4:
+  if (fn ().a[0] != 42)
+   __builtin_abort ();
+  break;
+case 5:
+  if (p->a[0] != 42)
+   __builtin_abort ();
+  break;
+case 6:
+  if (fn2 ().a[0] != 42)
+   __builtin_abort ();
+  break;
+}
+}
+
+int
+main (void)
+{
+  foo (1);
+}
diff --git gcc/testsuite/gcc.dg/nested-func-9.c 
gcc/testsuite/gcc.dg/nested-func-9.c
index e69de29..902c258 100644
--- gcc/testsuite/gcc.dg/nested-func-9.c
+++ gcc/testsuite/gcc.dg/nested-func-9.c
@@ -0,0 +1,47 @@
+/* PR c/70093 */
+/* { dg-do run } */
+/* { dg-options "" } */
+
+void
+foo (int n)
+{
+  struct S { int a[n]; };
+
+  struct S
+  fn (void)
+  {
+struct S s;
+s.a[0] = 42;
+return s;
+  }
+
+  auto struct S
+  fn2 (void)
+  {
+

Re: C PATCH for c/70093 (ICE with nested-function returning VM type)

2016-03-09 Thread Jakub Jelinek 
On Wed, Mar 09, 2016 at 04:20:24PM +0100, Marek Polacek wrote:
> --- gcc/testsuite/gcc.dg/nested-func-10.c
> +++ gcc/testsuite/gcc.dg/nested-func-10.c
> @@ -0,0 +1,49 @@
> +/* PR c/70093 */
> +/* { dg-do compile } */
> +/* { dg-options "" } */
> +
> +void
> +foo (int n)
> +{
> +  struct S { int a[n]; };
> +
> +  struct S __attribute__((noreturn))
> +  fn (void)
> +  {
> +__builtin_abort ();
> +  }
> +
> +  auto struct S __attribute__((noreturn))
> +  fn2 (void)
> +  {
> +__builtin_abort ();
> +  }
> +
> +  switch (n)
> +{
> +case 42:;
> +  struct S x;
> +  fn ();
> +  fn2 ();
> +  x = fn ();
> +
> +  if (x.a[0] != 42)
> + __builtin_abort ();
> +
> +  if (fn ().a[0] != 42)
> + __builtin_abort ();
> +
> +  __typeof__ (fn ()) *p = 
> +  if (p->a[0] != 42)
> + __builtin_abort ();
> +
> +  if (fn2 ().a[0] != 42)
> + __builtin_abort ();

No, I meant:
  switch (n)
{
  struct S x;
case 1:
  fn ();
  break;
case 2:
  fn2 ();
  break;
case 3:
  x = fn ();
  if (x.a[0] != 42)
__builtin_abort ();
  break;
case 4:
  if (fn ().a[0] != 42)
__builtin_abort ();
  break;
...

The reason is that anything after a noreturn call can be optimized away
shortly afterwards.  Perhaps you want __attribute__((noinline, noclone)) on
the function too just in case (I know you haven't included -O*).

Otherwise LGTM.

Jakub

Re: C PATCH for c/70093 (ICE with nested-function returning VM type)

2016-03-09 Thread Marek Polacek 
On Wed, Mar 09, 2016 at 03:45:45PM +0100, Jakub Jelinek wrote:
> Instead of the expecting warnings, wouldn't it be better to simply call
> __builtin_abort () in fn ()?
 
Maybe.  Done.

> > +  struct S x;
> > +  x = fn ();
> > +
> > +  if (x.a[0] != 42)
> > +__builtin_abort ();
> > +
> > +  if (fn ().a[0] != 42)
> > +__builtin_abort ();
> > +
> > +  __typeof__ (fn ()) *p = 
> > +  if (p->a[0] != 42)
> > +__builtin_abort ();
> > +
> > +  if (fn2 ().a[0] != 42)
> > +__builtin_abort ();
> 
> And do these all just conditionally, say in a big switch on foo's parameter?
 
Like in the following?

> And, I'm really surprised that you haven't included the case of a call
> without lhs at the source level, so just
>   fn ();
> and
>   fn2 ();
> somewhere.
 
Uhm, yes.  Dunno why they're gone.  So I've added them:

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-03-09  Marek Polacek  

PR c/70093
* c-typeck.c (build_function_call_vec): Create a TARGET_EXPR for
nested functions returning VM types.

* cgraphunit.c (cgraph_node::expand_thunk): Also build call to the
function being thunked if the result type doesn't have fixed size.
* gimplify.c (gimplify_modify_expr): Also set LHS if the result type
doesn't have fixed size.

* gcc.dg/nested-func-10.c: New test.
* gcc.dg/nested-func-9.c: New test.

diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index 6aa0f03..de9d465 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -3068,6 +3068,16 @@ build_function_call_vec (location_t loc, vec 
arg_loc,
 result = build_call_array_loc (loc, TREE_TYPE (fntype),
   function, nargs, argarray);
 
+  /* In this improbable scenario, a nested function returns a VM type.
+ Create a TARGET_EXPR so that the call always has a LHS, much as
+ what the C++ FE does for functions returning non-PODs.  */
+  if (variably_modified_type_p (TREE_TYPE (fntype), NULL_TREE))
+{
+  tree tmp = create_tmp_var_raw (TREE_TYPE (fntype));
+  result = build4 (TARGET_EXPR, TREE_TYPE (fntype), tmp, result,
+  NULL_TREE, NULL_TREE);
+}
+
   if (VOID_TYPE_P (TREE_TYPE (result)))
 {
   if (TYPE_QUALS (TREE_TYPE (result)) != TYPE_UNQUALIFIED)
diff --git gcc/cgraphunit.c gcc/cgraphunit.c
index 8b3fddc..4351ae4 100644
--- gcc/cgraphunit.c
+++ gcc/cgraphunit.c
@@ -1708,7 +1708,9 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool 
force_gimple_thunk)
 
   /* Build call to the function being thunked.  */
   if (!VOID_TYPE_P (restype)
- && (!alias_is_noreturn || TREE_ADDRESSABLE (restype)))
+ && (!alias_is_noreturn
+ || TREE_ADDRESSABLE (restype)
+ || TREE_CODE (TYPE_SIZE_UNIT (restype)) != INTEGER_CST))
{
  if (DECL_BY_REFERENCE (resdecl))
{
diff --git gcc/gimplify.c gcc/gimplify.c
index b331e41..692d168 100644
--- gcc/gimplify.c
+++ gcc/gimplify.c
@@ -4838,7 +4838,8 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, 
gimple_seq *post_p,
}
   notice_special_calls (call_stmt);
   if (!gimple_call_noreturn_p (call_stmt)
- || TREE_ADDRESSABLE (TREE_TYPE (*to_p)))
+ || TREE_ADDRESSABLE (TREE_TYPE (*to_p))
+ || TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (*to_p))) != INTEGER_CST)
gimple_call_set_lhs (call_stmt, *to_p);
   assign = call_stmt;
 }
diff --git gcc/testsuite/gcc.dg/nested-func-10.c 
gcc/testsuite/gcc.dg/nested-func-10.c
index e69de29..c12ff3f 100644
--- gcc/testsuite/gcc.dg/nested-func-10.c
+++ gcc/testsuite/gcc.dg/nested-func-10.c
@@ -0,0 +1,49 @@
+/* PR c/70093 */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+void
+foo (int n)
+{
+  struct S { int a[n]; };
+
+  struct S __attribute__((noreturn))
+  fn (void)
+  {
+__builtin_abort ();
+  }
+
+  auto struct S __attribute__((noreturn))
+  fn2 (void)
+  {
+__builtin_abort ();
+  }
+
+  switch (n)
+{
+case 42:;
+  struct S x;
+  fn ();
+  fn2 ();
+  x = fn ();
+
+  if (x.a[0] != 42)
+   __builtin_abort ();
+
+  if (fn ().a[0] != 42)
+   __builtin_abort ();
+
+  __typeof__ (fn ()) *p = 
+  if (p->a[0] != 42)
+   __builtin_abort ();
+
+  if (fn2 ().a[0] != 42)
+   __builtin_abort ();
+}
+}
+
+int
+main (void)
+{
+  foo (1);
+}
diff --git gcc/testsuite/gcc.dg/nested-func-9.c 
gcc/testsuite/gcc.dg/nested-func-9.c
index e69de29..902c258 100644
--- gcc/testsuite/gcc.dg/nested-func-9.c
+++ gcc/testsuite/gcc.dg/nested-func-9.c
@@ -0,0 +1,47 @@
+/* PR c/70093 */
+/* { dg-do run } */
+/* { dg-options "" } */
+
+void
+foo (int n)
+{
+  struct S { int a[n]; };
+
+  struct S
+  fn (void)
+  {
+struct S s;
+s.a[0] = 42;
+return s;
+  }
+
+  auto struct S
+  fn2 (void)
+  {
+return fn ();
+  }
+
+  struct S x;
+  fn ();
+  fn2 ();
+  x = fn ();
+
+  if (x.a[0] != 42)
+__builtin_abort ();
+
+  if (fn

Re: C PATCH for c/70093 (ICE with nested-function returning VM type)

2016-03-09 Thread Jakub Jelinek 
On Wed, Mar 09, 2016 at 03:34:40PM +0100, Marek Polacek wrote:
> --- gcc/testsuite/gcc.dg/nested-func-10.c
> +++ gcc/testsuite/gcc.dg/nested-func-10.c
> @@ -0,0 +1,45 @@
> +/* PR c/70093 */
> +/* { dg-do compile } */
> +/* { dg-options "" } */
> +
> +void
> +foo (int n)
> +{
> +  struct S { int a[n]; };
> +
> +  struct S __attribute__((noreturn))
> +  fn (void)
> +  {
> +struct S s;
> +s.a[0] = 42;
> +return s; /* { dg-warning "function declared .noreturn.|.noreturn. 
> function does return" } */
> +  }
> +
> +  auto struct S __attribute__((noreturn))
> +  fn2 (void)
> +  {
> +return fn (); /* { dg-warning "function declared .noreturn." } */
> +  }

Instead of the expecting warnings, wouldn't it be better to simply call
__builtin_abort () in fn ()?

> +  struct S x;
> +  x = fn ();
> +
> +  if (x.a[0] != 42)
> +__builtin_abort ();
> +
> +  if (fn ().a[0] != 42)
> +__builtin_abort ();
> +
> +  __typeof__ (fn ()) *p = 
> +  if (p->a[0] != 42)
> +__builtin_abort ();
> +
> +  if (fn2 ().a[0] != 42)
> +__builtin_abort ();

And do these all just conditionally, say in a big switch on foo's parameter?

And, I'm really surprised that you haven't included the case of a call
without lhs at the source level, so just
  fn ();
and
  fn2 ();
somewhere.

> --- gcc/testsuite/gcc.dg/nested-func-9.c
> +++ gcc/testsuite/gcc.dg/nested-func-9.c
> @@ -0,0 +1,45 @@
> +/* PR c/70093 */
> +/* { dg-do run } */
> +/* { dg-options "" } */
> +
> +void
> +foo (int n)
> +{
> +  struct S { int a[n]; };
> +
> +  struct S
> +  fn (void)
> +  {
> +struct S s;
> +s.a[0] = 42;
> +return s;
> +  }
> +
> +  auto struct S
> +  fn2 (void)
> +  {
> +return fn ();
> +  }
> +
> +  struct S x;
> +  x = fn ();
> +
> +  if (x.a[0] != 42)
> +__builtin_abort ();
> +
> +  if (fn ().a[0] != 42)
> +__builtin_abort ();
> +
> +  __typeof__ (fn ()) *p = 
> +  if (p->a[0] != 42)
> +__builtin_abort ();
> +
> +  if (fn2 ().a[0] != 42)
> +__builtin_abort ();

Similarly here, I miss calls that don't use the return value.

Jakub

Re: C PATCH for c/70093 (ICE with nested-function returning VM type)

2016-03-09 Thread Marek Polacek 
On Wed, Mar 09, 2016 at 12:24:42PM +0100, Jakub Jelinek wrote:
> On Wed, Mar 09, 2016 at 12:05:51PM +0100, Marek Polacek wrote:
> > This PR points out that nested functions returning VM types don't work as
> > expected (yeah, go figure).  We got an ICE on the testcase because we were
> > trying to allocate variable-sized temporary instead of using 
> > __builtin_alloca
> > or its kin.  Jakub suggested to follow what the C++ front end does here.  It
> > seems to be the case that it creates a TARGET_EXPR if the call doesn't have
> > a LHS.  That seems to work out well.  The run-time testcase sanity-checks 
> > that
> > we do something reasonable.
> > 
> > Not a regression, but on the other hand the patch doesn't change anything 
> > for
> > 99.9% programs out there.
> 
> Wonder if you still can get an ICE if you add __attribute__((noreturn)) to
> such nested function.  Quick grep shows that there are some suspicious spots
> and others are fine:
[...]

Wow, indeed it ICEs with __attribute__((noreturn)).  Technically, only the
gimplify.c part is needed to fix the new ICE, but I've also fixed the
cgraphunit.c spot for good measure.  New compile test added.

Bootstrapped/regtested on x86_64-linux, ok for trunk or gcc-7?

2016-03-09  Marek Polacek  

PR c/70093
* c-typeck.c (build_function_call_vec): Create a TARGET_EXPR for
nested functions returning VM types.

* cgraphunit.c (cgraph_node::expand_thunk): Also build call to the
function being thunked if the result type doesn't have fixed size.
* gimplify.c (gimplify_modify_expr): Also set LHS if the result type
doesn't have fixed size.

* gcc.dg/nested-func-10.c: New test.
* gcc.dg/nested-func-9.c: New test.

diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index 6aa0f03..de9d465 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -3068,6 +3068,16 @@ build_function_call_vec (location_t loc, vec 
arg_loc,
 result = build_call_array_loc (loc, TREE_TYPE (fntype),
   function, nargs, argarray);
 
+  /* In this improbable scenario, a nested function returns a VM type.
+ Create a TARGET_EXPR so that the call always has a LHS, much as
+ what the C++ FE does for functions returning non-PODs.  */
+  if (variably_modified_type_p (TREE_TYPE (fntype), NULL_TREE))
+{
+  tree tmp = create_tmp_var_raw (TREE_TYPE (fntype));
+  result = build4 (TARGET_EXPR, TREE_TYPE (fntype), tmp, result,
+  NULL_TREE, NULL_TREE);
+}
+
   if (VOID_TYPE_P (TREE_TYPE (result)))
 {
   if (TYPE_QUALS (TREE_TYPE (result)) != TYPE_UNQUALIFIED)
diff --git gcc/cgraphunit.c gcc/cgraphunit.c
index 8b3fddc..4351ae4 100644
--- gcc/cgraphunit.c
+++ gcc/cgraphunit.c
@@ -1708,7 +1708,9 @@ cgraph_node::expand_thunk (bool output_asm_thunks, bool 
force_gimple_thunk)
 
   /* Build call to the function being thunked.  */
   if (!VOID_TYPE_P (restype)
- && (!alias_is_noreturn || TREE_ADDRESSABLE (restype)))
+ && (!alias_is_noreturn
+ || TREE_ADDRESSABLE (restype)
+ || TREE_CODE (TYPE_SIZE_UNIT (restype)) != INTEGER_CST))
{
  if (DECL_BY_REFERENCE (resdecl))
{
diff --git gcc/gimplify.c gcc/gimplify.c
index b331e41..692d168 100644
--- gcc/gimplify.c
+++ gcc/gimplify.c
@@ -4838,7 +4838,8 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, 
gimple_seq *post_p,
}
   notice_special_calls (call_stmt);
   if (!gimple_call_noreturn_p (call_stmt)
- || TREE_ADDRESSABLE (TREE_TYPE (*to_p)))
+ || TREE_ADDRESSABLE (TREE_TYPE (*to_p))
+ || TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (*to_p))) != INTEGER_CST)
gimple_call_set_lhs (call_stmt, *to_p);
   assign = call_stmt;
 }
diff --git gcc/testsuite/gcc.dg/nested-func-10.c 
gcc/testsuite/gcc.dg/nested-func-10.c
index e69de29..1b869ac 100644
--- gcc/testsuite/gcc.dg/nested-func-10.c
+++ gcc/testsuite/gcc.dg/nested-func-10.c
@@ -0,0 +1,45 @@
+/* PR c/70093 */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+void
+foo (int n)
+{
+  struct S { int a[n]; };
+
+  struct S __attribute__((noreturn))
+  fn (void)
+  {
+struct S s;
+s.a[0] = 42;
+return s; /* { dg-warning "function declared .noreturn.|.noreturn. 
function does return" } */
+  }
+
+  auto struct S __attribute__((noreturn))
+  fn2 (void)
+  {
+return fn (); /* { dg-warning "function declared .noreturn." } */
+  }
+
+  struct S x;
+  x = fn ();
+
+  if (x.a[0] != 42)
+__builtin_abort ();
+
+  if (fn ().a[0] != 42)
+__builtin_abort ();
+
+  __typeof__ (fn ()) *p = 
+  if (p->a[0] != 42)
+__builtin_abort ();
+
+  if (fn2 ().a[0] != 42)
+__builtin_abort ();
+}
+
+int
+main (void)
+{
+  foo (1);
+}
diff --git gcc/testsuite/gcc.dg/nested-func-9.c 
gcc/testsuite/gcc.dg/nested-func-9.c
index e69de29..b703f3a 100644
--- gcc/testsuite/gcc.dg/nested-func-9.c
+++ gcc/testsuite/gcc.dg/nested-func-9.c

Re: C PATCH for c/70093 (ICE with nested-function returning VM type)

2016-03-09 Thread Jakub Jelinek 
On Wed, Mar 09, 2016 at 12:05:51PM +0100, Marek Polacek wrote:
> This PR points out that nested functions returning VM types don't work as
> expected (yeah, go figure).  We got an ICE on the testcase because we were
> trying to allocate variable-sized temporary instead of using __builtin_alloca
> or its kin.  Jakub suggested to follow what the C++ front end does here.  It
> seems to be the case that it creates a TARGET_EXPR if the call doesn't have
> a LHS.  That seems to work out well.  The run-time testcase sanity-checks that
> we do something reasonable.
> 
> Not a regression, but on the other hand the patch doesn't change anything for
> 99.9% programs out there.

Wonder if you still can get an ICE if you add __attribute__((noreturn)) to
such nested function.  Quick grep shows that there are some suspicious spots
and others are fine:
cgraphunit.c-  /* Build call to the function being thunked.  */
cgraphunit.c-  if (!VOID_TYPE_P (restype)
cgraphunit.c: && (!alias_is_noreturn || TREE_ADDRESSABLE (restype)))
cgraphunit.c-   {
^^^ needs checking
gimplify.c:  if (!gimple_call_noreturn_p (call_stmt)
gimplify.c-   || TREE_ADDRESSABLE (TREE_TYPE (*to_p)))
gimplify.c- gimple_call_set_lhs (call_stmt, *to_p);
^^^ likewise
tree-cfg.c-  if (lhs
tree-cfg.c-  && gimple_call_ctrl_altering_p (stmt)
tree-cfg.c:  && gimple_call_noreturn_p (stmt)
tree-cfg.c-  && TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (lhs))) == INTEGER_CST
tree-cfg.c-  && !TREE_ADDRESSABLE (TREE_TYPE (lhs)))
tree-cfg.c-{
tree-cfg.c:  error ("LHS in noreturn call");
tree-cfg.c-  return true;
tree-cfg.c-}
^^^ looks fine
tree-cfgcleanup.c-  /* If there is an LHS, remove it, but only if its type has 
fixed size.
tree-cfgcleanup.c- The LHS will need to be recreated during RTL expansion 
and creating
tree-cfgcleanup.c- temporaries of variable-sized types is not supported.  
Also don't
tree-cfgcleanup.c- do this with TREE_ADDRESSABLE types, as assign_temp will 
abort.  */
tree-cfgcleanup.c-  tree lhs = gimple_call_lhs (stmt);
tree-cfgcleanup.c-  if (lhs && TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (lhs))) == 
INTEGER_CST
tree-cfgcleanup.c-  && !TREE_ADDRESSABLE (TREE_TYPE (lhs)))
tree-cfgcleanup.c-{
tree-cfgcleanup.c-  gimple_call_set_lhs (stmt, NULL_TREE);
^^^ likewise

Jakub