diff mbox

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

Message ID 4FC739B4.3000009@net-b.de
State New
Headers show

Commit Message

Tobias Burnus May 31, 2012, 9:28 a.m. UTC 
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

Comments

Jakub Jelinek May 31, 2012, 9:45 a.m. UTC | #1 
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
Janne Blomqvist May 31, 2012, 12:49 p.m. UTC | #2 
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.
diff mbox

Patch

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;
 }