On Sun, May 29, 2011 at 9:02 PM, Florian Weimer <f...@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. >
