Patchwork [fortran] PR28105 Overflow check for memory allocation

login
register
mail settings
Submitter Janne Blomqvist
Date Dec. 13, 2010, 9:06 p.m.
Message ID <AANLkTin=anKwViM_6s6PCBCM+rgEJg3PS9M16YfC5jpO@mail.gmail.com>
Download mbox | patch
Permalink /patch/75407/
State New
Headers show

Comments

Janne Blomqvist - Dec. 13, 2010, 9:06 p.m.
Hi,

as a result of a recent commit, size_t in the gfortran frontend is now
properly an unsigned type. As a side-effect of this, testing whether a
value of type size_t is negative just generates dead code which is
optimized away. This kind of test was previously used as a crude kind
of overflow check for memory allocation (for the ALLOCATE statement
there is now a proper implementation).

The attached patch removes the generation of this dead code in the first place.

Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
Steve Kargl - Dec. 14, 2010, 6:41 a.m.
On Mon, Dec 13, 2010 at 11:06:08PM +0200, Janne Blomqvist wrote:
> Hi,
> 
> as a result of a recent commit, size_t in the gfortran frontend is now
> properly an unsigned type. As a side-effect of this, testing whether a
> value of type size_t is negative just generates dead code which is
> optimized away. This kind of test was previously used as a crude kind
> of overflow check for memory allocation (for the ALLOCATE statement
> there is now a proper implementation).
> 
> The attached patch removes the generation of this dead code in the first place.
> 
> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
> 

OK.

Just out of curiousity, your error message is now "Out of memory".
If someone's shell restricts that amount of memory allowed to be
allocate, one could run into an allocation issue even though the
system has memory available.  Won't a better error message be
"memory allocation issue"?
Janne Blomqvist - Dec. 15, 2010, 10:18 a.m.
On Tue, Dec 14, 2010 at 08:41, Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
> On Mon, Dec 13, 2010 at 11:06:08PM +0200, Janne Blomqvist wrote:
>> Hi,
>>
>> as a result of a recent commit, size_t in the gfortran frontend is now
>> properly an unsigned type. As a side-effect of this, testing whether a
>> value of type size_t is negative just generates dead code which is
>> optimized away. This kind of test was previously used as a crude kind
>> of overflow check for memory allocation (for the ALLOCATE statement
>> there is now a proper implementation).
>>
>> The attached patch removes the generation of this dead code in the first place.
>>
>> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
>>
>
> OK.

Thanks.

> Just out of curiousity, your error message is now "Out of memory".
> If someone's shell restricts that amount of memory allowed to be
> allocate, one could run into an allocation issue even though the
> system has memory available.  Won't a better error message be
> "memory allocation issue"?

Good point. POSIX specifies that if malloc() returns NULL, errno is
set to ENOMEM which, according to the spec, means "Insufficient
storage space is available."

http://pubs.opengroup.org/onlinepubs/9699919799/functions/malloc.html

This might mean that, yes, the system is out of memory, or it might
mean something else like a per-process or per-user limit preventing
the allocation from succeeding, as you say. Any ideas for a suitable
wording? IMHO the POSIX "Insufficient storage space is available."
error message might confuse a user to believe e.g. that (s)he is out
of disk space or something like that, so I'd suggest "Memory
allocation error" or "Error allocating memory". "Issue" is maybe a bit
vague (did it actually succeed or not?)?

OTOH since we're calling _gfortran_os_error which calls perror() we
get the same message twice just worded slightly differently. E.g.

$ ./largealloc
Operating system error: Cannot allocate memory
Out of memory

which is a bit redundant. So maybe just an empty string (or NULL) is sufficient?
Steve Kargl - Dec. 15, 2010, 4:20 p.m.
On Wed, Dec 15, 2010 at 12:18:16PM +0200, Janne Blomqvist wrote:
> 
> OTOH since we're calling _gfortran_os_error which calls perror() we
> get the same message twice just worded slightly differently. E.g.
> 
> $ ./largealloc
> Operating system error: Cannot allocate memory
> Out of memory
> 
> which is a bit redundant. So maybe just an empty string (or NULL)
> is sufficient?
> 

