Patchwork [C++] Omit overflow check for new char[n]

login
register
mail settings
Submitter Florian Weimer
Date Oct. 8, 2012, 3:50 p.m.
Message ID <5072F64A.6030509@redhat.com>
Download mbox | patch
Permalink /patch/190053/
State New
Headers show

Comments

Florian Weimer - Oct. 8, 2012, 3:50 p.m.
If the size of the inner array elements is 1 and we do not need a 
cookie, we do not need to insert an overflow check.  This applies to the 
relatively frequent new char[n] case.

Built and regression-tested on x86_64-redhat-linux-gnu.  Okay for trunk?
Dodji Seketeli - Oct. 10, 2012, 4:02 p.m.
Hello Florian,

Let's CC Jason for this optimization patch.

Florian Weimer <fweimer@redhat.com> a écrit:

> If the size of the inner array elements is 1 and we do not need a
> cookie, we do not need to insert an overflow check.  This applies to
> the relatively frequent new char[n] case.

I just have one question for own education.

Regarding:

@@ -2450,7 +2450,13 @@
 	  if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type))
 	    size = size_binop (PLUS_EXPR, size, cookie_size);
 	  else
-	    cookie_size = NULL_TREE;
+	    {
+	      cookie_size = NULL_TREE;
+	      /* No size arithmetic necessary, so the size check is
+		 not needed. */
+	      if (outer_nelts_check != NULL && inner_size == double_int_one)
+		outer_nelts_check = NULL_TREE;
+	    }

I couldn't find where in code is TYPE_VEC_NEW_USES_COOKIE is set.  Is it
still used?
Florian Weimer - Oct. 10, 2012, 4:06 p.m.
On 10/10/2012 06:02 PM, Dodji Seketeli wrote:

> I just have one question for own education.
>
> Regarding:
>
> @@ -2450,7 +2450,13 @@
>   	  if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type))
>   	    size = size_binop (PLUS_EXPR, size, cookie_size);
>   	  else
> -	    cookie_size = NULL_TREE;
> +	    {
> +	      cookie_size = NULL_TREE;
> +	      /* No size arithmetic necessary, so the size check is
> +		 not needed. */
> +	      if (outer_nelts_check != NULL && inner_size == double_int_one)
> +		outer_nelts_check = NULL_TREE;
> +	    }
>
> I couldn't find where in code is TYPE_VEC_NEW_USES_COOKIE is set.  Is it
> still used?

It's set in gcc/cp/class.c:

    5191   /* Figure out whether or not we will need a cookie when 
dynamically
    5192      allocating an array of this type.  */
    5193   TYPE_LANG_SPECIFIC (t)->u.c.vec_new_uses_cookie
    5194     = type_requires_array_cookie (t);

I'm not sure if we've got proper test coverage for the concrete cookie 
value, but the test case I've included implicitly check if there's a 
cookie if there's a non-trivial destructor.
Dodji Seketeli - Oct. 11, 2012, 7:18 a.m.
Florian Weimer <fweimer@redhat.com> a écrit:

> On 10/10/2012 06:02 PM, Dodji Seketeli wrote:
>
>> I just have one question for own education.
>>
>> Regarding:
>>
>> @@ -2450,7 +2450,13 @@
>>   	  if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type))
>>   	    size = size_binop (PLUS_EXPR, size, cookie_size);
>>   	  else
>> -	    cookie_size = NULL_TREE;
>> +	    {
>> +	      cookie_size = NULL_TREE;
>> +	      /* No size arithmetic necessary, so the size check is
>> +		 not needed. */
>> +	      if (outer_nelts_check != NULL && inner_size == double_int_one)
>> +		outer_nelts_check = NULL_TREE;
>> +	    }
>>
>> I couldn't find where in code is TYPE_VEC_NEW_USES_COOKIE is set.  Is it
>> still used?
>
> It's set in gcc/cp/class.c:
>
>    5191   /* Figure out whether or not we will need a cookie when
> dynamically
>    5192      allocating an array of this type.  */
>    5193   TYPE_LANG_SPECIFIC (t)->u.c.vec_new_uses_cookie
>    5194     = type_requires_array_cookie (t);

Ah, I see.  So maybe that statement should be trivially replaced by:

    TYPE_VEC_NEW_USES_COOKIE (t) = type_requires_array_cookie (t);

