[Patch, Fortran] PR53521 - Fix size == 0 handling with reallocate

[Patch, Fortran] PR53521 - Fix size == 0 handling with reallocate

[Tobias Burnus]

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

Dear all,

gfortran was producing the following code:

   res = realloc (mem, size)
   if (size == 0)
     res = NULL;
   ...
   if (res != 0)
     free (res)

The problem is that "realloc (..., 0)" does not have to produce a NULL 
pointer as result.


While in "valgrind" one only gets the warning that "0 bytes in <n> 
blocks" where not freed, looking at "top", one sees an excessive use of 
memory.

That seems to cause crashes in CP2K. At least the valgrind issue is a 
GCC 4.3 to 4.8 regression.


Without the patch, gfortran produces:
--------------------------------------------------------------------
       D.1888 = (integer(kind=4)[0] * restrict) __builtin_realloc ((void 
*) atmp.0.data, D.1887);
       if (D.1888 == 0B && D.1887 != 0)
         {
           _gfortran_os_error (&"Allocation would exceed memory 
limit"[1]{lb: 1 sz: 1});
         }
       if (D.1887 == 0)
         {
           D.1888 = 0B;
         }
       atmp.0.data = (void * restrict) D.1888;
--------------------------------------------------------------------


I see two possibilities:


1) FIRST approach: manual freeing/NULL setting; that's the closed what 
is currently done. (Note: It calls free unconditionally; "free(0)" is 
optimized away by the middle end.
(Note: That requires an update of the "free" count in 
gfortran.dg/allocatable_function_4.f90)
--------------------------------------------------------------------
       if (D.1887 == 0)
         {
           __builtin_free ((void *) atmp.0.data);
           D.1888 = 0B;
         }
       else
         {
           D.1888 = (integer(kind=4)[0] * restrict) __builtin_realloc 
((void *) atmp.0.data, D.1887);
           if (D.1888 == 0B && D.1887 != 0)
             {
               _gfortran_os_error (&"Allocation would exceed memory 
limit"[1]{lb: 1 sz: 1});
             }
         }
       atmp.0.data = (void * restrict) D.1888;
--------------------------------------------------------------------


2) SECOND approach: Simply removing the NULL setting and just using the 
result which realloc returns; if not NULL, the free() will properly 
handle it.
--------------------------------------------------------------------
       D.1888 = (integer(kind=4)[0] * restrict) __builtin_realloc ((void 
*) atmp.0.data, D.1887);
       if (D.1888 == 0B && D.1887 != 0)
         {
           _gfortran_os_error (&"Allocation would exceed memory 
limit"[1]{lb: 1 sz: 1});
         }
       atmp.0.data = (void * restrict) D.1888;
--------------------------------------------------------------------


Both patches have been tested with Joost's example and the test suite.
Which version do you prefer? OK for the trunk?

I like the second version more. (And I would even consider to get rid of 
the os_error.)

Tobias

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

2012-05-31  Tobias Burnus  <burnus@net-b.de>

	PR fortran/53521
	* trans.c (gfc_deallocate_scalar_with_status): Properly
	handle the case size == 0.

diff --git a/gcc/fortran/trans.c b/gcc/fortran/trans.c
index 5d6e6ef..d53112f 100644
--- a/gcc/fortran/trans.c
+++ b/gcc/fortran/trans.c
@@ -1126,20 +1126,24 @@ gfc_deallocate_scalar_with_status (tree pointer, tree status, bool can_fail,
 void *
 internal_realloc (void *mem, size_t size)
 {
+  if (size == 0)
+    {
+      free (mem);
+      return NULL;
+    }
+
   res = realloc (mem, size);
   if (!res && size != 0)
     _gfortran_os_error ("Allocation would exceed memory limit");
 
-  if (size == 0)
-    return NULL;
-
   return res;
 }  */
 tree
 gfc_call_realloc (stmtblock_t * block, tree mem, tree size)
 {
-  tree msg, res, nonzero, zero, null_result, tmp;
+  tree msg, res, cond, null_result, tmp;
   tree type = TREE_TYPE (mem);
+  stmtblock_t nonzero, zero;
 
   size = gfc_evaluate_now (size, block);
 
@@ -1149,17 +1153,20 @@ gfc_call_realloc (stmtblock_t * block, tree mem, tree size)
   /* Create a variable to hold the result.  */
   res = gfc_create_var (type, NULL);
 
+  gfc_init_block (&nonzero);
+  gfc_init_block (&zero);
+
   /* Call realloc and check the result.  */
   tmp = build_call_expr_loc (input_location,
 			 builtin_decl_explicit (BUILT_IN_REALLOC), 2,
 			 fold_convert (pvoid_type_node, mem), size);
-  gfc_add_modify (block, res, fold_convert (type, tmp));
+  gfc_add_modify (&nonzero, res, fold_convert (type, tmp));
   null_result = fold_build2_loc (input_location, EQ_EXPR, boolean_type_node,
 				 res, build_int_cst (pvoid_type_node, 0));
-  nonzero = fold_build2_loc (input_location, NE_EXPR, boolean_type_node, size,
+  cond = fold_build2_loc (input_location, NE_EXPR, boolean_type_node, size,
 			     build_int_cst (size_type_node, 0));
   null_result = fold_build2_loc (input_location, TRUTH_AND_EXPR, boolean_type_node,
-				 null_result, nonzero);
+				 null_result, cond);
   msg = gfc_build_addr_expr (pchar_type_node, gfc_build_localized_cstring_const
 			     ("Allocation would exceed memory limit"));
   tmp = fold_build3_loc (input_location, COND_EXPR, void_type_node,
@@ -1167,15 +1174,22 @@ gfc_call_realloc (stmtblock_t * block, tree mem, tree size)
 			 build_call_expr_loc (input_location,
 					      gfor_fndecl_os_error, 1, msg),
 			 build_empty_stmt (input_location));
-  gfc_add_expr_to_block (block, tmp);
+  gfc_add_expr_to_block (&nonzero, tmp);
 
   /* if (size == 0) then the result is NULL.  */
+  tmp = build_call_expr_loc (input_location,
+			      builtin_decl_explicit (BUILT_IN_FREE),
+			      1, fold_convert (pvoid_type_node, mem));
+  gfc_add_expr_to_block (&zero, tmp);
   tmp = fold_build2_loc (input_location, MODIFY_EXPR, type, res,
 			 build_int_cst (type, 0));
-  zero = fold_build1_loc (input_location, TRUTH_NOT_EXPR, boolean_type_node,
-			  nonzero);
-  tmp = fold_build3_loc (input_location, COND_EXPR, void_type_node, zero, tmp,
-			 build_empty_stmt (input_location));
+
+  gfc_add_expr_to_block (&zero, tmp);
+  cond = fold_build1_loc (input_location, TRUTH_NOT_EXPR, boolean_type_node,
+			  cond);
+  tmp = fold_build3_loc (input_location, COND_EXPR, void_type_node, cond,
+			 gfc_finish_block (&zero),
+			 gfc_finish_block (&nonzero));
   gfc_add_expr_to_block (block, tmp);
 
   return res;

[-- Attachment #3: realloc-alt2.diff --]
[-- Type: text/x-patch, Size: 1337 bytes --]

2012-05-31  Tobias Burnus  <burnus@net-b.de>

	PR fortran/53521
	* trans.c (gfc_deallocate_scalar_with_status): Properly
	handle the case size == 0.

diff --git a/gcc/fortran/trans.c b/gcc/fortran/trans.c
index 5d6e6ef..3313be9 100644
--- a/gcc/fortran/trans.c
+++ b/gcc/fortran/trans.c
@@ -1130,15 +1130,12 @@ internal_realloc (void *mem, size_t size)
   if (!res && size != 0)
     _gfortran_os_error ("Allocation would exceed memory limit");
 
-  if (size == 0)
-    return NULL;
-
   return res;
 }  */
 tree
 gfc_call_realloc (stmtblock_t * block, tree mem, tree size)
 {
-  tree msg, res, nonzero, zero, null_result, tmp;
+  tree msg, res, nonzero, null_result, tmp;
   tree type = TREE_TYPE (mem);
 
   size = gfc_evaluate_now (size, block);
@@ -1169,15 +1166,6 @@ gfc_call_realloc (stmtblock_t * block, tree mem, tree size)
 			 build_empty_stmt (input_location));
   gfc_add_expr_to_block (block, tmp);
 
-  /* if (size == 0) then the result is NULL.  */
-  tmp = fold_build2_loc (input_location, MODIFY_EXPR, type, res,
-			 build_int_cst (type, 0));
-  zero = fold_build1_loc (input_location, TRUTH_NOT_EXPR, boolean_type_node,
-			  nonzero);
-  tmp = fold_build3_loc (input_location, COND_EXPR, void_type_node, zero, tmp,
-			 build_empty_stmt (input_location));
-  gfc_add_expr_to_block (block, tmp);
-
   return res;
 }
 

Re: [Patch, Fortran] PR53521 - Fix size == 0 handling with reallocate

[Jakub Jelinek]

On Thu, May 31, 2012 at 11:28:20AM +0200, Tobias Burnus wrote:
> I see two possibilities:
> 
> 
> 1) FIRST approach: manual freeing/NULL setting; that's the closed
> what is currently done. (Note: It calls free unconditionally;
> "free(0)" is optimized away by the middle end.
> (Note: That requires an update of the "free" count in
> gfortran.dg/allocatable_function_4.f90)
> --------------------------------------------------------------------
>       if (D.1887 == 0)
>         {
>           __builtin_free ((void *) atmp.0.data);
>           D.1888 = 0B;
>         }
>       else
>         {
>           D.1888 = (integer(kind=4)[0] * restrict) __builtin_realloc
> ((void *) atmp.0.data, D.1887);
>           if (D.1888 == 0B && D.1887 != 0)

You wouldn't need to check D.1887 != 0 here, that check has been performed
earlier.

> 2) SECOND approach: Simply removing the NULL setting and just using
> the result which realloc returns; if not NULL, the free() will
> properly handle it.
> --------------------------------------------------------------------
>       D.1888 = (integer(kind=4)[0] * restrict) __builtin_realloc
> ((void *) atmp.0.data, D.1887);
>       if (D.1888 == 0B && D.1887 != 0)
>         {
>           _gfortran_os_error (&"Allocation would exceed memory
> limit"[1]{lb: 1 sz: 1});
>         }
>       atmp.0.data = (void * restrict) D.1888;
> --------------------------------------------------------------------

Yeah, POSIX says on realloc return value:
If size was equal to 0, either NULL or a pointer suitable to be passed to
free() is returned.  So, for POSIX compatible realloc implementation the
second approach should be fine.

> I like the second version more. (And I would even consider to get
> rid of the os_error.)

Without the os_error, you'd just crash silently on allocation failures?

	Jakub

Re: [Patch, Fortran] PR53521 - Fix size == 0 handling with reallocate

[Janne Blomqvist]

On Thu, May 31, 2012 at 12:28 PM, Tobias Burnus <burnus@net-b.de> wrote:
> Dear all,
>
> gfortran was producing the following code:
>
>  res = realloc (mem, size)
>  if (size == 0)
>    res = NULL;
>  ...
>  if (res != 0)
>    free (res)
>
> The problem is that "realloc (..., 0)" does not have to produce a NULL
> pointer as result.
>
>
> While in "valgrind" one only gets the warning that "0 bytes in <n> blocks"
> where not freed, looking at "top", one sees an excessive use of memory.
>
> That seems to cause crashes in CP2K. At least the valgrind issue is a GCC
> 4.3 to 4.8 regression.
>
>
> Without the patch, gfortran produces:
> --------------------------------------------------------------------
>      D.1888 = (integer(kind=4)[0] * restrict) __builtin_realloc ((void *)
> atmp.0.data, D.1887);
>      if (D.1888 == 0B && D.1887 != 0)
>        {
>          _gfortran_os_error (&"Allocation would exceed memory limit"[1]{lb:
> 1 sz: 1});
>        }
>      if (D.1887 == 0)
>        {
>          D.1888 = 0B;
>        }
>      atmp.0.data = (void * restrict) D.1888;
> --------------------------------------------------------------------
>
>
> I see two possibilities:
>
>
> 1) FIRST approach: manual freeing/NULL setting; that's the closed what is
> currently done. (Note: It calls free unconditionally; "free(0)" is optimized
> away by the middle end.
> (Note: That requires an update of the "free" count in
> gfortran.dg/allocatable_function_4.f90)
> --------------------------------------------------------------------
>      if (D.1887 == 0)
>        {
>          __builtin_free ((void *) atmp.0.data);
>          D.1888 = 0B;
>        }
>      else
>        {
>          D.1888 = (integer(kind=4)[0] * restrict) __builtin_realloc ((void
> *) atmp.0.data, D.1887);
>          if (D.1888 == 0B && D.1887 != 0)
>            {
>              _gfortran_os_error (&"Allocation would exceed memory
> limit"[1]{lb: 1 sz: 1});
>            }
>        }
>      atmp.0.data = (void * restrict) D.1888;
> --------------------------------------------------------------------
>
>
> 2) SECOND approach: Simply removing the NULL setting and just using the
> result which realloc returns; if not NULL, the free() will properly handle
> it.
> --------------------------------------------------------------------
>      D.1888 = (integer(kind=4)[0] * restrict) __builtin_realloc ((void *)
> atmp.0.data, D.1887);
>      if (D.1888 == 0B && D.1887 != 0)
>        {
>          _gfortran_os_error (&"Allocation would exceed memory limit"[1]{lb:
> 1 sz: 1});
>        }
>      atmp.0.data = (void * restrict) D.1888;
> --------------------------------------------------------------------
>
>
> Both patches have been tested with Joost's example and the test suite.
> Which version do you prefer? OK for the trunk?
>
> I like the second version more. (And I would even consider to get rid of the
> os_error.)

I prefer the second approach as well, Ok for trunk.


-- 
Janne Blomqvist