In the above, how about changing "Out of memory" to 
"Allocation would exceed system or user memory limit".
Thus, one would get 

$ ./largealloc
Operating system error: Cannot allocate memory
Allocation would exceed system or user memory limit

Second, provides a hint to user where to look for 
the problem.  Whatever you decide is fine with me.
Janne Blomqvist - Dec. 15, 2010, 5:17 p.m.
On Tue, Dec 14, 2010 at 08:41, Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
> On Mon, Dec 13, 2010 at 11:06:08PM +0200, Janne Blomqvist wrote:
>> Hi,
>>
>> as a result of a recent commit, size_t in the gfortran frontend is now
>> properly an unsigned type. As a side-effect of this, testing whether a
>> value of type size_t is negative just generates dead code which is
>> optimized away. This kind of test was previously used as a crude kind
>> of overflow check for memory allocation (for the ALLOCATE statement
>> there is now a proper implementation).
>>
>> The attached patch removes the generation of this dead code in the first place.
>>
>> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
>>
>
> OK.

Sending        fortran/ChangeLog
Sending        fortran/trans.c
Transmitting file data ..
Committed revision 167860.

> Just out of curiousity, your error message is now "Out of memory".

FWIW, the above commit was the original patch, I'll fix the error
messages in a separate commit.

Patch

diff --git a/gcc/fortran/trans.c b/gcc/fortran/trans.c
index b84029b..f3914a1 100644
--- a/gcc/fortran/trans.c
+++ b/gcc/fortran/trans.c
@@ -519,7 +519,7 @@  gfc_trans_runtime_check (bool error, bool once, tree cond, stmtblock_t * pblock,
 
 
 /* Call malloc to allocate size bytes of memory, with special conditions:
-      + if size <= 0, return a malloced area of size 1,
+      + if size == 0, return a malloced area of size 1,
       + if malloc returns NULL, issue a runtime error.  */
 tree
 gfc_call_malloc (stmtblock_t * block, tree type, tree size)
@@ -584,37 +584,21 @@  gfc_call_malloc (stmtblock_t * block, tree type, tree size)
       if (stat)
 	*stat = 0;
 
-      // The only time this can happen is the size wraps around.
-      if (size < 0)
+      newmem = malloc (MAX (size, 1));
+      if (newmem == NULL)
       {
-	if (stat)
-	{
-	  *stat = LIBERROR_ALLOCATION;
-	  newmem = NULL;
-	}
-	else
-	  runtime_error ("Attempt to allocate negative amount of memory. "
-			 "Possible integer overflow");
-      }
-      else
-      {
-	newmem = malloc (MAX (size, 1));
-	if (newmem == NULL)
-	{
-	  if (stat)
-	    *stat = LIBERROR_ALLOCATION;
-	  else
-	    runtime_error ("Out of memory");
-	}
+        if (stat)
+          *stat = LIBERROR_ALLOCATION;
+        else
+	  runtime_error ("Out of memory");
       }
-
       return newmem;
     }  */
 tree
 gfc_allocate_with_status (stmtblock_t * block, tree size, tree status)
 {
   stmtblock_t alloc_block;
-  tree res, tmp, error, msg, cond;
+  tree res, tmp, msg, cond;
   tree status_type = status ? TREE_TYPE (TREE_TYPE (status)) : NULL_TREE;
 
   /* Evaluate size only once, and make sure it has the right type.  */
@@ -640,32 +624,6 @@  gfc_allocate_with_status (stmtblock_t * block, tree size, tree status)
       gfc_add_expr_to_block (block, tmp);
     }
 
