| Message ID | 87ipwd6quz.fsf@mid.deneb.enyo.de |
|---|---|
| State | New |
| Headers | show |
* Florian Weimer: > I have run "make check-c++" with no new failures on x86_64-gnu-linux > twice, with the operator new[] check enabled and disabled; there were > no new failures. If the check is disabled, trunk and patch produce > identical assembler code for the test case. > > I still need some guidance where to put the test case. There don't > seem to be any direct tests for operator new[] funcionality. > > 2011-02-21 Florian Weimer <fw@deneb.enyo.de> > > * c.opt (foperator-new-overflow-check): New. > > 2011-02-21 Florian Weimer <fw@deneb.enyo.de> > > PR c++/19351 > > * init.c (maybe_build_size_mult_saturated, build_new_size_expr): Add. > (build_new_1): Use these new functions in size calculations. > > * call.c (build_operator_new_call): Add size_with_cookie argument. > * cp-tree.h (build_operator_new_call): Likewise. Sorry, but: ping? <http://gcc.gnu.org/ml/gcc-patches/2011-02/msg01376.html>
Sorry it's taken so long to review this. On 02/21/2011 04:05 PM, Florian Weimer wrote: > build_operator_new_call (tree fnname, VEC(tree,gc) **args, > - tree *size, tree *cookie_size, > + tree *size, tree size_with_cookie, tree *cookie_size, We don't need both size_with_cookie and cookie_size here. Let's change the cookie_size parameter to be size_with_cookie instead (and adjust the places in build_new_1 that check for cookie_size being set or not). > + flag_new_overflow_check is true. > + */ Close comment on the last line of text rather than the next line. > + if (!flag_new_overflow_check) > + return result; Let's check for constant results here. If we have a TREE_CONSTANT result that overflows, we can handle it even if we aren't emitting the checks for non-constant values. And if we have a TREE_CONSTANT result that doesn't overflow, we don't have to bother building up the complicated trees in order to fold them away. > + return build3 (COND_EXPR, size_type_node, > + build2 (EQ_EXPR, size_type_node, mul2, size_zero_node), > + add == NULL_TREE ? size_zero_node : add, > + build3 (COND_EXPR, size_type_node, > + build2 (LE_EXPR, size_type_node, mul1, max_mul1), > + result, TYPE_MAX_VALUE (size_type_node))); Go ahead and fold here. The C++ front end has not yet been converted to delay folding like the C front end. > +static tree > +build_new_size_expr (tree elt_type, tree nelts, tree cookie_size) This function needs a comment. > + nelts = maybe_build_size_mult_saturated > + (convert (sizetype, nelts), > + convert (sizetype, array_type_nelts_top (elt_type)), > + NULL_TREE); It seems unfortunate to do this runtime checking more than once for multi-dimensional new. Since the element size and all but one of the array dimensions are going to be constant in most cases, let's try to multiply the element size with the constant dimensions before we bring in the potentially non-constant dimension. > I still need some guidance where to put the test case. There don't > seem to be any direct tests for operator new[] funcionality. Let's put it in g++.dg/init. Jason
* Jason Merrill: > Sorry it's taken so long to review this. Same here. *sigh* Thanks for your comments. > On 02/21/2011 04:05 PM, Florian Weimer wrote: >> build_operator_new_call (tree fnname, VEC(tree,gc) **args, >> - tree *size, tree *cookie_size, >> + tree *size, tree size_with_cookie, tree *cookie_size, > > We don't need both size_with_cookie and cookie_size here. Let's > change the cookie_size parameter to be size_with_cookie instead (and > adjust the places in build_new_1 that check for cookie_size being set > or not). Okay. >> + if (!flag_new_overflow_check) >> + return result; > > Let's check for constant results here. If we have a TREE_CONSTANT > result that overflows, we can handle it even if we aren't emitting the > checks for non-constant values. I assume I can report an error in this case? > And if we have a TREE_CONSTANT result that doesn't overflow, we > don't have to bother building up the complicated trees in order to > fold them away. Understood. This is sort-of needed ... >> + nelts = maybe_build_size_mult_saturated >> + (convert (sizetype, nelts), >> + convert (sizetype, array_type_nelts_top (elt_type)), >> + NULL_TREE); > > It seems unfortunate to do this runtime checking more than once for > multi-dimensional new. Since the element size and all but one of the > array dimensions are going to be constant in most cases, let's try to > multiply the element size with the constant dimensions before we bring > in the potentially non-constant dimension. ... to implement this anyway. I plan to accumulate constant and variable factors separately, and multiply them together in the end. VARIABLE * VARIABLE will still incur an expensive overflow check (involving integer division, even), but there's currently no way around that. I already have something in that direction, but it is still rather messy. Hopefully, it will be a temporary measure only, and this burden can go away when the middle-end learns about saturating arithmetic, or we implement std::bad_array_new_length from C++0X (and have removed VLA support). >> I still need some guidance where to put the test case. There don't >> seem to be any direct tests for operator new[] funcionality. > > Let's put it in g++.dg/init. Okay. Curiously, the test suite has rather poor coverage of operator new[] right now. Multi-dimensional new is also extremely rare in the C++ code bases I looked at.
On Sun, May 29, 2011 at 9:02 PM, Florian Weimer <fw@deneb.enyo.de> wrote: > * Jason Merrill: > >> Sorry it's taken so long to review this. > > Same here. *sigh* Thanks for your comments. > >> On 02/21/2011 04:05 PM, Florian Weimer wrote: >>> build_operator_new_call (tree fnname, VEC(tree,gc) **args, >>> - tree *size, tree *cookie_size, >>> + tree *size, tree size_with_cookie, tree *cookie_size, >> >> We don't need both size_with_cookie and cookie_size here. Let's >> change the cookie_size parameter to be size_with_cookie instead (and >> adjust the places in build_new_1 that check for cookie_size being set >> or not). > > Okay. > >>> + if (!flag_new_overflow_check) >>> + return result; >> >> Let's check for constant results here. If we have a TREE_CONSTANT >> result that overflows, we can handle it even if we aren't emitting the >> checks for non-constant values. > > I assume I can report an error in this case? Only a warning. The code is only undefined at runtime. But you could convert the allocation to __builtin_trap () (hmm, what about exceptions? Is such an allocation required to throw?) Richard. >> And if we have a TREE_CONSTANT result that doesn't overflow, we >> don't have to bother building up the complicated trees in order to >> fold them away. > > Understood. This is sort-of needed ... > >>> + nelts = maybe_build_size_mult_saturated >>> + (convert (sizetype, nelts), >>> + convert (sizetype, array_type_nelts_top (elt_type)), >>> + NULL_TREE); >> >> It seems unfortunate to do this runtime checking more than once for >> multi-dimensional new. Since the element size and all but one of the >> array dimensions are going to be constant in most cases, let's try to >> multiply the element size with the constant dimensions before we bring >> in the potentially non-constant dimension. > > ... to implement this anyway. I plan to accumulate constant and > variable factors separately, and multiply them together in the end. > VARIABLE * VARIABLE will still incur an expensive overflow check > (involving integer division, even), but there's currently no way > around that. > > I already have something in that direction, but it is still rather > messy. Hopefully, it will be a temporary measure only, and this > burden can go away when the middle-end learns about saturating > arithmetic, or we implement std::bad_array_new_length from C++0X (and > have removed VLA support). > >>> I still need some guidance where to put the test case. There don't >>> seem to be any direct tests for operator new[] funcionality. >> >> Let's put it in g++.dg/init. > > Okay. Curiously, the test suite has rather poor coverage of operator > new[] right now. Multi-dimensional new is also extremely rare in the > C++ code bases I looked at. >
* Richard Guenther: >>>> + if (!flag_new_overflow_check) >>>> + return result; >>> >>> Let's check for constant results here. If we have a TREE_CONSTANT >>> result that overflows, we can handle it even if we aren't emitting the >>> checks for non-constant values. >> >> I assume I can report an error in this case? > > Only a warning. The code is only undefined at runtime. But you > could convert the allocation to __builtin_trap () (hmm, what > about exceptions? Is such an allocation required to throw?) With a std::nothrow specifier, I don't think it is. But without that, it's required to throw std::bad_array_new_length in C++0X. C++03 is ambiguous.
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index bb928fa..78e4020 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -898,6 +898,10 @@ foperator-names C++ ObjC++ Recognize C++ keywords like \"compl\" and \"xor\" +foperator-new-overflow-check +C++ ObjC++ Var(flag_new_overflow_check) Init(1) +Check for overflow during operator new[] + foptional-diags C++ ObjC++ Ignore Does nothing. Preserved for backward compatibility. diff --git a/gcc/cp/call.c b/gcc/cp/call.c index 8dccbbe..15bc570 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -3635,7 +3635,7 @@ build_new_function_call (tree fn, VEC(tree,gc) **args, bool koenig_p, tree build_operator_new_call (tree fnname, VEC(tree,gc) **args, - tree *size, tree *cookie_size, + tree *size, tree size_with_cookie, tree *cookie_size, tree *fn) { tree fns; @@ -3706,7 +3706,7 @@ build_operator_new_call (tree fnname, VEC(tree,gc) **args, if (use_cookie) { /* Update the total size. */ - *size = size_binop (PLUS_EXPR, *size, *cookie_size); + *size = size_with_cookie; /* Update the argument list to reflect the adjusted size. */ VEC_replace (tree, *args, 0, *size); } diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 238d0cf..4598f10 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -4614,7 +4614,7 @@ extern tree build_user_type_conversion (tree, tree, int); extern tree build_new_function_call (tree, VEC(tree,gc) **, bool, tsubst_flags_t); extern tree build_operator_new_call (tree, VEC(tree,gc) **, tree *, - tree *, tree *); + tree, tree *, tree *); extern tree build_new_method_call (tree, tree, VEC(tree,gc) **, tree, int, tree *, tsubst_flags_t); diff --git a/gcc/cp/init.c b/gcc/cp/init.c index e590118..87a90a4 100644 --- a/gcc/cp/init.c +++ b/gcc/cp/init.c @@ -1913,6 +1913,43 @@ 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); } +/* Multiplies MUL1 with MUL2, and adds ADD. Returns + std::limits<size_t>::max() if the result cannot be be represented + as a size_t value. If ADD is null_tree, treat it as a zero + constant. Uses saturated arithmetic only if + flag_new_overflow_check is true. + */ +static tree +maybe_build_size_mult_saturated (tree mul1, tree mul2, tree add) +{ + tree max_mul1, result; + result = size_binop (MULT_EXPR, mul1, mul2); + if (add != NULL_TREE) + result = size_binop (PLUS_EXPR, result, add); + if (!flag_new_overflow_check) + return result; + max_mul1 = TYPE_MAX_VALUE (size_type_node); + if (add != NULL_TREE) + max_mul1 = build2 (MINUS_EXPR, size_type_node, max_mul1, add); + max_mul1 = build2 (TRUNC_DIV_EXPR, size_type_node, max_mul1, mul2); + return build3 (COND_EXPR, size_type_node, + build2 (EQ_EXPR, size_type_node, mul2, size_zero_node), + add == NULL_TREE ? size_zero_node : add, + build3 (COND_EXPR, size_type_node, + build2 (LE_EXPR, size_type_node, mul1, max_mul1), + result, TYPE_MAX_VALUE (size_type_node))); +} + +static tree +build_new_size_expr (tree elt_type, tree nelts, tree cookie_size) +{ + tree elt_size = size_in_bytes (elt_type); + if (nelts == NULL_TREE) + return elt_size; + return maybe_build_size_mult_saturated + (convert (sizetype, nelts), elt_size, cookie_size); +} + /* 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 @@ -1982,10 +2019,10 @@ build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts, for (elt_type = type; TREE_CODE (elt_type) == ARRAY_TYPE; elt_type = TREE_TYPE (elt_type)) - nelts = cp_build_binary_op (input_location, - MULT_EXPR, nelts, - array_type_nelts_top (elt_type), - complain); + nelts = maybe_build_size_mult_saturated + (convert (sizetype, nelts), + convert (sizetype, array_type_nelts_top (elt_type)), + NULL_TREE); if (TREE_CODE (elt_type) == VOID_TYPE) { @@ -2037,10 +2074,6 @@ build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts, return error_mark_node; } - size = size_in_bytes (elt_type); - if (array_p) - size = size_binop (MULT_EXPR, size, convert (sizetype, nelts)); - alloc_fn = NULL_TREE; /* If PLACEMENT is a single simple pointer type not passed by @@ -2104,8 +2137,8 @@ build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts, if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type)) { cookie_size = targetm.cxx.get_cookie_size (elt_type); - size = size_binop (PLUS_EXPR, size, cookie_size); } + size = build_new_size_expr (elt_type, nelts, cookie_size); /* Create the argument list. */ VEC_safe_insert (tree, gc, *placement, 0, size); /* Do name-lookup to find the appropriate operator. */ @@ -2136,14 +2169,24 @@ build_new_1 (VEC(tree,gc) **placement, tree type, tree nelts, { /* Use a global operator new. */ /* See if a cookie might be required. */ + tree size_with_cookie; + size = build_new_size_expr (elt_type, nelts, NULL_TREE); if (array_p && TYPE_VEC_NEW_USES_COOKIE (elt_type)) - cookie_size = targetm.cxx.get_cookie_size (elt_type); + { + cookie_size = targetm.cxx.get_cookie_size (elt_type); + size_with_cookie = build_new_size_expr + (elt_type, nelts, cookie_size); + } else - cookie_size = NULL_TREE; + { + cookie_size = NULL_TREE; + size_with_cookie = size; + } - alloc_call = build_operator_new_call (fnname, placement, - &size, &cookie_size, - &alloc_fn); + alloc_call = build_operator_new_call + (fnname, placement, + &size, size_with_cookie, &cookie_size, + &alloc_fn); } } diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 5295c39..c066693 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -1998,6 +1998,18 @@ Do not treat the operator name keywords @code{and}, @code{bitand}, @code{bitor}, @code{compl}, @code{not}, @code{or} and @code{xor} as synonyms as keywords. +@item -foperator-new-overflow-check +@itemx -fno-operator-new-overflow-check +@opindex foperator-new-overflow-check +@opindex fno-operator-new-overflow-check +If @option{-foperator-new-overflow-check} is specified, @code{operator +new[]} expressions will throw @code{std::bad_alloc} if the required size +cannot be represented as a value of type @code{size_t}. With +@option{-fno-operator-new-overflow-check}, no such checks are performed +and @code{operator new[]} may return a pointer to a memory area which +is too small for the requested number of elements. The default is to +perform the check, which increases code size slightly. + @item -fno-optional-diags @opindex fno-optional-diags Disable diagnostics that the standard says a compiler does not need to