Re: [PING] Re: [PATCH] c++/67913, 67917 - fix new expression with wrong number of elements

[James Greenhalgh <james dot greenhalgh at arm dot com>]

On Thu, Nov 05, 2015 at 12:30:08PM -0700, Martin Sebor wrote:
> On 11/02/2015 09:55 PM, Jason Merrill wrote:
> >On 10/26/2015 10:06 PM, Martin Sebor wrote:
> >>+      if (TREE_CONSTANT (maybe_constant_value (outer_nelts)))
> >>+    {
> >>+      if (tree_int_cst_lt (max_outer_nelts_tree, outer_nelts))
> >
> >maybe_constant_value may return a constant, but that doesn't mean that
> >outer_nelts was already constant; if it wasn't, the call to
> >tree_int_cst_lt will fail.
> 
> Thanks for the hint. I wasn't able to trigger the failure. I suspect
> outer_nelts must have already been folded at this point because the
> maybe_constant_value call isn't necessary. I removed it.
> 
> >Since we're moving toward delayed folding, I'd prefer to use the result
> >of maybe_constant_value only for this diagnostic, and then continue to
> >pass the unfolded value along.
> 
> Sure. Done in the attached patch.
> 
> Martin

> gcc/cp/ChangeLog
> 
> 2015-10-19  Martin Sebor  <msebor@redhat.com>
> 
> 	PR c++/67913
> 	PR c++/67927
> 	* call.c (build_operator_new_call): Do not assume size_check
> 	is non-null, analogously to the top half of the function.
> 	* init.c (build_new_1): Detect and diagnose array sizes in
> 	excess of the maximum of roughly SIZE_MAX / 2.
> 	Insert a runtime check only for arrays with a non-constant size.
> 	(build_new): Detect and diagnose negative array sizes.
> 
> gcc/testsuite/ChangeLog
> 
> 2015-10-19  Martin Sebor  <msebor@redhat.com>
> 
> 	* init/new45.C: New test to verify that operator new is invoked
> 	with or without overhead for a cookie.

This new test fails for ARM targets, snipping some of the diff...

> +inline __attribute__ ((always_inline))
> +void* operator new[] (size_t n)
> +{
> +    // Verify that array new is invoked with an argument large enough
> +    // for the array and a size_t cookie to store the number of elements.
> +    // (This holds for classes with user-defined types but not POD types).
> +    if (n != N * sizeof (UDClass) + sizeof n) abort ();
> +    return malloc (n);
> +}

Cookies on ARM are 8-bytes [1], but sizeof ((size_t) n) is only 4-bytes,
so this check will fail (We'll ask for 500 bytes, the test here will only
be looking for 496).

Would it undermine the test for other architectures if I were to swap out
the != for a >= ? I think that is in line with the "argument large enough
for the array" that this test is looking for, but would not catch bugs where
we were allocating more memory than neccessary.

Otherwise I can spin a patch which skips the test for ARM targets.

Thanks,
James