[patch] Fix ICE on function [not] returning variable size

[patch] Fix ICE on function [not] returning variable size

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]

Hi,

this fixes an ICE on a function returning a variable-sized record type but 
discovered to be no-return by the optimizer.  In this case, the LHS of the 
GIMPLE call statement is removed so the RTL expander attempts to allocate a 
temporary and fails:

eric@polaris:~/build/gcc/native> gcc/gnat1 -quiet varsize_return2.ads -O
+===========================GNAT BUG DETECTED==============================+
| 6.0.0 20150531 (experimental) [trunk revision 223897] (x86_64-suse-linux) 
GCC error:|
| in assign_stack_temp_for_type, at function.c:793        

because it too cannot create temporaries of variable size.

The attached patch simply preserves the LHS throughout the GIMPLE pipeline.
It also simplifies the relevant test in gimplify_modify_expr_rhs, which was 
overly broad.

Tested on x86_64-suse-linux, OK for the mainline?


2015-06-01  Eric Botcazou  <ebotcazou@adacore.com>

	* gimplify.c (gimplify_modify_expr_rhs): Use simple test on the size.
	* cgraph.c (cgraph_redirect_edge_call_stmt_to_callee): Do not remove
	the LHS of a no-return call if its type has variable size.
	* tree-cfgcleanup.c (fixup_noreturn_call): Likewise.
	* tree-cfg.c (verify_gimple_call): Accept these no-return calls.


2015-06-01  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/specs/varsize_return2.ads: New test.
	* gnat.dg/specs/varsize_return2_pkg.ad[sb]: New helper.


-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 3916 bytes --]

Index: cgraph.c
===================================================================
--- cgraph.c	(revision 223897)
+++ cgraph.c	(working copy)
@@ -1449,8 +1449,10 @@ cgraph_edge::redirect_call_stmt_to_calle
       update_stmt_fn (DECL_STRUCT_FUNCTION (e->caller->decl), new_stmt);
     }
 
-  /* If the call becomes noreturn, remove the lhs.  */
-  if (lhs && (gimple_call_flags (new_stmt) & ECF_NORETURN))
+  /* If the call becomes noreturn, remove the LHS if possible.  */
+  if (lhs
+      && (gimple_call_flags (new_stmt) & ECF_NORETURN)
+      && TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (lhs))) == INTEGER_CST)
     {
       if (TREE_CODE (lhs) == SSA_NAME)
 	{
Index: gimplify.c
===================================================================
--- gimplify.c	(revision 223897)
+++ gimplify.c	(working copy)
@@ -4341,7 +4341,8 @@ gimplify_modify_expr_rhs (tree *expr_p,
 		/* It's OK to use the target directly if it's being
 		   initialized. */
 		use_target = true;
-	      else if (variably_modified_type_p (TREE_TYPE (*to_p), NULL_TREE))
+	      else if (TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (*to_p)))
+		       != INTEGER_CST)
 		/* Always use the target and thus RSO for variable-sized types.
 		   GIMPLE cannot deal with a variable-sized assignment
 		   embedded in a call statement.  */
Index: tree-cfgcleanup.c
===================================================================
--- tree-cfgcleanup.c	(revision 223897)
+++ tree-cfgcleanup.c	(working copy)
@@ -612,9 +612,11 @@ fixup_noreturn_call (gimple stmt)
 	}
     }
 
-  /* If there is an LHS, remove it.  */
+  /* If there is an LHS, remove it, but only if its type has fixed size.
+     The LHS will need to be recreated during RTL expansion and creating
+     temporaries of variable-sized types is not supported.  */
   tree lhs = gimple_call_lhs (stmt);
-  if (lhs)
+  if (lhs && TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (lhs))) == INTEGER_CST)
     {
       gimple_call_set_lhs (stmt, NULL_TREE);
 
Index: tree-cfg.c
===================================================================
--- tree-cfg.c	(revision 223897)
+++ tree-cfg.c	(working copy)
@@ -3391,17 +3391,19 @@ verify_gimple_call (gcall *stmt)
        return true;
      }
 
-  if (gimple_call_lhs (stmt)
-      && (!is_gimple_lvalue (gimple_call_lhs (stmt))
-	  || verify_types_in_gimple_reference (gimple_call_lhs (stmt), true)))
+  tree lhs = gimple_call_lhs (stmt);
+  if (lhs
+      && (!is_gimple_lvalue (lhs)
+	  || verify_types_in_gimple_reference (lhs, true)))
     {
       error ("invalid LHS in gimple call");
       return true;
     }
 