for better maintainability?

> I'm not sure if we've got proper test coverage for the concrete cookie
> value, but the test case I've included implicitly check if there's a
> cookie if there's a non-trivial destructor.

I see, Thanks.
Jakub Jelinek - Oct. 31, 2012, 9:05 a.m.
On Mon, Oct 08, 2012 at 05:50:34PM +0200, Florian Weimer wrote:
> gcc/:
> 
> 2012-10-08  Florian Weimer  <fweimer@redhat.com>
> 
> 	* init.c (build_new_1): Do not check for arithmetic overflow if
> 	inner array size is 1.
> 
> gcc/testsuite/:
> 
> 2012-10-08  Florian Weimer  <fweimer@redhat.com>
> 
> 	* g++.dg/init/new40.C: New.

> @@ -2450,7 +2450,13 @@
>  	  if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type))
>  	    size = size_binop (PLUS_EXPR, size, cookie_size);
>  	  else
> -	    cookie_size = NULL_TREE;
> +	    {
> +	      cookie_size = NULL_TREE;
> +	      /* No size arithmetic necessary, so the size check is
> +		 not needed. */
> +	      if (outer_nelts_check != NULL && inner_size == double_int_one)
> +		outer_nelts_check = NULL_TREE;
> +	    }

I don't see any == double_int_one (or zero) comparisons in grep,
so I'd say inner_size.is_one () should be used instead (which is used
pretty frequently).  Ditto in the second spot.
Otherwise the patch looks good to me, but I'd like Jason to chime in too.

>  	  /* Perform the overflow check.  */
>  	  if (outer_nelts_check != NULL_TREE)
>              size = fold_build3 (COND_EXPR, sizetype, outer_nelts_check,
> @@ -2486,7 +2492,13 @@
>  	  /* Use a global operator new.  */
>  	  /* See if a cookie might be required.  */
>  	  if (!(array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type)))
> -	    cookie_size = NULL_TREE;
> +	    {
> +	      cookie_size = NULL_TREE;
> +	      /* No size arithmetic necessary, so the size check is
> +		 not needed. */
> +	      if (outer_nelts_check != NULL && inner_size == double_int_one)
> +		outer_nelts_check = NULL_TREE;
> +	    }
>  
>  	  alloc_call = build_operator_new_call (fnname, placement,
>  						&size, &cookie_size,

	Jakub

Patch

gcc/:

2012-10-08  Florian Weimer  <fweimer@redhat.com>

	* init.c (build_new_1): Do not check for arithmetic overflow if
	inner array size is 1.

gcc/testsuite/:

2012-10-08  Florian Weimer  <fweimer@redhat.com>

	* g++.dg/init/new40.C: New.

Index: gcc/cp/ChangeLog
===================================================================
--- gcc/cp/ChangeLog	(revision 192206)
+++ gcc/cp/ChangeLog	(working copy)
@@ -1,3 +1,8 @@ 
+2012-10-08  Florian Weimer  <fweimer@redhat.com>
+
+	* init.c (build_new_1): Do not check for arithmetic overflow if
+	inner array size is 1.
+
 2012-10-08  Dodji Seketeli  <dodji@redhat.com>
 
 	PR c++/53528 C++11 attribute support
Index: gcc/cp/init.c
===================================================================
--- gcc/cp/init.c	(revision 192206)
+++ gcc/cp/init.c	(working copy)
@@ -2184,6 +2184,8 @@ 
   bool outer_nelts_from_type = false;
   double_int inner_nelts_count = double_int_one;
   tree alloc_call, alloc_expr;
+  /* Size of the inner array elements. */
+  double_int inner_size;
   /* The address returned by the call to "operator new".  This node is
      a VAR_DECL and is therefore reusable.  */
   tree alloc_node;
@@ -2345,8 +2347,6 @@ 
       double_int max_size
 	= double_int_one.llshift (TYPE_PRECISION (sizetype) - 1,
 				  HOST_BITS_PER_DOUBLE_INT);
-      /* Size of the inner array elements. */
-      double_int inner_size;
       /* Maximum number of outer elements which can be allocated. */
       double_int max_outer_nelts;
       tree max_outer_nelts_tree;
@@ -2450,7 +2450,13 @@ 
 	  if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type))
 	    size = size_binop (PLUS_EXPR, size, cookie_size);
 	  else