-  /* Generate the block of code handling (size < 0).  */
-  msg = gfc_build_addr_expr (pchar_type_node, gfc_build_localized_cstring_const
-			("Attempt to allocate negative amount of memory. "
-			 "Possible integer overflow"));
-  error = build_call_expr_loc (input_location,
-			   gfor_fndecl_runtime_error, 1, msg);
-
-  if (status != NULL_TREE && !integer_zerop (status))
-    {
-      /* Set the status variable if it's present.  */
-      stmtblock_t set_status_block;
-
-      gfc_start_block (&set_status_block);
-      gfc_add_modify (&set_status_block,
-		      fold_build1_loc (input_location, INDIRECT_REF,
-				       status_type, status),
-			   build_int_cst (status_type, LIBERROR_ALLOCATION));
-      gfc_add_modify (&set_status_block, res,
-			   build_int_cst (prvoid_type_node, 0));
-
-      tmp = fold_build2_loc (input_location, EQ_EXPR, boolean_type_node,
-			     status, build_int_cst (TREE_TYPE (status), 0));
-      error = fold_build3_loc (input_location, COND_EXPR, void_type_node, tmp,
-			       error, gfc_finish_block (&set_status_block));
-    }
-
   /* The allocation itself.  */
   gfc_start_block (&alloc_block);
   gfc_add_modify (&alloc_block, res,
@@ -703,12 +661,7 @@  gfc_allocate_with_status (stmtblock_t * block, tree size, tree status)
 					  build_int_cst (prvoid_type_node, 0)),
 			 tmp, build_empty_stmt (input_location));
   gfc_add_expr_to_block (&alloc_block, tmp);
-
-  cond = fold_build2_loc (input_location, LT_EXPR, boolean_type_node, size,
-			  build_int_cst (TREE_TYPE (size), 0));
-  tmp = fold_build3_loc (input_location, COND_EXPR, void_type_node, cond, error,
-			 gfc_finish_block (&alloc_block));
-  gfc_add_expr_to_block (block, tmp);
+  gfc_add_expr_to_block (block, gfc_finish_block (&alloc_block));
 
   return res;
 }
@@ -1048,8 +1001,6 @@  gfc_deallocate_scalar_with_status (tree pointer, tree status, bool can_fail,
 void *
 internal_realloc (void *mem, size_t size)
 {
-  if (size < 0)
-    runtime_error ("Attempt to allocate a negative amount of memory.");
   res = realloc (mem, size);
   if (!res && size != 0)
     _gfortran_os_error ("Out of memory");
@@ -1062,7 +1013,7 @@  internal_realloc (void *mem, size_t size)
 tree
 gfc_call_realloc (stmtblock_t * block, tree mem, tree size)
 {
-  tree msg, res, negative, nonzero, zero, null_result, tmp;
+  tree msg, res, nonzero, zero, null_result, tmp;
   tree type = TREE_TYPE (mem);
 
   size = gfc_evaluate_now (size, block);
@@ -1073,17 +1024,6 @@  gfc_call_realloc (stmtblock_t * block, tree mem, tree size)
   /* Create a variable to hold the result.  */
   res = gfc_create_var (type, NULL);
 
-  /* size < 0 ?  */
-  negative = fold_build2_loc (input_location, LT_EXPR, boolean_type_node, size,
-			      build_int_cst (size_type_node, 0));
-  msg = gfc_build_addr_expr (pchar_type_node, gfc_build_localized_cstring_const
-      ("Attempt to allocate a negative amount of memory."));
-  tmp = fold_build3_loc (input_location, COND_EXPR, void_type_node, negative,
-			 build_call_expr_loc (input_location,
-					    gfor_fndecl_runtime_error, 1, msg),
-			 build_empty_stmt (input_location));
-  gfc_add_expr_to_block (block, tmp);
-
   /* Call realloc and check the result.  */
   tmp = build_call_expr_loc (input_location,
 			 built_in_decls[BUILT_IN_REALLOC], 2,