-  if (gimple_call_ctrl_altering_p (stmt)
-      && gimple_call_lhs (stmt)
-      && gimple_call_noreturn_p (stmt))
+  if (lhs
+      && gimple_call_ctrl_altering_p (stmt)
+      && gimple_call_noreturn_p (stmt)
+      && TREE_CODE (TYPE_SIZE_UNIT (TREE_TYPE (lhs))) == INTEGER_CST)
     {
       error ("LHS in noreturn call");
       return true;
@@ -3409,19 +3411,18 @@ verify_gimple_call (gcall *stmt)
 
   fntype = gimple_call_fntype (stmt);
   if (fntype
-      && gimple_call_lhs (stmt)
-      && !useless_type_conversion_p (TREE_TYPE (gimple_call_lhs (stmt)),
-				     TREE_TYPE (fntype))
+      && lhs
+      && !useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (fntype))
       /* ???  At least C++ misses conversions at assignments from
 	 void * call results.
 	 ???  Java is completely off.  Especially with functions
 	 returning java.lang.Object.
 	 For now simply allow arbitrary pointer type conversions.  */
-      && !(POINTER_TYPE_P (TREE_TYPE (gimple_call_lhs (stmt)))
+      && !(POINTER_TYPE_P (TREE_TYPE (lhs))
 	   && POINTER_TYPE_P (TREE_TYPE (fntype))))
     {
       error ("invalid conversion in gimple call");
-      debug_generic_stmt (TREE_TYPE (gimple_call_lhs (stmt)));
+      debug_generic_stmt (TREE_TYPE (lhs));
       debug_generic_stmt (TREE_TYPE (fntype));
       return true;
     }

[-- Attachment #3: varsize_return2.ads --]
[-- Type: text/x-adasrc, Size: 213 bytes --]

-- { dg-do compile }
-- { dg-options "-O" }

with Varsize_Return2_Pkg; use Varsize_Return2_Pkg;

package Varsize_Return2 is

   package My_G is new G (0);

   Result : constant T := My_G.Get;

end Varsize_Return2;

[-- Attachment #4: varsize_return2_pkg.adb --]
[-- Type: text/x-adasrc, Size: 310 bytes --]

package body Varsize_Return2_Pkg is

   function Len return Positive is
   begin
      return 4;
   end;

   package body G is

      function Get return Small_T is
      begin
         raise Program_Error;
         return Get;
      end;

   end G;

end Varsize_Return2_Pkg;\r

[-- Attachment #5: varsize_return2_pkg.ads --]
[-- Type: text/x-adasrc, Size: 341 bytes --]

-- { dg-excess-errors "no code generated" }

package Varsize_Return2_Pkg is

   type T (D: Positive) is record
      Data: String (1 .. D);
   end record;

   function Len return Positive;

   generic
      I : Integer;
   package G is

      subtype Small_T is T(Len);
      function Get return Small_T;

   end G;

end Varsize_Return2_Pkg;

Re: [patch] Fix ICE on function [not] returning variable size

[Richard Biener]

On Mon, Jun 1, 2015 at 12:43 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> this fixes an ICE on a function returning a variable-sized record type but
> discovered to be no-return by the optimizer.  In this case, the LHS of the
> GIMPLE call statement is removed so the RTL expander attempts to allocate a
> temporary and fails:
>
> eric@polaris:~/build/gcc/native> gcc/gnat1 -quiet varsize_return2.ads -O
> +===========================GNAT BUG DETECTED==============================+
> | 6.0.0 20150531 (experimental) [trunk revision 223897] (x86_64-suse-linux)
> GCC error:|
> | in assign_stack_temp_for_type, at function.c:793
>
> because it too cannot create temporaries of variable size.
>
> The attached patch simply preserves the LHS throughout the GIMPLE pipeline.
> It also simplifies the relevant test in gimplify_modify_expr_rhs, which was
> overly broad.
>
> Tested on x86_64-suse-linux, OK for the mainline?

Ok.  (I wonder if there are any cases where the return value is allocated by the
callee?)

Thanks,
Richard.

>
> 2015-06-01  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gimplify.c (gimplify_modify_expr_rhs): Use simple test on the size.
>         * cgraph.c (cgraph_redirect_edge_call_stmt_to_callee): Do not remove
>         the LHS of a no-return call if its type has variable size.
>         * tree-cfgcleanup.c (fixup_noreturn_call): Likewise.
>         * tree-cfg.c (verify_gimple_call): Accept these no-return calls.
>
>
> 2015-06-01  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/specs/varsize_return2.ads: New test.
>         * gnat.dg/specs/varsize_return2_pkg.ad[sb]: New helper.
>
>
> --
> Eric Botcazou

Re: [patch] Fix ICE on function [not] returning variable size

[Eric Botcazou]

> Ok.  (I wonder if there are any cases where the return value is allocated by
> the callee?)

Thanks.

Do you mean in GCC or in programming languages in general or...?  In GNAT, we 
have something like that: when a function returns an unconstrained type whose 
size depends on some discriminants of the type, the caller doesn't know the 
size of the return value in advance, so the callee allocates the return value 
on the "secondary stack" and effectively returns a pointer to it.  Of course 
you need a specific machinery to manage this secondary stack.  And, needless 
to say, this is quite inefficient, so we attempt to reduce its usage:
  https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02632.html

-- 
Eric Botcazou

Re: [patch] Fix ICE on function [not] returning variable size

[Richard Biener]

On Mon, Jun 1, 2015 at 11:08 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Ok.  (I wonder if there are any cases where the return value is allocated by
>> the callee?)
>
> Thanks.
>
> Do you mean in GCC or in programming languages in general or...?  In GNAT, we
> have something like that: when a function returns an unconstrained type whose
> size depends on some discriminants of the type, the caller doesn't know the
> size of the return value in advance, so the callee allocates the return value
> on the "secondary stack" and effectively returns a pointer to it.  Of course
> you need a specific machinery to manage this secondary stack.  And, needless
> to say, this is quite inefficient, so we attempt to reduce its usage:
>   https://gcc.gnu.org/ml/gcc-patches/2015-05/msg02632.html

Yes, in general in GCC.  In this case we could still remove the lhs (not sure if
it is worth the trouble or even easy to detect on GIMPLE)?

Richard.

> --
> Eric Botcazou

Re: [patch] Fix ICE on function [not] returning variable size

[Eric Botcazou]

> Yes, in general in GCC.  In this case we could still remove the lhs (not
> sure if it is worth the trouble or even easy to detect on GIMPLE)?

At least for the case I described in Ada, that's already done since the 
function effectively returns a pointer type.

-- 
Eric Botcazou