diff mbox

C++/v3 PATCH to add/throw std::bad_array_new_length

Message ID 51841D0B.10402@redhat.com
State New
Headers show

Commit Message

Jason Merrill May 3, 2013, 8:24 p.m. UTC
Last year Florian fixed the compiler to detect overflow in array new 
size calculations and pass (size_t)-1 in that case.  But C++11 specifies 
that in case of overflow the program throws std::bad_array_new_length 
(http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#624), so 
I've adjusted the checking code accordingly.

This patch also adds the type to libsupc++, and several exports to 
libstdc++.

Tested x86_64-pc-linux-gnu, applying to trunk.

Comments

Florian Weimer May 6, 2013, 9:46 a.m. UTC | #1
On 05/03/2013 10:24 PM, Jason Merrill wrote:
> Last year Florian fixed the compiler to detect overflow in array new
> size calculations and pass (size_t)-1 in that case.  But C++11 specifies
> that in case of overflow the program throws std::bad_array_new_length
> (http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#624), so
> I've adjusted the checking code accordingly.

Nice, this is simpler than expected.  However, it makes the call sites 
even more bloated.

> This patch also adds the type to libsupc++, and several exports to
> libstdc++.

There's also overflow checking inside __cxa_vec_new[23].  At this point, 
we don't know if the caller was compiled in C++11 mode.  But for C++03 
code, throwing a subclass of std::bad_alloc probably won't hurt.

I noticed you use throw() in the declaration of 
std::bad_array_new_length and _GLIBCXX_USE_NOEXCEPT in the definition, 
which seems rather odd.  I'm surprised that this even compiles.
Jason Merrill May 6, 2013, 12:39 p.m. UTC | #2
On 05/06/2013 05:46 AM, Florian Weimer wrote:
> Nice, this is simpler than expected.  However, it makes the call sites
> even more bloated.

Hmm, perhaps the checking should be wrapped in an inline function, so 
that the inliner can decide whether or not to expand it at the call site...

>> This patch also adds the type to libsupc++, and several exports to
>> libstdc++.
>
> There's also overflow checking inside __cxa_vec_new[23].  At this point,
> we don't know if the caller was compiled in C++11 mode.  But for C++03
> code, throwing a subclass of std::bad_alloc probably won't hurt.

And we never use __cxa_vec_new* anyway, they're only there because the 
ABI requires them.

> I noticed you use throw() in the declaration of
> std::bad_array_new_length and _GLIBCXX_USE_NOEXCEPT in the definition,
> which seems rather odd.  I'm surprised that this even compiles.

15.4 [except.spec]/3:

Two exception-specifications are compatible if:
— both are non-throwing (see below), regardless of their form
...

Jason
Florian Weimer May 6, 2013, 12:46 p.m. UTC | #3
On 05/06/2013 02:39 PM, Jason Merrill wrote:
> On 05/06/2013 05:46 AM, Florian Weimer wrote:
>> Nice, this is simpler than expected.  However, it makes the call sites
>> even more bloated.
>
> Hmm, perhaps the checking should be wrapped in an inline function, so
> that the inliner can decide whether or not to expand it at the call site...

Or we could call __cxa_vec_new[23] and rely on the check there (in most 
cases—for new T[a][b], we'd still need a separate overflow check).

>>> This patch also adds the type to libsupc++, and several exports to
>>> libstdc++.
>>
>> There's also overflow checking inside __cxa_vec_new[23].  At this point,
>> we don't know if the caller was compiled in C++11 mode.  But for C++03
>> code, throwing a subclass of std::bad_alloc probably won't hurt.
>
> And we never use __cxa_vec_new* anyway, they're only there because the
> ABI requires them.

EDG-derived compilers will call it, so we should fix it as well.

>> I noticed you use throw() in the declaration of
>> std::bad_array_new_length and _GLIBCXX_USE_NOEXCEPT in the definition,
>> which seems rather odd.  I'm surprised that this even compiles.
>
> 15.4 [except.spec]/3:
>
> Two exception-specifications are compatible if:
> — both are non-throwing (see below), regardless of their form
> ...

Thanks, I suspected as much.
Jason Merrill May 6, 2013, 3:56 p.m. UTC | #4
On 05/06/2013 08:46 AM, Florian Weimer wrote:
> On 05/06/2013 02:39 PM, Jason Merrill wrote:
>> On 05/06/2013 05:46 AM, Florian Weimer wrote:
>>> Nice, this is simpler than expected.  However, it makes the call sites
>>> even more bloated.
>>
>> Hmm, perhaps the checking should be wrapped in an inline function, so
>> that the inliner can decide whether or not to expand it at the call
>> site...
>
> Or we could call __cxa_vec_new[23] and rely on the check there

True.  The problem with using those is the indirect calls to the 
(possibly inline) constructors, though it might be worth doing 
conditionally.  Would you be interested in working on that change?

> (in most
> cases—for new T[a][b], we'd still need a separate overflow check).

But new T[a][b] is ill-formed, so we don't need to handle that case.

>>>> This patch also adds the type to libsupc++, and several exports to
>>>> libstdc++.
>>>
>>> There's also overflow checking inside __cxa_vec_new[23].  At this point,
>>> we don't know if the caller was compiled in C++11 mode.  But for C++03
>>> code, throwing a subclass of std::bad_alloc probably won't hurt.
>>
>> And we never use __cxa_vec_new* anyway, they're only there because the
>> ABI requires them.
>
> EDG-derived compilers will call it, so we should fix it as well.

Right.

Jason
Florian Weimer May 13, 2013, 8:41 a.m. UTC | #5
On 05/06/2013 05:56 PM, Jason Merrill wrote:
> On 05/06/2013 08:46 AM, Florian Weimer wrote:
>> On 05/06/2013 02:39 PM, Jason Merrill wrote:
>>> On 05/06/2013 05:46 AM, Florian Weimer wrote:
>>>> Nice, this is simpler than expected.  However, it makes the call sites
>>>> even more bloated.
>>>
>>> Hmm, perhaps the checking should be wrapped in an inline function, so
>>> that the inliner can decide whether or not to expand it at the call
>>> site...
>>
>> Or we could call __cxa_vec_new[23] and rely on the check there
>
> True.  The problem with using those is the indirect calls to the
> (possibly inline) constructors, though it might be worth doing
> conditionally.  Would you be interested in working on that change?

Yes, but it's probably better if you commit your patch right away.

>> (in most
>> cases—for new T[a][b], we'd still need a separate overflow check).
>
> But new T[a][b] is ill-formed, so we don't need to handle that case.

I meant with one of a or b as a constant (I can't remember which it is). 
  We still have to perform one multiplication inline, to get the total 
number of elements, and that needs overflow checking as well.
diff mbox

Patch

commit 9c81583687db4d3678ac2e7ee78c7c15076e7ac4
Author: Jason Merrill <jason@redhat.com>
Date:   Thu May 2 11:31:05 2013 -0400

    	Core 624/N2932: Throw bad_array_new_length on overflow
    	in array new size calculation.
    
    libstdc++-v3/
    	* libsupc++/new: Add std::bad_array_new_length.
    	* libsupc++/bad_array_new.cc: New.
    	* libsupc++/eh_aux_runtime.cc: Add __cxa_bad_array_new_length.
    	* libsupc++/Makefile.in: Build them.
    	* config/abi/pre/gnu.ver: Add new symbols.
    	* config/abi/pre/gnu-versioned-namespace.ver: Add new symbols.
    gcc/cp/
    	* init.c (throw_bad_array_new_length): New.
    	(build_new_1): Use it.  Don't warn about braced-init-list.
    	(build_vec_init): Use it.
    	* call.c (build_operator_new_call): Use it.

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 88bf100..bd8f531 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -3951,8 +3951,13 @@  build_operator_new_call (tree fnname, vec<tree, va_gc> **args,
     *fn = NULL_TREE;
   /* Set to (size_t)-1 if the size check fails.  */
   if (size_check != NULL_TREE)
-    *size = fold_build3 (COND_EXPR, sizetype, size_check,
-			 original_size, TYPE_MAX_VALUE (sizetype));
+    {
+      tree errval = TYPE_MAX_VALUE (sizetype);
+      if (cxx_dialect >= cxx11)
+	errval = throw_bad_array_new_length ();
+      *size = fold_build3 (COND_EXPR, sizetype, size_check,
+			   original_size, errval);
+    }
   vec_safe_insert (*args, 0, *size);
   *args = resolve_args (*args, complain);
   if (*args == NULL)
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 78fd56b..dec7390 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5357,6 +5357,7 @@  extern tree build_value_init			(tree, tsubst_flags_t);
 extern tree build_value_init_noctor		(tree, tsubst_flags_t);
 extern tree build_offset_ref			(tree, tree, bool,
 						 tsubst_flags_t);
+extern tree throw_bad_array_new_length		(void);
 extern tree build_new				(vec<tree, va_gc> **, tree, tree,
 						 vec<tree, va_gc> **, int,
                                                  tsubst_flags_t);
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 3587b08..73186eb 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -2170,6 +2170,21 @@  diagnose_uninitialized_cst_or_ref_member (tree type, bool using_new, bool compla
   return diagnose_uninitialized_cst_or_ref_member_1 (type, type, using_new, complain);
 }
 
+/* Call __cxa_bad_array_new_length to indicate that the size calculation
+   overflowed.  Pretend it returns sizetype so that it plays nicely in the
+   COND_EXPR.  */
+
+tree
+throw_bad_array_new_length (void)
+{
+  tree fn = get_identifier ("__cxa_bad_array_new_length");
+  if (!get_global_value_if_present (fn, &fn))
+    fn = push_throw_library_fn (fn, build_function_type_list (sizetype,
+							      NULL_TREE));
+
+  return build_cxx_call (fn, 0, NULL, tf_warning_or_error);
+}
+
 /* Generate code for a new-expression, including calling the "operator
    new" function, initializing the object, and, if an exception occurs
    during construction, cleaning up.  The arguments are as for
@@ -2472,9 +2487,12 @@  build_new_1 (vec<tree, va_gc> **placement, tree type, tree nelts,
 		outer_nelts_check = NULL_TREE;
 	    }
 	  /* Perform the overflow check.  */
+	  tree errval = TYPE_MAX_VALUE (sizetype);
+	  if (cxx_dialect >= cxx11)
+	    errval = throw_bad_array_new_length ();
 	  if (outer_nelts_check != NULL_TREE)
             size = fold_build3 (COND_EXPR, sizetype, outer_nelts_check,
-                                size, TYPE_MAX_VALUE (sizetype));
+                                size, errval);
 	  /* Create the argument list.  */
 	  vec_safe_insert (*placement, 0, size);
 	  /* Do name-lookup to find the appropriate operator.  */
@@ -2699,12 +2717,8 @@  build_new_1 (vec<tree, va_gc> **placement, tree type, tree nelts,
 		    domain = compute_array_index_type (NULL_TREE, nelts,
 						       complain);
 		  else
-		    {
-		      domain = NULL_TREE;
-		      if (CONSTRUCTOR_NELTS (vecinit) > 0)
-			warning (0, "non-constant array size in new, unable "
-				 "to verify length of initializer-list");
-		    }
+		    /* We'll check the length at runtime.  */
+		    domain = NULL_TREE;
 		  arraytype = build_cplus_array_type (type, domain);
 		  vecinit = digest_init (arraytype, vecinit, complain);
 		}
@@ -3291,6 +3305,7 @@  build_vec_init (tree base, tree maxindex, tree init,
   tree obase = base;
   bool xvalue = false;
   bool errors = false;
+  tree length_check = NULL_TREE;
 
   if (TREE_CODE (atype) == ARRAY_TYPE && TYPE_DOMAIN (atype))
     maxindex = array_type_nelts (atype);
@@ -3309,6 +3324,14 @@  build_vec_init (tree base, tree maxindex, tree init,
       && from_array != 2)
     init = TARGET_EXPR_INITIAL (init);
 
+  /* If we have a braced-init-list, make sure that the array
+     is big enough for all the initializers.  */
+  if (init && TREE_CODE (init) == CONSTRUCTOR
+      && CONSTRUCTOR_NELTS (init) > 0
+      && !TREE_CONSTANT (maxindex))
+    length_check = fold_build2 (LT_EXPR, boolean_type_node, maxindex,
+				ssize_int (CONSTRUCTOR_NELTS (init) - 1));
+
   if (init
       && TREE_CODE (atype) == ARRAY_TYPE
       && (from_array == 2
@@ -3441,6 +3464,15 @@  build_vec_init (tree base, tree maxindex, tree init,
       vec<constructor_elt, va_gc> *new_vec;
       from_array = 0;
 
+      if (length_check)
+	{
+	  tree throw_call;
+	    throw_call = throw_bad_array_new_length ();
+	  length_check = build3 (COND_EXPR, void_type_node, length_check,
+				 throw_call, void_zero_node);
+	  finish_expr_stmt (length_check);
+	}
+
       if (try_const)
 	vec_alloc (new_vec, CONSTRUCTOR_NELTS (init));
       else
diff --git a/gcc/testsuite/g++.dg/cpp0x/bad_array_new1.C b/gcc/testsuite/g++.dg/cpp0x/bad_array_new1.C
new file mode 100644
index 0000000..cd5f0c8
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/bad_array_new1.C
@@ -0,0 +1,20 @@ 
+// Test for throwing bad_array_new_length on invalid array length
+// { dg-options -std=c++11 }
+// { dg-do run }
+
+#include <new>
+
+void * f(int i)
+{
+  return new int[i];
+}
+
+int main()
+{
+  try
+    {
+      f(-1);
+    }
+  catch (std::bad_array_new_length) { return 0; }
+  __builtin_abort ();
+}
diff --git a/gcc/testsuite/g++.dg/cpp0x/bad_array_new2.C b/gcc/testsuite/g++.dg/cpp0x/bad_array_new2.C
new file mode 100644
index 0000000..ab36510
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/bad_array_new2.C
@@ -0,0 +1,21 @@ 
+// Test for throwing bad_array_new_length on invalid array length
+// { dg-options -std=c++11 }
+// { dg-do run }
+
+#include <new>
+
+void * f(int i)
+{
+  return new int[i]{1,2,3,4};
+}
+
+int main()
+{
+  f(4);				// OK
+  try
+    {
+      f(3);
+    }
+  catch (std::bad_array_new_length) { return 0; }
+  __builtin_abort ();
+}
diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist21.C b/gcc/testsuite/g++.dg/cpp0x/initlist21.C
index 9412a08..16923f8 100644
--- a/gcc/testsuite/g++.dg/cpp0x/initlist21.C
+++ b/gcc/testsuite/g++.dg/cpp0x/initlist21.C
@@ -12,7 +12,6 @@  class X
 int f(int n)
 {
   const float * pData = new const float[1] { 1.5, 2.5 }; // { dg-error "too many initializers" }
-  pData = new const float[n] { 1.5, 2.5 }; // { dg-warning "array size" }
 
   return 0;
 }
diff --git a/gcc/testsuite/g++.dg/init/new40.C b/gcc/testsuite/g++.dg/init/new40.C
index 4b283a4..026712d 100644
--- a/gcc/testsuite/g++.dg/init/new40.C
+++ b/gcc/testsuite/g++.dg/init/new40.C
@@ -1,5 +1,7 @@ 
 // Testcase for overflow handling in operator new[].
 // Optimization of unnecessary overflow checks.
+// In C++11 we throw bad_array_new_length instead.
+// { dg-options -std=c++03 }
 // { dg-do run }
 
 #include <assert.h>
diff --git a/libstdc++-v3/config/abi/pre/gnu-versioned-namespace.ver b/libstdc++-v3/config/abi/pre/gnu-versioned-namespace.ver
index 7880767..122ebb8 100644
--- a/libstdc++-v3/config/abi/pre/gnu-versioned-namespace.ver
+++ b/libstdc++-v3/config/abi/pre/gnu-versioned-namespace.ver
@@ -232,6 +232,9 @@  CXXABI_2.0 {
     _ZTSSt17bad_function_call;
     _ZTVSt17bad_function_call;
 
+    __cxa_bad_array_new_length;
+    _Z*St20bad_array_new_length*;
+
     # Default function.
     _ZSt11_Hash_bytesPKv*;
 
diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
index 978641f..84b0b91 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -1556,6 +1556,10 @@  CXXABI_1.3.7 {
     __cxa_thread_atexit;
 } CXXABI_1.3.6;
 
+CXXABI_1.3.8 {
+    __cxa_bad_array_new_length;
+    _Z*St20bad_array_new_length*;
+} CXXABI_1.3.7;
 
 # Symbols in the support library (libsupc++) supporting transactional memory.
 CXXABI_TM_1 {
diff --git a/libstdc++-v3/libsupc++/Makefile.in b/libstdc++-v3/libsupc++/Makefile.in
index eb13f1e..9f24fef 100644
--- a/libstdc++-v3/libsupc++/Makefile.in
+++ b/libstdc++-v3/libsupc++/Makefile.in
@@ -92,7 +92,8 @@  am__installdirs = "$(DESTDIR)$(toolexeclibdir)" "$(DESTDIR)$(bitsdir)" \
 LTLIBRARIES = $(noinst_LTLIBRARIES) $(toolexeclib_LTLIBRARIES)
 libsupc___la_LIBADD =
 am__objects_1 = array_type_info.lo atexit_arm.lo atexit_thread.lo \
-	bad_alloc.lo bad_cast.lo bad_typeid.lo class_type_info.lo \
+	bad_alloc.lo bad_array_new.lo bad_cast.lo bad_typeid.lo \
+	class_type_info.lo \
 	del_op.lo del_opnt.lo del_opv.lo del_opvnt.lo dyncast.lo \
 	eh_alloc.lo eh_arm.lo eh_aux_runtime.lo eh_call.lo eh_catch.lo \
 	eh_exception.lo eh_globals.lo eh_personality.lo eh_ptr.lo \
@@ -366,6 +367,7 @@  sources = \
 	atexit_arm.cc \
 	atexit_thread.cc \
 	bad_alloc.cc \
+	bad_array_new.cc \
 	bad_cast.cc \
 	bad_typeid.cc \
 	class_type_info.cc \
@@ -788,6 +790,16 @@  cp-demangle.o: cp-demangle.c
 	$(C_COMPILE) -DIN_GLIBCPP_V3 -Wno-error -c $<
 
 # Use special rules for the C++11 sources so that the proper flags are passed.
+bad_array_new.lo: bad_array_new.cc
+	$(LTCXXCOMPILE) -std=gnu++11 -c $<
+bad_array_new.o: bad_array_new.cc
+	$(CXXCOMPILE) -std=gnu++11 -c $<
+
+eh_aux_runtime.lo: eh_aux_runtime.cc
+	$(LTCXXCOMPILE) -std=gnu++11 -c $<
+eh_aux_runtime.o: eh_aux_runtime.cc
+	$(CXXCOMPILE) -std=gnu++11 -c $<
+
 eh_ptr.lo: eh_ptr.cc
 	$(LTCXXCOMPILE) -std=gnu++11 -c $<
 eh_ptr.o: eh_ptr.cc
diff --git a/libstdc++-v3/libsupc++/bad_array_new.cc b/libstdc++-v3/libsupc++/bad_array_new.cc
new file mode 100644
index 0000000..5282f52
--- /dev/null
+++ b/libstdc++-v3/libsupc++/bad_array_new.cc
@@ -0,0 +1,36 @@ 
+// Copyright (C) 2013 Free Software Foundation, Inc.
+//
+// This file is part of GCC.
+//
+// GCC is free software; you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// GCC is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// Under Section 7 of GPL version 3, you are granted additional
+// permissions described in the GCC Runtime Library Exception, version
+// 3.1, as published by the Free Software Foundation.
+
+// You should have received a copy of the GNU General Public License and
+// a copy of the GCC Runtime Library Exception along with this program;
+// see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
+// <http://www.gnu.org/licenses/>.
+
+#include <new>
+
+namespace std {
+
+bad_array_new_length::~bad_array_new_length() _GLIBCXX_USE_NOEXCEPT { }
+
+const char*
+bad_array_new_length::what() const _GLIBCXX_USE_NOEXCEPT
+{
+  return "std::bad_array_new_length";
+}
+
+} // namespace std
diff --git a/libstdc++-v3/libsupc++/cxxabi.h b/libstdc++-v3/libsupc++/cxxabi.h
index f5301c0..a062a46 100644
--- a/libstdc++-v3/libsupc++/cxxabi.h
+++ b/libstdc++-v3/libsupc++/cxxabi.h
@@ -151,6 +151,9 @@  namespace __cxxabiv1
   void 
   __cxa_bad_typeid() __attribute__((__noreturn__));
 
+  void
+  __cxa_bad_array_new_length() __attribute__((__noreturn__));
+
 
   /**
    *  @brief Demangling routine.
diff --git a/libstdc++-v3/libsupc++/eh_aux_runtime.cc b/libstdc++-v3/libsupc++/eh_aux_runtime.cc
index 7798014..dcfef6b 100644
--- a/libstdc++-v3/libsupc++/eh_aux_runtime.cc
+++ b/libstdc++-v3/libsupc++/eh_aux_runtime.cc
@@ -24,6 +24,7 @@ 
 
 #include "typeinfo"
 #include "exception"
+#include "new"
 #include <cstdlib>
 #include "unwind-cxx.h"
 #include <bits/exception_defines.h>
@@ -36,3 +37,6 @@  extern "C" void
 __cxxabiv1::__cxa_bad_typeid ()
 { _GLIBCXX_THROW_OR_ABORT(std::bad_typeid()); }
 
+extern "C" void
+__cxxabiv1::__cxa_bad_array_new_length ()
+{ _GLIBCXX_THROW_OR_ABORT(std::bad_array_new_length()); }
diff --git a/libstdc++-v3/libsupc++/new b/libstdc++-v3/libsupc++/new
index e3f0f77..3087502 100644
--- a/libstdc++-v3/libsupc++/new
+++ b/libstdc++-v3/libsupc++/new
@@ -64,6 +64,21 @@  namespace std
     virtual const char* what() const throw();
   };
 
+#if __cplusplus >= 201103L
+  class bad_array_new_length : public bad_alloc
+  {
+  public:
+    bad_array_new_length() throw() { };
+
+    // This declaration is not useless:
+    // http://gcc.gnu.org/onlinedocs/gcc-3.0.2/gcc_6.html#SEC118
+    virtual ~bad_array_new_length() throw();
+
+    // See comment in eh_exception.cc.
+    virtual const char* what() const throw();
+  };
+#endif
+
   struct nothrow_t { };
 
   extern const nothrow_t nothrow;