Re: [C++] Omit overflow check for new char[n]

2012-11-06 Thread Jason Merrill 

OK.

Jason

Re: [C++] Omit overflow check for new char[n]

2012-10-31 Thread Jakub Jelinek 
On Mon, Oct 08, 2012 at 05:50:34PM +0200, Florian Weimer wrote:
 gcc/:
 
 2012-10-08  Florian Weimer  fwei...@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  fwei...@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

Re: [C++] Omit overflow check for new char[n]

2012-10-31 Thread Florian Weimer 

On 10/31/2012 10:05 AM, Jakub Jelinek wrote:


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.


Thanks, I'm attaching the updated patch.

Bootstrapped and tested on x86_64-redhat-linux-gnu, with no apparent 
regressions.


--
Florian Weimer / Red Hat Product Security Team
gcc/:

2012-10-31  Florian Weimer  fwei...@redhat.com

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

gcc/testsuite/:

2012-10-31  Florian Weimer  fwei...@redhat.com

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

Index: gcc/cp/init.c
===
--- gcc/cp/init.c	(revision 193033)
+++ gcc/cp/init.c	(working copy)
@@ -2185,6 +2185,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;
@@ -2346,8 +2348,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;
@@ -2451,7 +2451,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.is_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,
@@ -2487,7 +2493,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.is_one())
+		outer_nelts_check = NULL_TREE;
+	}
 
 	  alloc_call = build_operator_new_call (fnname, placement,
 		size, cookie_size,
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;
+}

Re: [C++] Omit overflow check for new char[n]

2012-10-31 Thread Florian Weimer 

On 10/31/2012 06:13 PM, Florian Weimer wrote:


+ if (outer_nelts_check != NULL  inner_size.is_one())


Uhm, I will add the missing space before commit.  Sorry.

--
Florian Weimer / Red Hat Product Security Team

Re: [C++] Omit overflow check for new char[n]

2012-10-11 Thread Dodji Seketeli 
Florian Weimer fwei...@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.

-- 
Dodji

Re: [C++] Omit overflow check for new char[n]

2012-10-10 Thread Dodji Seketeli 
Hello Florian,

Let's CC Jason for this optimization patch.

Florian Weimer fwei...@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?

-- 
Dodji

Re: [C++] Omit overflow check for new char[n]

2012-10-10 Thread Florian Weimer 

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.


--
Florian Weimer / Red Hat Product Security Team