diff mbox

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

Message ID 20160309152024.GU10006@redhat.com
State New
Headers show

Commit Message

Marek Polacek March 9, 2016, 3:20 p.m. UTC 
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 = &x;
> > +  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  <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.


	Marek

Comments

Jakub Jelinek March 9, 2016, 3:31 p.m. UTC | #1 
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 = &x;
> +      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
diff mbox

Patch

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..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 = &x;
+      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 ().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);
+}