-	    cookie_size = NULL_TREE;
+	    {
+	      cookie_size = NULL_TREE;
+	      /* No size arithmetic necessary, so the size check is
+		 not needed. */
+	      if (outer_nelts_check != NULL && inner_size == double_int_one)
+		outer_nelts_check = NULL_TREE;
+	    }
 	  /* Perform the overflow check.  */
 	  if (outer_nelts_check != NULL_TREE)
             size = fold_build3 (COND_EXPR, sizetype, outer_nelts_check,
@@ -2486,7 +2492,13 @@ 
 	  /* Use a global operator new.  */
 	  /* See if a cookie might be required.  */
 	  if (!(array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type)))
-	    cookie_size = NULL_TREE;
+	    {
+	      cookie_size = NULL_TREE;
+	      /* No size arithmetic necessary, so the size check is
+		 not needed. */
+	      if (outer_nelts_check != NULL && inner_size == double_int_one)
+		outer_nelts_check = NULL_TREE;
+	    }
 
 	  alloc_call = build_operator_new_call (fnname, placement,
 						&size, &cookie_size,
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog	(revision 192206)
+++ gcc/testsuite/ChangeLog	(working copy)
@@ -1,3 +1,7 @@ 
+2012-10-08  Florian Weimer  <fweimer@redhat.com>
+
+	* g++.dg/init/new40.C: New.
+
 2012-10-08  Oleg Endo  <olegendo@gcc.gnu.org>
 
 	PR target/54685
Index: gcc/testsuite/g++.dg/init/new40.C
===================================================================
--- gcc/testsuite/g++.dg/init/new40.C	(revision 0)
+++ gcc/testsuite/g++.dg/init/new40.C	(working copy)
@@ -0,0 +1,112 @@ 
+// Testcase for overflow handling in operator new[].
+// Optimization of unnecessary overflow checks.
+// { dg-do run }
+
+#include <assert.h>
+#include <stdlib.h>
+#include <stdexcept>
+
+static size_t magic_allocation_size
+  = 1 + (size_t (1) << (sizeof (size_t) * 8 - 1));
+
+struct exc : std::bad_alloc {
+};
+
+static size_t expected_size;
+
+struct pod_with_new {
+  char ch;
+  void *operator new[] (size_t sz)
+  {
+    if (sz != expected_size)
+      abort ();
+    throw exc ();
+  }
+};
+
+struct with_new {
+  char ch;
+  with_new () { }
+  ~with_new () { }
+  void *operator new[] (size_t sz)
+  {
+    if (sz != size_t (-1))
+      abort ();
+    throw exc ();
+  }
+};
+
+struct non_pod {
+  char ch;
+  non_pod () { }
+  ~non_pod () { }
+};
+
+void *
+operator new (size_t sz) _GLIBCXX_THROW (std::bad_alloc)
+{
+  if (sz != expected_size)
+    abort ();
+  throw exc ();
+}
+
+int
+main ()
+{
+  if (sizeof (pod_with_new) == 1)
+    expected_size = magic_allocation_size;
+  else
+    expected_size = -1;
+
+  try {
+    new pod_with_new[magic_allocation_size];
+    abort ();
+  } catch (exc &) {
+  }
+
+  if (sizeof (with_new) == 1)
+    expected_size = magic_allocation_size;
+  else
+    expected_size = -1;
+
+  try {
+    new with_new[magic_allocation_size];
+    abort ();
+  } catch (exc &) {
+  }
+
+  expected_size = magic_allocation_size;
+  try {
+    new char[magic_allocation_size];
+    abort ();
+  } catch (exc &) {
+  }
+
+  expected_size = -1;
+
+  try {
+    new pod_with_new[magic_allocation_size][2];
+    abort ();
+  } catch (exc &) {
+  }
+
+  try {
+    new with_new[magic_allocation_size][2];
+    abort ();
+  } catch (exc &) {
+  }
+
+  try {
+    new char[magic_allocation_size][2];
+    abort ();
+  } catch (exc &) {
+  }
+
+  try {
+    new non_pod[magic_allocation_size];
+    abort ();
+  } catch (exc &) {
+  }
+
+  return 0;
+}