Patchwork [fortran] Always return malloc(1) for empty arrays in the library

login
register
mail settings
Submitter Thomas Koenig
Date June 28, 2011, 9:50 p.m.
Message ID <4E0A4C94.7030602@netcologne.de>
Download mbox | patch
Permalink /patch/102489/
State New
Headers show

Comments

Thomas Koenig - June 28, 2011, 9:50 p.m.
Hello world,

looking at PR 49479 and other functions in the library made me realize
there are lots of places where we don't malloc one byte for empty
arrays.

This patch is an attempt at fixing the ton of regressions likely
caused by this (like in the PR) which haven't been found yet.
No test cases, as they haven't been found yet :-)

I also noticed two places where we had a memory leak (in eoshift1 and
eoshift3), which I also fixed.

Regression-tested.  OK for trunk and, after a few days, for 4.6?

Thomas
2011-06-28  Thomas Koenig  <tkoenig@gcc.gnu.org>

	* m4/in_pack.m4 (internal_pack_'rtype_ccode`):  If size is
	less than one, allocate a single byte.
	* m4/transpose.m4 (transpose_'rtype_code`):  Likewise.
	* m4/cshift1.m4 (cshift1):  Likewise.
	* m4/matmull.m4 (matmul_'rtype_code`):  Likewise.
	* m4/unpack.m4 (unpack0_'rtype_code`):  Likewise.
	* m4/ifunction_logical.m4 (name`'rtype_qual`_'atype_code):  Likewise.
	* m4/matmul.m4 (name`'rtype_qual`_'atype_code):  Likewise.
	* intrinics/transpose_generic.c (transpose_internal):  Likewise.
	* intrinsics/unpack_generic.c (unpack_internal):  Likewise.
	* m4/eoshift1.m4 (eoshift1):  Remove double allocation.
	* m4/eoshift3.m4 (eoshift3):  Likewise.
	* generated/all_l16.c: Regenerated.
	* generated/all_l1.c: Regenerated.
	* generated/all_l2.c: Regenerated.
	* generated/all_l4.c: Regenerated.
	* generated/all_l8.c: Regenerated.
	* generated/any_l16.c: Regenerated.
	* generated/any_l1.c: Regenerated.
	* generated/any_l2.c: Regenerated.
	* generated/any_l4.c: Regenerated.
	* generated/any_l8.c: Regenerated.
	* generated/count_16_l.c: Regenerated.
	* generated/count_1_l.c: Regenerated.
	* generated/count_2_l.c: Regenerated.
	* generated/count_4_l.c: Regenerated.
	* generated/count_8_l.c: Regenerated.
	* generated/cshift1_16.c: Regenerated.
	* generated/cshift1_4.c: Regenerated.
	* generated/cshift1_8.c: Regenerated.
	* generated/eoshift1_16.c: Regenerated.
	* generated/eoshift1_4.c: Regenerated.
	* generated/eoshift1_8.c: Regenerated.
	* generated/eoshift3_16.c: Regenerated.
	* generated/eoshift3_4.c: Regenerated.
	* generated/eoshift3_8.c: Regenerated.
	* generated/in_pack_c10.c: Regenerated.
	* generated/in_pack_c16.c: Regenerated.
	* generated/in_pack_c4.c: Regenerated.
	* generated/in_pack_c8.c: Regenerated.
	* generated/in_pack_i16.c: Regenerated.
	* generated/in_pack_i1.c: Regenerated.
	* generated/in_pack_i2.c: Regenerated.
	* generated/in_pack_i4.c: Regenerated.
	* generated/in_pack_i8.c: Regenerated.
	* generated/in_pack_r10.c: Regenerated.
	* generated/in_pack_r16.c: Regenerated.
	* generated/in_pack_r4.c: Regenerated.
	* generated/in_pack_r8.c: Regenerated.
	* generated/matmul_c10.c: Regenerated.
	* generated/matmul_c16.c: Regenerated.
	* generated/matmul_c4.c: Regenerated.
	* generated/matmul_c8.c: Regenerated.
	* generated/matmul_i16.c: Regenerated.
	* generated/matmul_i1.c: Regenerated.
	* generated/matmul_i2.c: Regenerated.
	* generated/matmul_i4.c: Regenerated.
	* generated/matmul_i8.c: Regenerated.
	* generated/matmul_l16.c: Regenerated.
	* generated/matmul_l4.c: Regenerated.
	* generated/matmul_l8.c: Regenerated.
	* generated/matmul_r10.c: Regenerated.
	* generated/matmul_r16.c: Regenerated.
	* generated/matmul_r4.c: Regenerated.
	* generated/matmul_r8.c: Regenerated.
	* generated/maxloc1_16_i16.c: Regenerated.
	* generated/maxloc1_16_i1.c: Regenerated.
	* generated/maxloc1_16_i2.c: Regenerated.
	* generated/maxloc1_16_i4.c: Regenerated.
	* generated/maxloc1_16_i8.c: Regenerated.
	* generated/maxloc1_16_r10.c: Regenerated.
	* generated/maxloc1_16_r16.c: Regenerated.
	* generated/maxloc1_16_r4.c: Regenerated.
	* generated/maxloc1_16_r8.c: Regenerated.
	* generated/maxloc1_4_i16.c: Regenerated.
	* generated/maxloc1_4_i1.c: Regenerated.
	* generated/maxloc1_4_i2.c: Regenerated.
	* generated/maxloc1_4_i4.c: Regenerated.
	* generated/maxloc1_4_i8.c: Regenerated.
	* generated/maxloc1_4_r10.c: Regenerated.
	* generated/maxloc1_4_r16.c: Regenerated.
	* generated/maxloc1_4_r4.c: Regenerated.
	* generated/maxloc1_4_r8.c: Regenerated.
	* generated/maxloc1_8_i16.c: Regenerated.
	* generated/maxloc1_8_i1.c: Regenerated.
	* generated/maxloc1_8_i2.c: Regenerated.
	* generated/maxloc1_8_i4.c: Regenerated.
	* generated/maxloc1_8_i8.c: Regenerated.
	* generated/maxloc1_8_r10.c: Regenerated.
	* generated/maxloc1_8_r16.c: Regenerated.
	* generated/maxloc1_8_r4.c: Regenerated.
	* generated/maxloc1_8_r8.c: Regenerated.
	* generated/maxval_i16.c: Regenerated.
	* generated/maxval_i1.c: Regenerated.
	* generated/maxval_i2.c: Regenerated.
	* generated/maxval_i4.c: Regenerated.
	* generated/maxval_i8.c: Regenerated.
	* generated/maxval_r10.c: Regenerated.
	* generated/maxval_r16.c: Regenerated.
	* generated/maxval_r4.c: Regenerated.
	* generated/maxval_r8.c: Regenerated.
	* generated/minloc1_16_i16.c: Regenerated.
	* generated/minloc1_16_i1.c: Regenerated.
	* generated/minloc1_16_i2.c: Regenerated.
	* generated/minloc1_16_i4.c: Regenerated.
	* generated/minloc1_16_i8.c: Regenerated.
	* generated/minloc1_16_r10.c: Regenerated.
	* generated/minloc1_16_r16.c: Regenerated.
	* generated/minloc1_16_r4.c: Regenerated.
	* generated/minloc1_16_r8.c: Regenerated.
	* generated/minloc1_4_i16.c: Regenerated.
	* generated/minloc1_4_i1.c: Regenerated.
	* generated/minloc1_4_i2.c: Regenerated.
	* generated/minloc1_4_i4.c: Regenerated.
	* generated/minloc1_4_i8.c: Regenerated.
	* generated/minloc1_4_r10.c: Regenerated.
	* generated/minloc1_4_r16.c: Regenerated.
	* generated/minloc1_4_r4.c: Regenerated.
	* generated/minloc1_4_r8.c: Regenerated.
	* generated/minloc1_8_i16.c: Regenerated.
	* generated/minloc1_8_i1.c: Regenerated.
	* generated/minloc1_8_i2.c: Regenerated.
	* generated/minloc1_8_i4.c: Regenerated.
	* generated/minloc1_8_i8.c: Regenerated.
	* generated/minloc1_8_r10.c: Regenerated.
	* generated/minloc1_8_r16.c: Regenerated.
	* generated/minloc1_8_r4.c: Regenerated.
	* generated/minloc1_8_r8.c: Regenerated.
	* generated/minval_i16.c: Regenerated.
	* generated/minval_i1.c: Regenerated.
	* generated/minval_i2.c: Regenerated.
	* generated/minval_i4.c: Regenerated.
	* generated/minval_i8.c: Regenerated.
	* generated/minval_r10.c: Regenerated.
	* generated/minval_r16.c: Regenerated.
	* generated/minval_r4.c: Regenerated.
	* generated/minval_r8.c: Regenerated.
	* generated/product_c10.c: Regenerated.
	* generated/product_c16.c: Regenerated.
	* generated/product_c4.c: Regenerated.
	* generated/product_c8.c: Regenerated.
	* generated/product_i16.c: Regenerated.
	* generated/product_i1.c: Regenerated.
	* generated/product_i2.c: Regenerated.
	* generated/product_i4.c: Regenerated.
	* generated/product_i8.c: Regenerated.
	* generated/product_r10.c: Regenerated.
	* generated/product_r16.c: Regenerated.
	* generated/product_r4.c: Regenerated.
	* generated/product_r8.c: Regenerated.
	* generated/sum_c10.c: Regenerated.
	* generated/sum_c16.c: Regenerated.
	* generated/sum_c4.c: Regenerated.
	* generated/sum_c8.c: Regenerated.
	* generated/sum_i16.c: Regenerated.
	* generated/sum_i1.c: Regenerated.
	* generated/sum_i2.c: Regenerated.
	* generated/sum_i4.c: Regenerated.
	* generated/sum_i8.c: Regenerated.
	* generated/sum_r10.c: Regenerated.
	* generated/sum_r16.c: Regenerated.
	* generated/sum_r4.c: Regenerated.
	* generated/sum_r8.c: Regenerated.
	* generated/transpose_c10.c: Regenerated.
	* generated/transpose_c16.c: Regenerated.
	* generated/transpose_c4.c: Regenerated.
	* generated/transpose_c8.c: Regenerated.
	* generated/transpose_i16.c: Regenerated.
	* generated/transpose_i4.c: Regenerated.
	* generated/transpose_i8.c: Regenerated.
	* generated/transpose_r10.c: Regenerated.
	* generated/transpose_r16.c: Regenerated.
	* generated/transpose_r4.c: Regenerated.
	* generated/transpose_r8.c: Regenerated.
	* generated/unpack_c10.c: Regenerated.
	* generated/unpack_c16.c: Regenerated.
	* generated/unpack_c4.c: Regenerated.
	* generated/unpack_c8.c: Regenerated.
	* generated/unpack_i16.c: Regenerated.
	* generated/unpack_i1.c: Regenerated.
	* generated/unpack_i2.c: Regenerated.
	* generated/unpack_i4.c: Regenerated.
	* generated/unpack_i8.c: Regenerated.
	* generated/unpack_r10.c: Regenerated.
	* generated/unpack_r16.c: Regenerated.
	* generated/unpack_r4.c: Regenerated.
	* generated/unpack_r8.c: Regenerated.
Janne Blomqvist - June 29, 2011, 7:04 a.m.
On Wed, Jun 29, 2011 at 00:50, Thomas Koenig <tkoenig@netcologne.de> wrote:
> Hello world,
>
> looking at PR 49479 and other functions in the library made me realize
> there are lots of places where we don't malloc one byte for empty
> arrays.

I'd prefer to add the zero check to runtime/memory.c
(internal_malloc_size), i.e. change

if (size == 0)
  return NULL;

to

if (size == 0)
  size = 1;

Or perhaps even better would be to add that check to get_mem() instead
(as a side effect making internal_malloc_size redundant), thus
matching how memory allocation works in the frontend? Looking at
get_mem() we actually have a small bug here; if size == 0 and malloc
happens to return NULL we treat it as an error.
Thomas Koenig - June 30, 2011, 6:09 p.m.
Hi Janne,

> I'd prefer to add the zero check to runtime/memory.c
> (internal_malloc_size), i.e. change
>
> if (size == 0)
>    return NULL;
>
> to
>
> if (size == 0)
>    size = 1;
>

Good point.  I have done so in the attached patch, plus removed
all special cases for checking for zero size.

Regression-tested.  OK for trunk?

For 4.6, I would just commit the change to internal_malloc_size
(which would also fix PR 49479), plus the test case for that
PR.

OK?

Regards

	Thomas
Janne Blomqvist - July 1, 2011, 12:34 p.m.
On Thu, Jun 30, 2011 at 21:09, Thomas Koenig <tkoenig@netcologne.de> wrote:
> Good point.  I have done so in the attached patch

Seems you forgot to attach it.. ;)

Patch

Index: m4/in_pack.m4
===================================================================
--- m4/in_pack.m4	(Revision 175598)
+++ m4/in_pack.m4	(Arbeitskopie)
@@ -45,6 +45,7 @@  internal_pack_'rtype_ccode` ('rtype` * source)
   index_type stride0;
   index_type dim;
   index_type ssize;
+  index_type alloc_size;
   const 'rtype_name` *src;
   'rtype_name` * restrict dest;
   'rtype_name` *destptr;
@@ -79,7 +80,12 @@  internal_pack_'rtype_ccode` ('rtype` * source)
     return source->data;
 
   /* Allocate storage for the destination.  */
-  destptr = ('rtype_name` *)internal_malloc_size (ssize * sizeof ('rtype_name`));
+  if (unlikely (ssize < 1))
+    alloc_size = 1;
+  else
+    alloc_size = ssize * sizeof ('rtype_name`);
+
+  destptr = ('rtype_name` *)internal_malloc_size (alloc_size);
   dest = destptr;
   src = source->data;
   stride0 = stride[0];
Index: m4/transpose.m4
===================================================================
--- m4/transpose.m4	(Revision 175598)
+++ m4/transpose.m4	(Arbeitskopie)
@@ -52,6 +52,8 @@  transpose_'rtype_code` ('rtype` * const restrict r
 
   if (ret->data == NULL)
     {
+      index_type alloc_size, array_size;
+
       assert (GFC_DESCRIPTOR_RANK (ret) == 2);
       assert (ret->dtype == source->dtype);
 
@@ -61,7 +63,13 @@  transpose_'rtype_code` ('rtype` * const restrict r
       GFC_DIMENSION_SET(ret->dim[1], 0, GFC_DESCRIPTOR_EXTENT(source,0) - 1,
 			GFC_DESCRIPTOR_EXTENT(source, 1));
 
-      ret->data = internal_malloc_size (sizeof ('rtype_name`) * size0 ((array_t *) ret));
+      array_size = size0 ((array_t *) ret);
+      if (unlikely (array_size < 1))
+        alloc_size = 1;
+      else
+        alloc_size = sizeof ('rtype_name`) * array_size;
+
+      ret->data = internal_malloc_size (alloc_size);
       ret->offset = 0;
     } else if (unlikely (compile_options.bounds_check))
     {
Index: m4/eoshift1.m4
===================================================================
--- m4/eoshift1.m4	(Revision 175598)
+++ m4/eoshift1.m4	(Arbeitskopie)
@@ -89,7 +89,6 @@  eoshift1 (gfc_array_char * const restrict ret,
     {
       int i;
 
-      ret->data = internal_malloc_size (size * arraysize);
       ret->offset = 0;
       ret->dtype = array->dtype;
       for (i = 0; i < GFC_DESCRIPTOR_RANK (array); i++)
@@ -107,7 +106,7 @@  eoshift1 (gfc_array_char * const restrict ret,
 	  GFC_DIMENSION_SET(ret->dim[i], 0, ub, str);
 
         }
-      if (arraysize > 0)
+      if (likely (arraysize > 0))
 	ret->data = internal_malloc_size (size * arraysize);
       else
 	ret->data = internal_malloc_size (1);
Index: m4/eoshift3.m4
===================================================================
--- m4/eoshift3.m4	(Revision 175598)
+++ m4/eoshift3.m4	(Arbeitskopie)
@@ -90,7 +90,6 @@  eoshift3 (gfc_array_char * const restrict ret,
     {
       int i;
 
-      ret->data = internal_malloc_size (size * arraysize);
       ret->offset = 0;
       ret->dtype = array->dtype;
       for (i = 0; i < GFC_DESCRIPTOR_RANK (array); i++)
Index: m4/cshift1.m4
===================================================================
--- m4/cshift1.m4	(Revision 175598)
+++ m4/cshift1.m4	(Arbeitskopie)
@@ -81,7 +81,6 @@  cshift1 (gfc_array_char * const restrict ret,
     {
       int i;
 
-      ret->data = internal_malloc_size (size * arraysize);
       ret->offset = 0;
       ret->dtype = array->dtype;
       for (i = 0; i < GFC_DESCRIPTOR_RANK (array); i++)
@@ -98,6 +97,11 @@  cshift1 (gfc_array_char * const restrict ret,
 
 	  GFC_DIMENSION_SET(ret->dim[i], 0, ub, str);
         }
+      if (likely (arraysize > 0))
+	ret->data = internal_malloc_size (size * arraysize);
+      else
+	ret->data = internal_malloc_size (1);
+
     }
   else if (unlikely (compile_options.bounds_check))
     {
Index: m4/matmull.m4
===================================================================
--- m4/matmull.m4	(Revision 175598)
+++ m4/matmull.m4	(Arbeitskopie)
@@ -68,6 +68,8 @@  matmul_'rtype_code` ('rtype` * const restrict reta
 
   if (retarray->data == NULL)
     {
+      index_type array_size, alloc_size;
+
       if (GFC_DESCRIPTOR_RANK (a) == 1)
         {
 	  GFC_DIMENSION_SET(retarray->dim[0], 0,
@@ -87,9 +89,15 @@  matmul_'rtype_code` ('rtype` * const restrict reta
 	                    GFC_DESCRIPTOR_EXTENT(b,1) - 1,
 			    GFC_DESCRIPTOR_EXTENT(retarray,0));
         }
-          
-      retarray->data
-	= internal_malloc_size (sizeof ('rtype_name`) * size0 ((array_t *) retarray));
+
+      array_size = size0 ((array_t *) retarray);
+
+      if (unlikely (array_size < 1))
+        alloc_size = 1;
+      else
+        alloc_size = sizeof ('rtype_name`) * alloc_size;
+
+      retarray->data = internal_malloc_size ( alloc_size );
       retarray->offset = 0;
     }
     else if (unlikely (compile_options.bounds_check))
Index: m4/unpack.m4
===================================================================
--- m4/unpack.m4	(Revision 175598)
+++ m4/unpack.m4	(Arbeitskopie)
@@ -86,6 +86,9 @@  unpack0_'rtype_code` ('rtype` *ret, const 'rtype`
     {
       /* The front end has signalled that we need to populate the
 	 return array descriptor.  */
+
+      index_type alloc_size;
+
       dim = GFC_DESCRIPTOR_RANK (mask);
       rs = 1;
       for (n = 0; n < dim; n++)
@@ -100,7 +103,13 @@  unpack0_'rtype_code` ('rtype` *ret, const 'rtype`
 	  rs *= extent[n];
 	}
       ret->offset = 0;
-      ret->data = internal_malloc_size (rs * sizeof ('rtype_name`));
+
+      if (unlikely (rs < 1))
+        alloc_size = 1;
+      else
+        alloc_size = rs * sizeof ('rtype_name`);
+
+      ret->data = internal_malloc_size (alloc_size);
     }
   else
     {
Index: m4/ifunction_logical.m4
===================================================================
--- m4/ifunction_logical.m4	(Revision 175598)
+++ m4/ifunction_logical.m4	(Arbeitskopie)
@@ -94,6 +94,7 @@  name`'rtype_qual`_'atype_code (rtype * const restr
 
       if (alloc_size == 0)
 	{
+	  retarray->data = internal_malloc_size (1);
 	  /* Make sure we have a zero-sized array.  */
 	  GFC_DIMENSION_SET(retarray->dim[0], 0, -1, 1);
 	  return;
Index: m4/ifunction.m4
===================================================================
--- m4/ifunction.m4	(Revision 175598)
+++ m4/ifunction.m4	(Arbeitskopie)
@@ -90,6 +90,7 @@  name`'rtype_qual`_'atype_code (rtype * const restr
 
       if (alloc_size == 0)
 	{
+	  retarray->data = internal_malloc_size (1);
 	  /* Make sure we have a zero-sized array.  */
 	  GFC_DIMENSION_SET(retarray->dim[0], 0, -1, 1);
 	  return;
@@ -269,6 +270,7 @@  void
 
       if (alloc_size == 0)
 	{
+	  retarray->data = internal_malloc_size (1);
 	  /* Make sure we have a zero-sized array.  */
 	  GFC_DIMENSION_SET(retarray->dim[0], 0, -1, 1);
 	  return;
Index: m4/matmul.m4
===================================================================
--- m4/matmul.m4	(Revision 175598)
+++ m4/matmul.m4	(Arbeitskopie)
@@ -104,6 +104,8 @@  matmul_'rtype_code` ('rtype` * const restrict reta
 
   if (retarray->data == NULL)
     {
+      index_type array_size, alloc_size;
+
       if (GFC_DESCRIPTOR_RANK (a) == 1)
         {
 	  GFC_DIMENSION_SET(retarray->dim[0], 0,
@@ -124,8 +126,14 @@  matmul_'rtype_code` ('rtype` * const restrict reta
 			    GFC_DESCRIPTOR_EXTENT(retarray,0));
         }
 
+      array_size = size0 ((array_t *) retarray);
+
+      if (unlikely (array_size < 1))
+        alloc_size = 1;
+      else
+        alloc_size = sizeof ('rtype_name`) * array_size;
       retarray->data
-	= internal_malloc_size (sizeof ('rtype_name`) * size0 ((array_t *) retarray));
+	= internal_malloc_size (alloc_size);
       retarray->offset = 0;
     }
     else if (unlikely (compile_options.bounds_check))
Index: intrinsics/transpose_generic.c
===================================================================
--- intrinsics/transpose_generic.c	(Revision 175598)
+++ intrinsics/transpose_generic.c	(Arbeitskopie)
@@ -52,6 +52,8 @@  transpose_internal (gfc_array_char *ret, gfc_array
 
   if (ret->data == NULL)
     {
+      index_type alloc_size, array_size;
+
       assert (ret->dtype == source->dtype);
 
       GFC_DIMENSION_SET(ret->dim[0], 0, GFC_DESCRIPTOR_EXTENT(source,1) - 1,
@@ -60,7 +62,14 @@  transpose_internal (gfc_array_char *ret, gfc_array
       GFC_DIMENSION_SET(ret->dim[1], 0, GFC_DESCRIPTOR_EXTENT(source,0) - 1,
 			GFC_DESCRIPTOR_EXTENT(source, 1));
 
-      ret->data = internal_malloc_size (size * size0 ((array_t*)ret));
+
+      array_size = size0 ((array_t*)ret);
+      if (unlikely (array_size < 1))
+	alloc_size = 1;
+      else
+	alloc_size = size * array_size;
+
+      ret->data = internal_malloc_size (alloc_size);
       ret->offset = 0;
     }
   else if (unlikely (compile_options.bounds_check))
Index: intrinsics/unpack_generic.c
===================================================================
--- intrinsics/unpack_generic.c	(Revision 175598)
+++ intrinsics/unpack_generic.c	(Arbeitskopie)
@@ -111,6 +111,8 @@  unpack_internal (gfc_array_char *ret, const gfc_ar
     {
       /* The front end has signalled that we need to populate the
 	 return array descriptor.  */
+      index_type alloc_size;
+
       dim = GFC_DESCRIPTOR_RANK (mask);
       rs = 1;
       for (n = 0; n < dim; n++)
@@ -126,7 +128,13 @@  unpack_internal (gfc_array_char *ret, const gfc_ar
 	  rs *= extent[n];
 	}
       ret->offset = 0;
-      ret->data = internal_malloc_size (rs * size);
+
+      if (unlikely (rs < 1))
+        alloc_size = 1;
+      else
+        alloc_size = rs * size;
+
+      ret->data = internal_malloc_size (alloc_size);
     }
   else
     {