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

[Marek Polacek <polacek at redhat dot com>]

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  <polacek@redhat.com>
> 
> 	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<location_t> 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 = &x;
> +  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)
> +  {
> +    return fn ();
> +  }
> +
> +  struct S x;
> +  fn ();
> +  fn2 ();
> +  x = fn ();
> +
> +  if (x.a[0] != 42)
> +    __builtin_abort ();
> +
> +  if (fn ().a[0] != 42)
> +    __builtin_abort ();
> +
> +  __typeof__ (fn ()) *p = &x;
> +  if (p->a[0] != 42)
> +    __builtin_abort ();
> +
> +  if (fn2 ().a[0] != 42)
> +    __builtin_abort ();
> +}
> +
> +int
> +main (void)
> +{
> +  foo (1);
> +}
> 
> 	Marek

	Marek