Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Martin Sebor]

On 04/15/2016 06:31 AM, Jakub Jelinek wrote:

On Thu, Apr 14, 2016 at 09:26:11AM -0600, Martin Sebor wrote:

/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C: In instantiation 
of 'struct TestType<32u>':
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:201:1:   
required from here
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:89:27: error: 
requested alignment 32 is larger than 16 [-Wattributes]


Thank you for the heads up (and sorry about the breakage).  I've
committed r234976 to fix that.


Probably because of this change you haven't reverted (removed) the vla11.C
testcase even when the ChangeLog said you've done that.
And, the testcase obviously fails on the trunk, so I've committed following
as obvious:


Probably.  I was rushing to unblock others whose bootstrap was
failing and didn't check carefully enough.  Thanks for fixing that.

Martin

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Jakub Jelinek]

On Thu, Apr 14, 2016 at 09:26:11AM -0600, Martin Sebor wrote:
> >/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C: In 
> >instantiation of 'struct TestType<32u>':
> >/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:201:1:   
> >required from here
> >/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:89:27: 
> >error: requested alignment 32 is larger than 16 [-Wattributes]
> 
> Thank you for the heads up (and sorry about the breakage).  I've
> committed r234976 to fix that.

Probably because of this change you haven't reverted (removed) the vla11.C
testcase even when the ChangeLog said you've done that.
And, the testcase obviously fails on the trunk, so I've committed following
as obvious:

2016-04-15  Jakub Jelinek  
 
PR c++/69517
PR c++/70019
PR c++/70588
* g++.dg/cpp1y/vla11.C: Revert for real.

--- g++.dg/cpp1y/vla11.C(revision 235020)
+++ g++.dg/cpp1y/vla11.C(revision 235021)
@@ -1,712 +0,0 @@
-// PR c++/69517 - [5/6 regression] SEGV on a VLA with excess initializer
-//   elements
-// PR c++/70019 - VLA size overflow not detected
-//
-// Runtime test to verify that attempting to either construct a VLA with
-// erroneous bounds, or initialize one with an initializer-list that
-// contains more elements than the VLA's non-constant (runtime) bounds
-// causes an exception to be thrown.  Test also verifies that valid
-// VLAs and their initializers don't cause such an exception.
-
-// { dg-do run { target c++11 } }
-// { dg-additional-options "-Wno-vla" }
-
-#pragma GCC diagnostic ignored "-Wvla"
-
-#define INT_MAX__INT_MAX__
-#define LONG_MAX   __LONG_MAX__
-#define SIZE_MAX   __SIZE_MAX__
-#define UINT_MAX   (~0U)
-#define ULONG_MAX  (~0LU)
-
-#define INT_MIN(-__INT_MAX__ - 1)
-#define LONG_MIN   (-__LONG_MAX__ - 1)
-
-// The size of the largest allowed VLA in bytes.  Bigger objects
-// cause an exception to be thrown.  Unless the maximum size is
-// obscenely large, smaller objects should be successfully created
-// provided there's enough stack space.  See TEST_NEAR_VLA_MAX_SIZE
-// below.
-#define MAX   (__SIZE_MAX__ / 2)
-
-// Define to non-zero to exercise very large VLAs with size just
-// below the implementation-defined maximum.
-#define TEST_NEAR_VLA_MAX_SIZE0
-
-// Define to zero to enable tests that cause an ICE due to c++/58646.
-#define BUG_58646 1
-
-// Helper macro to make it possible to pass as one multpile arguments
-// to another macro.
-#define Init(...) __VA_ARGS__
-
-typedef __SIZE_TYPE__ size_t;
-
-// Incremented for each test failure.
-int fail;
-
-// Used to convert a constant array dimension to a non-constant one.
-template 
-T d (T n)
-{
-  return n;
-}
-
-// Verify either that an expected exception has been thrown or that
-// one hasn't been thrown if one isn't expected.
-int __attribute__ ((noclone, noinline))
-sink (void *p, int line, bool expect, const char *expr)
-{
-  if (!p != expect)
-{
-  __builtin_printf ("line %i: Assertion failed: '%s': "
-"exception unexpectedly %sthrown\n",
-line, expr, !p ? "" : "not ");
-  ++fail;
-}
-  else
-{
-#if defined DEBUG && DEBUG
-__builtin_printf ("line %i: Assertion passed: '%s': "
- "exception %sthrown as expected\n",
- line, expr, !p ? "" : "not ");
-#endif
-}
-  return 0;
-}
-
-#define _CAT(name, line) name ## line
-#define CAT(name, line) _CAT (name, line)
-
-#define STR(...) #__VA_ARGS__
-
-// Type to exercise VLA with.  TYPESIZE is the size of the type in bytes.
-// Using a template serves two purposes.  First, it makes it possible to
-// parameterize the test on VLAs of different size.  Second, it verifies
-// that the checking code can deal with templates (i.e., completes
-// the element type of the VLA when necessary).
-template 
-union TestType
-{
-  char data;
-  char padding [TypeSize];
-};
-
-// Test function invoked with a pointer to each test case.  Must
-// return a value though what value doesn't matter.
-int __attribute__ ((noclone, noinline))
-tester (int (*testcase)(const char*),
-   const char *str, int line, bool expect)
-{
-  try
-{
-  return testcase (str);
-}
-  catch (...)
-{
-  return sink (0, line, expect, str);
-}
-}
-
-// Macro to define a unique specialization of a function template to
-// exercise a VLA of type T, rank N, with dimensions given by Dims
-// and initializer Init.  Expect is true when the VLA initialization
-// is expected to trigger an exception.
-// The macro creates a unique global dummy int object and initializes
-// it with the result of the function.  The dummy object servers no
-// other purpose but to call the function.  The function verifies
-// the expected postconditions.
-#define TEST(TypeSize, Dims, Init, Expect) \
-  static int CAT (testcase, __LINE__)(const char *str)   

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Christophe Lyon]

On 14 April 2016 at 17:26, Martin Sebor  wrote:
> On 04/14/2016 04:39 AM, Andreas Schwab wrote:
>>
>> Martin Sebor  writes:
>>
>>> diff --git a/gcc/testsuite/g++.dg/cpp1y/vla11.C
>>> b/gcc/testsuite/g++.dg/cpp1y/vla11.C
>>> new file mode 100644
>>> index 000..af9624a
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/cpp1y/vla11.C
>>> @@ -0,0 +1,711 @@
>>> +// PR c++/69517 - [5/6 regression] SEGV on a VLA with excess initializer
>>> +//   elements
>>> +// PR c++/70019 - VLA size overflow not detected
>>> +//
>>> +// Runtime test to verify that attempting to either construct a VLA with
>>> +// erroneous bounds, or initialize one with an initializer-list that
>>> +// contains more elements than the VLA's non-constant (runtime) bounds
>>> +// causes an exception to be thrown.  Test also verifies that valid
>>> +// VLAs and their initializers don't cause such an exception.
>>> +
>>> +// { dg-do run { target c++11 } }
>>> +// { dg-additional-options "-Wno-vla" }
>>
>>
>> On m68k:
>>
>> /daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C: In
>> instantiation of 'struct TestType<32u>':
>> /daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:201:1:
>> required from here
>> /daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:89:27:
>> error: requested alignment 32 is larger than 16 [-Wattributes]
>
>
> Thank you for the heads up (and sorry about the breakage).  I've
> committed r234976 to fix that.
>
Hi,

In your follow-up commit r234981, your gcc/testsuite/ChangeLog entry
says that you reverted vla11.C, but the commit does not actually
modify this testcase. As a matter of fact, I see it failing on arm and aarch64.

Did you forget to remove it, or did you expect r234976 to fix it?

Christophe.

> Martin

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Martin Sebor]

It caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70652


Sorry about that (I missed java among the languages when configuring
my build for regression testing).  I'm testing a libjava patch that
I expect will eliminate the linker error.

Martin

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Martin Sebor]

On 04/14/2016 04:39 AM, Andreas Schwab wrote:

Martin Sebor  writes:


diff --git a/gcc/testsuite/g++.dg/cpp1y/vla11.C 
b/gcc/testsuite/g++.dg/cpp1y/vla11.C
new file mode 100644
index 000..af9624a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/vla11.C
@@ -0,0 +1,711 @@
+// PR c++/69517 - [5/6 regression] SEGV on a VLA with excess initializer
+//   elements
+// PR c++/70019 - VLA size overflow not detected
+//
+// Runtime test to verify that attempting to either construct a VLA with
+// erroneous bounds, or initialize one with an initializer-list that
+// contains more elements than the VLA's non-constant (runtime) bounds
+// causes an exception to be thrown.  Test also verifies that valid
+// VLAs and their initializers don't cause such an exception.
+
+// { dg-do run { target c++11 } }
+// { dg-additional-options "-Wno-vla" }


On m68k:

/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C: In instantiation 
of 'struct TestType<32u>':
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:201:1:   
required from here
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:89:27: error: 
requested alignment 32 is larger than 16 [-Wattributes]


Thank you for the heads up (and sorry about the breakage).  I've
committed r234976 to fix that.

Martin

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Andreas Schwab]

Martin Sebor  writes:

> diff --git a/gcc/testsuite/g++.dg/cpp1y/vla11.C 
> b/gcc/testsuite/g++.dg/cpp1y/vla11.C
> new file mode 100644
> index 000..af9624a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/vla11.C
> @@ -0,0 +1,711 @@
> +// PR c++/69517 - [5/6 regression] SEGV on a VLA with excess initializer
> +//   elements
> +// PR c++/70019 - VLA size overflow not detected
> +//
> +// Runtime test to verify that attempting to either construct a VLA with
> +// erroneous bounds, or initialize one with an initializer-list that
> +// contains more elements than the VLA's non-constant (runtime) bounds
> +// causes an exception to be thrown.  Test also verifies that valid
> +// VLAs and their initializers don't cause such an exception.
> +
> +// { dg-do run { target c++11 } }
> +// { dg-additional-options "-Wno-vla" }

On m68k:

/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C: In 
instantiation of 'struct TestType<32u>':
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:201:1:   
required from here
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:89:27: error: 
requested alignment 32 is larger than 16 [-Wattributes]
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C: In function 
'int testcase201(const char*)':
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:122:5: error: 
static assertion failed: wrong test type size
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:201:1: note: 
in expansion of macro 'TEST'
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C: In 
instantiation of 'struct TestType<64u>':
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:202:1:   
required from here
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:89:27: error: 
requested alignment 64 is larger than 16 [-Wattributes]
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C: In function 
'int testcase202(const char*)':
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:122:5: error: 
static assertion failed: wrong test type size
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:202:1: note: 
in expansion of macro 'TEST'
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C: In function 
'int testcase704(const char*)':
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:122:5: error: 
static assertion failed: wrong test type size
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:704:1: note: 
in expansion of macro 'TEST'
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C: In function 
'int testcase705(const char*)':
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:122:5: error: 
static assertion failed: wrong test type size
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:705:1: note: 
in expansion of macro 'TEST'

FAIL: g++.dg/cpp1y/vla11.C  -std=c++11 (test for excess errors)
Excess errors:
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:89:27: error: 
requested alignment 32 is larger than 16 [-Wattributes]
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:122:5: error: 
static assertion failed: wrong test type size
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:89:27: error: 
requested alignment 64 is larger than 16 [-Wattributes]
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:122:5: error: 
static assertion failed: wrong test type size
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:122:5: error: 
static assertion failed: wrong test type size
/daten/aranym/gcc/gcc-20160414/gcc/testsuite/g++.dg/cpp1y/vla11.C:122:5: error: 
static assertion failed: wrong test type size

Andreas.

-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[H.J. Lu]

On Wed, Apr 13, 2016 at 11:36 AM, Martin Sebor  wrote:
> On 04/12/2016 12:17 PM, Jason Merrill wrote:
>>
>> On 04/10/2016 07:14 PM, Martin Sebor wrote:
>>>
>>> The call to build_vla_check() in check_initializer() is to check
>>> only explicitly initialized VLAs.  The latter function may need
>>> to complete the VLA element type and reshape the initializer so
>>> the call cannot be made earlier.  The function returns the runtime
>>> initializer code so the call cannot be made after it returns.
>>
>>
>> But this call isn't checking the initializer; we're looking at the
>> initializer in build_vec_init now.
>
>
> Let me try to explain.  Strings aside, there are three cases each
> of which the patch handles in a different function:
>
> 1) The No Initializer case is handled by calling build_vla_check
> directly from cp_finish_decl.
>
> 2) The Empty initializer case is handled from check_initializer
> in the hunk you had pasted upthread.  (It isn't handled in
> build_vec_init because length_check is false for empty
> initializers, and because it's too late to check VLA bounds
> there anyway -- see below.)
>
> 3) In the Non-empty Initializer case, excess initializer elements
> are checked for in build_vec_init.  VLA bounds are checked from
> check_initializer same as in (2).
>
> As I mentioned last Friday, emitting the check for the VLA bounds
> in build_vec_init appears to be too late because by then the code
> to allocate the stack has already been emitted.  Maybe there's
> a way to do it, I don't know.  Controlling what piece of code is
> emitted when is something I don't know much about yet.
>
>>> The call to build_vla_check() in cp_finish_decl() is guarded with
>>> !init and made to check VLAs that are not explicitly initialized.
>>> This call could perhaps be moved into check_initializer() though
>>> it doesn't seem like it logically belongs there (since there is
>>> no initializer).
>>
>>
>> check_initializer handles implicit (default-) as well as explicit
>> initialization.
>>
>>> If it were moved there, it seems to me that it
>>> would require more extensive changes to the function than making
>>> it in cp_finish_decl().
>>
>>
>> I don't see that; you ought to be able to move the check_initializer
>> copy down out of the if/else structure and use that for both cases.
>
>
> You're right, it was simpler than I thought.  I was being overly
> conservative in an effort to avoid changing more code than is
> absolutely necessary.
>
> Attached is an updated patch with this simplification. It avoids
> case (1) above.  It also adds an argument to build_vla_check to
> avoid building the size check when it's called from build_vec_init.
>
> I also modified the alternate patch accordingly.  It's attached
> for comparison. I still find it preferable to the first patch.
> It's simpler because it doesn't require the special handling for
> strings and avoids parameterizing build_vla_check so as not to
> build the duplicate check in build_vec_init.
>
> I don't see the benefit in doing the checking in build_vec_init
> and split_constant_init_1 when it can all be done just in
> check_initializer.  I'm sure you have your reasons for going that
> route so I'd appreciate if you could let me know why you think
> that's better.

It caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70652


-- 
H.J.

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Jason Merrill]

On 04/13/2016 02:36 PM, Martin Sebor wrote:

I don't see the benefit in doing the checking in build_vec_init
and split_constant_init_1 when it can all be done just in
check_initializer.  I'm sure you have your reasons for going that
route so I'd appreciate if you could let me know why you think
that's better.


In order to share the code for checking VLAs and array new, but I take 
your point that they aren't similar enough for that.  Your alternate 
patch is OK.


Jason

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Martin Sebor]

On 04/12/2016 12:17 PM, Jason Merrill wrote:

On 04/10/2016 07:14 PM, Martin Sebor wrote:

The call to build_vla_check() in check_initializer() is to check
only explicitly initialized VLAs.  The latter function may need
to complete the VLA element type and reshape the initializer so
the call cannot be made earlier.  The function returns the runtime
initializer code so the call cannot be made after it returns.


But this call isn't checking the initializer; we're looking at the
initializer in build_vec_init now.


Let me try to explain.  Strings aside, there are three cases each
of which the patch handles in a different function:

1) The No Initializer case is handled by calling build_vla_check
directly from cp_finish_decl.

2) The Empty initializer case is handled from check_initializer
in the hunk you had pasted upthread.  (It isn't handled in
build_vec_init because length_check is false for empty
initializers, and because it's too late to check VLA bounds
there anyway -- see below.)

3) In the Non-empty Initializer case, excess initializer elements
are checked for in build_vec_init.  VLA bounds are checked from
check_initializer same as in (2).

As I mentioned last Friday, emitting the check for the VLA bounds
in build_vec_init appears to be too late because by then the code
to allocate the stack has already been emitted.  Maybe there's
a way to do it, I don't know.  Controlling what piece of code is
emitted when is something I don't know much about yet.


The call to build_vla_check() in cp_finish_decl() is guarded with
!init and made to check VLAs that are not explicitly initialized.
This call could perhaps be moved into check_initializer() though
it doesn't seem like it logically belongs there (since there is
no initializer).


check_initializer handles implicit (default-) as well as explicit
initialization.


If it were moved there, it seems to me that it
would require more extensive changes to the function than making
it in cp_finish_decl().


I don't see that; you ought to be able to move the check_initializer
copy down out of the if/else structure and use that for both cases.


You're right, it was simpler than I thought.  I was being overly
conservative in an effort to avoid changing more code than is
absolutely necessary.

Attached is an updated patch with this simplification. It avoids
case (1) above.  It also adds an argument to build_vla_check to
avoid building the size check when it's called from build_vec_init.

I also modified the alternate patch accordingly.  It's attached
for comparison. I still find it preferable to the first patch.
It's simpler because it doesn't require the special handling for
strings and avoids parameterizing build_vla_check so as not to
build the duplicate check in build_vec_init.

I don't see the benefit in doing the checking in build_vec_init
and split_constant_init_1 when it can all be done just in
check_initializer.  I'm sure you have your reasons for going that
route so I'd appreciate if you could let me know why you think
that's better.

Thanks
Martin
PR c++/69517 - [5/6 regression] SEGV on a VLA with excess initializer elements
PR c++/70019 - VLA size overflow not detected

gcc/testsuite/ChangeLog:
2016-04-10  Martin Sebor  

	PR c++/69517
	PR c++/70019
	* c-c++-common/ubsan/vla-1.c (main): Catch exceptions.
	* g++.dg/cpp1y/vla11.C: New test.
	* g++.dg/cpp1y/vla12.C: New test.
	* g++.dg/cpp1y/vla13.C: New test.
	* g++.dg/cpp1y/vla14.C: New test.
	* g++.dg/cpp1y/vla3.C: Restore deleted test.
	* gcc/testsuite/g++.dg/init/array24.C: Fully brace VLA initializer.
	* g++.dg/ubsan/vla-1.C: Disable exceptions.

gcc/cp/ChangeLog:
2016-04-10  Martin Sebor  

	PR c++/69517
	PR c++/70019
	* cp-tree.h (throw_bad_array_length, build_vla_check): Declare new
	functions.
	* decl.c (check_initializer): Call them.
	(reshape_init_r): Reject incompletely braced intializer-lists
	for VLAs.
	* init.c (throw_bad_array_length, build_vla_check)
	(build_vla_size_check, build_vla_init_check): Define new functions.
	(build_vec_init): Use variably_modified_type_p() to detect a VLA.
	Call throw_bad_array_length() and build_vla_check().
	* typeck2.c (split_nonconstant_init_1): Use variably_modified_type_p()
	to detect a VLA.
	(store_init_value): Same.
	(split_nonconstant_init): Handle excess elements in string literal
	initializers for VLAs.

gcc/doc/ChangeLog:

2016-04-10  Martin Sebor  

	PR c++/69517
	PR c++/70019
	* extend.texi (Variable Length): Document C++ specifics.

libstdc++-v3/ChangeLog:

2016-04-10  Martin Sebor  

	PR c++/69517
	* testsuite/25_algorithms/rotate/moveable2.cc: Make sure VLA
	upper bound is positive.

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index b7b770f..2936ac9 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5978,6 +5978,7 @@ extern tree build_value_init_noctor		(tree, tsubst_flags_t);
 extern tree get_nsdmi(tree, bool);
 extern tree 

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Jason Merrill]

On 04/10/2016 07:14 PM, Martin Sebor wrote:

The call to build_vla_check() in check_initializer() is to check
only explicitly initialized VLAs.  The latter function may need
to complete the VLA element type and reshape the initializer so
the call cannot be made earlier.  The function returns the runtime
initializer code so the call cannot be made after it returns.


But this call isn't checking the initializer; we're looking at the 
initializer in build_vec_init now.



The call to build_vla_check() in cp_finish_decl() is guarded with
!init and made to check VLAs that are not explicitly initialized.
This call could perhaps be moved into check_initializer() though
it doesn't seem like it logically belongs there (since there is
no initializer).


check_initializer handles implicit (default-) as well as explicit 
initialization.



If it were moved there, it seems to me that it
would require more extensive changes to the function than making
it in cp_finish_decl().


I don't see that; you ought to be able to move the check_initializer 
copy down out of the if/else structure and use that for both cases.



+/* Build an expression to determine whether the VLA TYPE is erroneous.
+   INIT is the VLA initializer expression or NULL_TREE when the VLA is
+   not initialized.  */
+
+/* Build an expression to determine whether the VLA TYPE is erroneous.
+   INIT is the VLA initializer expression or NULL_TREE when the VLA is
+   not initialized.  */


Duplicate comment.

Jason

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Martin Sebor]

On 04/12/2016 09:43 AM, Jason Merrill wrote:

On 04/10/2016 07:14 PM, Martin Sebor wrote:

+  if (TREE_CODE (type) == ARRAY_TYPE
+  && variably_modified_type_p (type, NULL_TREE)
+  && !processing_template_decl)
+{
+  /* Statically check for overflow in VLA bounds and build
+ an expression that checks at runtime whether the VLA
+ is erroneous due to invalid (runtime) bounds.
+ Another expression to check for excess initializers
+ is built in build_vec_init.  */


Why do this both in check_initializer and then again in cp_finish_decl
right after the call to check_initializer?


I would have liked to make just one call but I couldn't find
a good way to do it.

The call to build_vla_check() in check_initializer() is to check
only explicitly initialized VLAs.  The latter function may need
to complete the VLA element type and reshape the initializer so
the call cannot be made earlier.  The function returns the runtime
initializer code so the call cannot be made after it returns.

The call to build_vla_check() in cp_finish_decl() is guarded with
!init and made to check VLAs that are not explicitly initialized.
This call could perhaps be moved into check_initializer() though
it doesn't seem like it logically belongs there (since there is
no initializer).  If it were moved there, it seems to me that it
would require more extensive changes to the function than making
it in cp_finish_decl().


+  /* Also check to see if the final array size is zero (the size
+ is unsigned so the earlier overflow check detects negative
+ values as well.  */
+  tree zerocheck = fold_build2 (EQ_EXPR, boolean_type_node,
+vlasize, size_zero_node);


I'm not sure whether we want this, given that GCC allows zero-length
arrays in general.  As I recall, with the C++14 stuff whether we checked
for zero was dependent on flag_iso, though I wasn't particularly happy
with that.  If you want to check this unconditionally that's fine.


The (sizeof A / sizeof *A) expression is commonly used to compute
the number of elements in an array.  When A is a two-dimensional
VLA with a runtime minor bound of zero, the division is undefined
and usually traps at runtime.  (This is also true for ordinary
arrays but there GCC warns about it with -Wdiv-by-zero.)  Since
we're already doing the runtime checking for VLAs it costs almost
nothing to detect and prevent it and results in arguably safer
code.  So if it's okay with you, my preference is to keep it
without providing a way to disable it.

Martin

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Jason Merrill]

On 04/10/2016 07:14 PM, Martin Sebor wrote:

+ if (TREE_CODE (type) == ARRAY_TYPE
+ && variably_modified_type_p (type, NULL_TREE)
+ && !processing_template_decl)
+   {
+ /* Statically check for overflow in VLA bounds and build
+an expression that checks at runtime whether the VLA
+is erroneous due to invalid (runtime) bounds.
+Another expression to check for excess initializers
+is built in build_vec_init.  */


Why do this both in check_initializer and then again in cp_finish_decl 
right after the call to check_initializer?



+  /* Also check to see if the final array size is zero (the size
+is unsigned so the earlier overflow check detects negative
+values as well.  */
+  tree zerocheck = fold_build2 (EQ_EXPR, boolean_type_node,
+   vlasize, size_zero_node);


I'm not sure whether we want this, given that GCC allows zero-length 
arrays in general.  As I recall, with the C++14 stuff whether we checked 
for zero was dependent on flag_iso, though I wasn't particularly happy 
with that.  If you want to check this unconditionally that's fine.


Jason

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Martin Sebor]

Attached is an updated patch with some the changes you suggested.
I also moved the new functions from decl.c to init.c because they
logically seem to belong there.

The suggested changes resulted in introducing the checking code
into split_nonconstant_init() in cp/typeck2.c in addition to
build_vec_init().  I would expect it to be preferable to check
in as few places as possible so I attach an alternate version
of the patch without this proliferation of call (the tests and
the documentation changes are the same so I excluded those).

Let me know which of the two approaches you prefer and/or if
you have any other requests or suggestions.

Martin

On 04/07/2016 06:05 PM, Martin Sebor wrote:

I've spent a ton of time trying to implement the suggested
changes (far too much in large part because of my own mistakes)
but I don't think they will work.  I'll try to clean up what
I have and post it for review.  I wanted to respond to this
how in case you have some suggestions or concerns with the
direction I'm taking in the meantime.


But if even a few MB seems too strict, I would find having even
an exceedingly liberal limit (say 1GB) much preferable to none
at all as it makes it possible to exercise boundary conditions
such as the size overflow problem you noted below.


That sounds reasonable, as long as users with unusual needs can adjust
it with a flag, but even so I'm nervous about doing this in stage 4.  It
certainly isn't a regression.


I'm not comfortable adding a new option at this stage.  I'm also
not sure that an option to impose a static limit is the best
solution.  It seems that if we go to the trouble of making the limit
customizable it should be possible to change it without recompiling
everything (e.g., on ELF, we could check for a weak function and
call it to get the most up-to-date limit).

Let me restore the 4.9.3 behavior by setting the VLA size limit to
SIZE_MAX / 2 (that fixes the other regression that I just raised
in c++/70588 for the record).


I don't think modifying build_vec_init() alone would be sufficient.
For example, the function isn't called for a VLA with a constant
bound like this one:

  int A [2][N] = { 1, 2, 3, 4 };


That seems like a bug, due to array_of_runtime_bound_p returning false
for that array.


It seems that a complete fix would involve (among other things)
replacing calls to array_of_runtime_bound_p with
variably_modified_type_p or similar since the N3639 arrays are
just a subset of those accepted by G++.  Unfortunately, that has
other repercussions (e.g., c++70555).

I replaced the call to array_of_runtime_bound_p in build_vec_init
with one to variably_modified_type_p to get around the above.
That  works, but it's only good for checking for excess
initializers in build_vec_init.  It's too late to check for
overflow in the VLA bounds because by that time the code to
allocate the stack has already been emitted.


Also, I think we should check for invalid bounds in
compute_array_index_type, next to the UBsan code.  Checking bounds only
from cp_finish_decl means that we don't check uses of VLA types other
than variable declarations.


I don't see how to make this work.  compute_array_index_type
doesn't have access to the CONSTRUCTOR for the initializer of
the VLA the initializer hasn't been parsed yet).  Without it
it's not possible to detect VLA size overflow in cases such
as in:

 T a [][N] = { { ... }, { ... } };

where the number of top-level elements determines whether or
not the size of the whole VLA would overflow or exceed the
maximum.

Given this, I believe the check does need to be implemented
somewhere in cp_finish_decl or one of the functions it calls
(such as check_initializer) and emitted before build_vec_init
is called or the initializer code it creates is emitted.



You mean VLA typedefs?  That's true, though I have consciously
avoided dealing with those.  They're outlawed in N3639 and so
I've been focusing just on variables.  But since GCC accepts
VLA typedefs too I was thinking I would bring them up at some
point in the future to decide what to do about them.


And cast to pointer to VLAs.  But for non-variable cases we don't care
about available stack, so we wouldn't want your allocation limit to
apply.


I don't want to implement it now, but I think the same limit
should apply in all cases, otherwise code remains susceptible
to unsigned integer wrapping.  For example:

   extern size_t N;
   typedef int A [N];
   int *a = (int*)malloc (sizeof (A));   // possible wraparound
   a [N - 1] = 0;// out-of-bounds write

It seems that the typedef will need to be accepted (in case it's
unused) but the runtime sizeof would need to do the checking and
potentially throw.  I haven't thought through the ramifications
yet.




As for where to add the bounds checking code, I also at first
thought of checking the bounds parallel to the UBSan code in
compute_array_index_type() and even tried that approach. The
problem with it is that 

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Martin Sebor]

I've spent a ton of time trying to implement the suggested
changes (far too much in large part because of my own mistakes)
but I don't think they will work.  I'll try to clean up what
I have and post it for review.  I wanted to respond to this
how in case you have some suggestions or concerns with the
direction I'm taking in the meantime.


But if even a few MB seems too strict, I would find having even
an exceedingly liberal limit (say 1GB) much preferable to none
at all as it makes it possible to exercise boundary conditions
such as the size overflow problem you noted below.


That sounds reasonable, as long as users with unusual needs can adjust
it with a flag, but even so I'm nervous about doing this in stage 4.  It
certainly isn't a regression.


I'm not comfortable adding a new option at this stage.  I'm also
not sure that an option to impose a static limit is the best
solution.  It seems that if we go to the trouble of making the limit
customizable it should be possible to change it without recompiling
everything (e.g., on ELF, we could check for a weak function and
call it to get the most up-to-date limit).

Let me restore the 4.9.3 behavior by setting the VLA size limit to
SIZE_MAX / 2 (that fixes the other regression that I just raised
in c++/70588 for the record).


I don't think modifying build_vec_init() alone would be sufficient.
For example, the function isn't called for a VLA with a constant
bound like this one:

  int A [2][N] = { 1, 2, 3, 4 };


That seems like a bug, due to array_of_runtime_bound_p returning false
for that array.


It seems that a complete fix would involve (among other things)
replacing calls to array_of_runtime_bound_p with
variably_modified_type_p or similar since the N3639 arrays are
just a subset of those accepted by G++.  Unfortunately, that has
other repercussions (e.g., c++70555).

I replaced the call to array_of_runtime_bound_p in build_vec_init
with one to variably_modified_type_p to get around the above.
That  works, but it's only good for checking for excess
initializers in build_vec_init.  It's too late to check for
overflow in the VLA bounds because by that time the code to
allocate the stack has already been emitted.


Also, I think we should check for invalid bounds in
compute_array_index_type, next to the UBsan code.  Checking bounds only
from cp_finish_decl means that we don't check uses of VLA types other
than variable declarations.


I don't see how to make this work.  compute_array_index_type
doesn't have access to the CONSTRUCTOR for the initializer of
the VLA the initializer hasn't been parsed yet).  Without it
it's not possible to detect VLA size overflow in cases such
as in:

T a [][N] = { { ... }, { ... } };

where the number of top-level elements determines whether or
not the size of the whole VLA would overflow or exceed the
maximum.

Given this, I believe the check does need to be implemented
somewhere in cp_finish_decl or one of the functions it calls
(such as check_initializer) and emitted before build_vec_init
is called or the initializer code it creates is emitted.



You mean VLA typedefs?  That's true, though I have consciously
avoided dealing with those.  They're outlawed in N3639 and so
I've been focusing just on variables.  But since GCC accepts
VLA typedefs too I was thinking I would bring them up at some
point in the future to decide what to do about them.


And cast to pointer to VLAs.  But for non-variable cases we don't care
about available stack, so we wouldn't want your allocation limit to apply.


I don't want to implement it now, but I think the same limit
should apply in all cases, otherwise code remains susceptible
to unsigned integer wrapping.  For example:

  extern size_t N;
  typedef int A [N];
  int *a = (int*)malloc (sizeof (A));   // possible wraparound
  a [N - 1] = 0;// out-of-bounds write

It seems that the typedef will need to be accepted (in case it's
unused) but the runtime sizeof would need to do the checking and
potentially throw.  I haven't thought through the ramifications
yet.




As for where to add the bounds checking code, I also at first
thought of checking the bounds parallel to the UBSan code in
compute_array_index_type() and even tried that approach. The
problem with it is that it only considers one array dimension
at a time, without the knowledge of the others.  As a result,
as noted in sanitizer/70051, it doesn't correctly detect
overflows in the bounds of multidimensional VLAs.


It doesn't, but I don't see why it couldn't.  It should be fine to check
each dimension for overflow separately; if an inner dimension doesn't
overflow, we can go on and consider the outer dimension.


As I explained above, I don't see how to make this work.



Incidentally, I was wondering if it would make sense to use the
overflowing calculation for both TYPE_SIZE and the sanity check when
we're doing both.


I'm not sure what you mean here.  Can you elaborate?




+  /* Avoid 

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Jason Merrill]

On 04/01/2016 07:02 PM, Martin Sebor wrote:

Fair enough.  I don't think we can impose an arbitrary 64K limit,
however, as that is a lot smaller than the 8MB default stack size, and
programs can use setrlimit to increase the stack farther.  For GCC 6 let
not impose any limit beyond non-negative/overflowing, and as you say we
can do something better in GCC 7.


Would you be open to imposing a somewhat more permissive limit,
say on the order of one or a few megabytes (but less than the
default 8 MB on Linux)?

I ask because I expect the majority of programmer errors with
VLAs to be due to out-of-range bounds that couldn't be
accommodated even if stack space was extended well beyond
the Linux default (say hundreds of MB), or that would result
in complete stack space exhaustion.  I.e., not caused by
deliberately trying to create very large VLAs but rather by
accidentally creating VLAs with unconstrained bounds (due to
a failure to validate input, uninitialized unsigned variables,
etc.)

I expect fewer cases to be due to negative or zero bounds or
excessive initializers.

I also expect programmers to want to find out about such bugs
sooner (i.e., in unit testing with plentiful stack space) rather
than after software has been deployed (and under low stack space
conditions not exercised during unit testing).

To that end, I think a lower limit is going to be more helpful
than a permissive one (or none at all).



But if even a few MB seems too strict, I would find having even
an exceedingly liberal limit (say 1GB) much preferable to none
at all as it makes it possible to exercise boundary conditions
such as the size overflow problem you noted below.


That sounds reasonable, as long as users with unusual needs can adjust 
it with a flag, but even so I'm nervous about doing this in stage 4.  It 
certainly isn't a regression.



During additional testing it dawned on me that there is no good
way to validate (or even to initialize) the initializer list of
a multi-dimensional VLA that isn't unambiguously braced.

For example, the following VLA with N = 2 and N = 3:

 int A [M][N] = { 1, 2, 3, 4 };

Unpatched, GCC initializes it to { { 1, 2, 3 }, { 0, 0, 0 } }.
With my first patch, GCC throws.  Neither is correct, but doing
the right thing would involve emitting parameterizing the
initialization code for the value of each bound.  While that
might be doable it feels like a bigger change than I would be
comfortable attempting at this stage.  To avoid the problem I've
made it an error to specify a partially braced VLA initializer.


Sounds good.


If you think it's worthwhile, I can see about implementing the
runtime reshaping in stage 1.


No, thanks.  I think requiring explicit bracing is fine.


It seems to me that we want use the existing check for excess
initializers in build_vec_init, in the if (length_check) section, though
as you mention in 70019 that needs to be improved to handle STRING_CST.


I don't think modifying build_vec_init() alone would be sufficient.
For example, the function isn't called for a VLA with a constant
bound like this one:

  int A [2][N] = { 1, 2, 3, 4 };


That seems like a bug, due to array_of_runtime_bound_p returning false 
for that array.



Also, I think we should check for invalid bounds in
compute_array_index_type, next to the UBsan code.  Checking bounds only
from cp_finish_decl means that we don't check uses of VLA types other
than variable declarations.


You mean VLA typedefs?  That's true, though I have consciously
avoided dealing with those.  They're outlawed in N3639 and so
I've been focusing just on variables.  But since GCC accepts
VLA typedefs too I was thinking I would bring them up at some
point in the future to decide what to do about them.


And cast to pointer to VLAs.  But for non-variable cases we don't care 
about available stack, so we wouldn't want your allocation limit to apply.



As for where to add the bounds checking code, I also at first
thought of checking the bounds parallel to the UBSan code in
compute_array_index_type() and even tried that approach. The
problem with it is that it only considers one array dimension
at a time, without the knowledge of the others.  As a result,
as noted in sanitizer/70051, it doesn't correctly detect
overflows in the bounds of multidimensional VLAs.


It doesn't, but I don't see why it couldn't.  It should be fine to check 
each dimension for overflow separately; if an inner dimension doesn't 
overflow, we can go on and consider the outer dimension.


Incidentally, I was wondering if it would make sense to use the 
overflowing calculation for both TYPE_SIZE and the sanity check when 
we're doing both.



+  /* Avoid instrumenting constexpr functions.  Those must
+ be checked statically, and the (non-constexpr) dynamic
+ instrumentation would cause them to be rejected.  */



Hmm, this sounds wrong; constexpr functions can also be called with
non-constant arguments, and the instrumentation 

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Martin Sebor]

Fair enough.  I don't think we can impose an arbitrary 64K limit,
however, as that is a lot smaller than the 8MB default stack size, and
programs can use setrlimit to increase the stack farther.  For GCC 6 let
not impose any limit beyond non-negative/overflowing, and as you say we
can do something better in GCC 7.


Would you be open to imposing a somewhat more permissive limit,
say on the order of one or a few megabytes (but less than the
default 8 MB on Linux)?

I ask because I expect the majority of programmer errors with
VLAs to be due to out-of-range bounds that couldn't be
accommodated even if stack space was extended well beyond
the Linux default (say hundreds of MB), or that would result
in complete stack space exhaustion.  I.e., not caused by
deliberately trying to create very large VLAs but rather by
accidentally creating VLAs with unconstrained bounds (due to
a failure to validate input, uninitialized unsigned variables,
etc.)

I expect fewer cases to be due to negative or zero bounds or
excessive initializers.

I also expect programmers to want to find out about such bugs
sooner (i.e., in unit testing with plentiful stack space) rather
than after software has been deployed (and under low stack space
conditions not exercised during unit testing).

To that end, I think a lower limit is going to be more helpful
than a permissive one (or none at all).

But if even a few MB seems too strict, I would find having even
an exceedingly liberal limit (say 1GB) much preferable to none
at all as it makes it possible to exercise boundary conditions
such as the size overflow problem you noted below.



I think all modes.


Sounds good.  I've enabled it in all modes.


The if (1) seems left over from development.


Right.



It looks like this will multiply *cst_size by the sub-array length once
for each element of the outer array, leading to a too-large result.  And
also generate redundant code to check the bounds of the sub-array
multiple times.


Great catch, thank you!  I think it was a mistake on my part to
try to build both kinds of checks in the same function.  In the
update patch I split it up into two: one to build the bounds
check and another to build the initializer check.

During additional testing it dawned on me that there is no good
way to validate (or even to initialize) the initializer list of
a multi-dimensional VLA that isn't unambiguously braced.

For example, the following VLA with N = 2 and N = 3:

int A [M][N] = { 1, 2, 3, 4 };

Unpatched, GCC initializes it to { { 1, 2, 3 }, { 0, 0, 0 } }.
With my first patch, GCC throws.  Neither is correct, but doing
the right thing would involve emitting parameterizing the
initialization code for the value of each bound.  While that
might be doable it feels like a bigger change than I would be
comfortable attempting at this stage.  To avoid the problem I've
made it an error to specify a partially braced VLA initializer.
If you think it's worthwhile, I can see about implementing the
runtime reshaping in stage 1.



It seems to me that we want use the existing check for excess
initializers in build_vec_init, in the if (length_check) section, though
as you mention in 70019 that needs to be improved to handle STRING_CST.


I don't think modifying build_vec_init() alone would be sufficient.
For example, the function isn't called for a VLA with a constant
bound like this one:

 int A [2][N] = { 1, 2, 3, 4 };


Also, I think we should check for invalid bounds in
compute_array_index_type, next to the UBsan code.  Checking bounds only
from cp_finish_decl means that we don't check uses of VLA types other
than variable declarations.


You mean VLA typedefs?  That's true, though I have consciously
avoided dealing with those.  They're outlawed in N3639 and so
I've been focusing just on variables.  But since GCC accepts
VLA typedefs too I was thinking I would bring them up at some
point in the future to decide what to do about them.

As for where to add the bounds checking code, I also at first
thought of checking the bounds parallel to the UBSan code in
compute_array_index_type() and even tried that approach. The
problem with it is that it only considers one array dimension
at a time, without the knowledge of the others.  As a result,
as noted in sanitizer/70051, it doesn't correctly detect
overflows in the bounds of multidimensional VLAs.




+  /* Avoid instrumenting constexpr functions.  Those must
+ be checked statically, and the (non-constexpr) dynamic
+ instrumentation would cause them to be rejected.  */


Hmm, this sounds wrong; constexpr functions can also be called with
non-constant arguments, and the instrumentation should be folded away
when evaluating a call with constant arguments.


You're right that constexpr functions should be checked as
well.  Unfortunately, at present, due to c++/70507 the check
(or rather the call to __builtin_mul_overflow) isn't folded
away and we end up with error: call to internal function.
As 

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Jason Merrill]

On 03/23/2016 03:47 PM, Martin Sebor wrote:

Thanks for the comments.


2) It hardwires a rather arbitrarily restrictive limit of 64 KB
on the size of the biggest C++ VLA.  (This could stand to be
improved and made more intelligent, and perhaps integrated
with stack checking via -fstack-limit, after the GCC 6
release.)


The bounds checking should share code with build_new_1.


I agree that sharing the same code is right long term approach.

I had initially started out down that path, by factoring out code
from build_new_1 into a general function that I had thought could
be shared between it and cp_finish_decl.  But I ended up abandoning
that approach two reasons:

a) The checking expression built for array new is quite a bit less
involved because only the major dimension of the array in requires
runtime checking (the others must be constant and are checked at
compile time).  In contrast, all VLA dimensions are potentially
dynamic and so must be checked at runtime.

b) While (a) can be solved by making the checking function smart
and general enough, it felt too intrusive and potentially risky
to change array new at this stage.

That said, I'm happy to do this refactoring in stage 1.


Fair enough.  I don't think we can impose an arbitrary 64K limit, 
however, as that is a lot smaller than the 8MB default stack size, and 
programs can use setrlimit to increase the stack farther.  For GCC 6 let 
not impose any limit beyond non-negative/overflowing, and as you say we 
can do something better in GCC 7.



3) By throwing an exception for erroneous VLAs the patch largely
defeats the VLA Sanitizer.  The sanitizer is still useful in
C++ 98 mode where the N3639 VLA runtime checking is disabled,
and when exceptions are disabled via -fno-exceptions.
Disabling  the VLA checking in C++ 98 mode doesn't seem like
a useful feature, but I didn't feel like reverting what was
a deliberate decision.


What deliberate decision?  The old code checked for C++14 mode because
the feature was part of the C++14 working paper.  What's the rationale
for C++11 as the cutoff?


Sorry, I had misremembered the original C++ 14 check as one for
C++ 11.  Are you suggesting to restore the checking only for C++
14 mode, or for all modes?  Either is easy enough to do though,
IMO, it should be safe in all modes and I would expect it to be
preferable to undefined behavior.


I think all modes.


+ /* Iterate over all non-empty initializers in this array, recursively
+building expressions to see if the elements of each are in excess
+of the (runtime) bounds of of the array type.  */
+ FOR_EACH_VEC_SAFE_ELT (v, i, ce)
+   {
+ if (1) // !vec_safe_is_empty (CONSTRUCTOR_ELTS (ce->value)))
+   {
+ tree subcheck = build_vla_check (TREE_TYPE (type),
+  ce->value,
+  vlasize, max_vlasize,
+  cst_size);
+ check = fold_build2 (TRUTH_OR_EXPR, boolean_type_node,
+  check, subcheck);
+   }
+   }


The if (1) seems left over from development.

It looks like this will multiply *cst_size by the sub-array length once 
for each element of the outer array, leading to a too-large result.  And 
also generate redundant code to check the bounds of the sub-array 
multiple times.


It seems to me that we want use the existing check for excess 
initializers in build_vec_init, in the if (length_check) section, though 
as you mention in 70019 that needs to be improved to handle STRING_CST.


Also, I think we should check for invalid bounds in 
compute_array_index_type, next to the UBsan code.  Checking bounds only 
from cp_finish_decl means that we don't check uses of VLA types other 
than variable declarations.



+ /* Avoid instrumenting constexpr functions.  Those must
+be checked statically, and the (non-constexpr) dynamic
+instrumentation would cause them to be rejected.  */


Hmm, this sounds wrong; constexpr functions can also be called with 
non-constant arguments, and the instrumentation should be folded away 
when evaluating a call with constant arguments.


Jason

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Martin Sebor]

Thanks for the comments.


2) It hardwires a rather arbitrarily restrictive limit of 64 KB
on the size of the biggest C++ VLA.  (This could stand to be
improved and made more intelligent, and perhaps integrated
with stack checking via -fstack-limit, after the GCC 6
release.)


The bounds checking should share code with build_new_1.


I agree that sharing the same code is right long term approach.

I had initially started out down that path, by factoring out code
from build_new_1 into a general function that I had thought could
be shared between it and cp_finish_decl.  But I ended up abandoning
that approach two reasons:

a) The checking expression built for array new is quite a bit less
   involved because only the major dimension of the array in requires
   runtime checking (the others must be constant and are checked at
   compile time).  In contrast, all VLA dimensions are potentially
   dynamic and so must be checked at runtime.

b) While (a) can be solved by making the checking function smart
   and general enough, it felt too intrusive and potentially risky
   to change array new at this stage.

That said, I'm happy to do this refactoring in stage 1.




3) By throwing an exception for erroneous VLAs the patch largely
defeats the VLA Sanitizer.  The sanitizer is still useful in
C++ 98 mode where the N3639 VLA runtime checking is disabled,
and when exceptions are disabled via -fno-exceptions.
Disabling  the VLA checking in C++ 98 mode doesn't seem like
a useful feature, but I didn't feel like reverting what was
a deliberate decision.


What deliberate decision?  The old code checked for C++14 mode because
the feature was part of the C++14 working paper.  What's the rationale
for C++11 as the cutoff?


Sorry, I had misremembered the original C++ 14 check as one for
C++ 11.  Are you suggesting to restore the checking only for C++
14 mode, or for all modes?  Either is easy enough to do though,
IMO, it should be safe in all modes and I would expect it to be
preferable to undefined behavior.

Martin

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Jason Merrill]

On 03/06/2016 08:38 PM, Martin Sebor wrote:

There are a few additional points to note about the patch:

1) It restores the std::bad_array_length exception from N3639,
even though the class isn't specified by the C++ standard.
At first I thought that introducing a different (private)
type would be more appropriate, but in the end couldn't come
up with a good argument for not keeping the same type.  Using
the same type also allows programs that rely on the exception
and that were built with GCC 4.9 to be ported to GCC 6 without
change.


Makes sense.


2) It hardwires a rather arbitrarily restrictive limit of 64 KB
on the size of the biggest C++ VLA.  (This could stand to be
improved and made more intelligent, and perhaps integrated
with stack checking via -fstack-limit, after the GCC 6
release.)


The bounds checking should share code with build_new_1.


3) By throwing an exception for erroneous VLAs the patch largely
defeats the VLA Sanitizer.  The sanitizer is still useful in
C++ 98 mode where the N3639 VLA runtime checking is disabled,
and when exceptions are disabled via -fno-exceptions.
Disabling  the VLA checking in C++ 98 mode doesn't seem like
a useful feature, but I didn't feel like reverting what was
a deliberate decision.


What deliberate decision?  The old code checked for C++14 mode because 
the feature was part of the C++14 working paper.  What's the rationale 
for C++11 as the cutoff?


Jason

Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[Martin Sebor]

Ping:
  https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00441.html

On 03/06/2016 06:38 PM, Martin Sebor wrote:

GCC 4.9 had added support for C++ VLAs as specified in WG21
document N3639 expected to be included in C++ 14.  However,
WG21 ultimately decided not to include N3639 in C++ 14 and
the G++ support was partially removed in 5.1.  Unfortunately,
the removal rendered some safe albeit erroneous G++ 4.9 code
undefined.  This patch restores the well-defined behavior of
such code by having it throw an exception in response to
the erroneous conditions.

While testing the patch I found a number of other problems in
the G++ support for VLAs, including PR c++/70019 - VLA size
overflow not detected, which was never implemented (not even
in 4.9).  Since this is closely related to the regression
discussed in 69517 the patch also provides that support.

There are a few additional points to note about the patch:

1) It restores the std::bad_array_length exception from N3639,
even though the class isn't specified by the C++ standard.
At first I thought that introducing a different (private)
type would be more appropriate, but in the end couldn't come
up with a good argument for not keeping the same type.  Using
the same type also allows programs that rely on the exception
and that were built with GCC 4.9 to be ported to GCC 6 without
change.

2) It hardwires a rather arbitrarily restrictive limit of 64 KB
on the size of the biggest C++ VLA.  (This could stand to be
improved and made more intelligent, and perhaps integrated
with stack  checking via -fstack-limit, after the GCC 6
release.)

3) By throwing an exception for erroneous VLAs the patch largely
defeats the VLA Sanitizer.  The sanitizer is still useful in
C++ 98 mode where the N3639 VLA runtime checking is disabled,
and when exceptions are disabled via -fno-exceptions.
Disabling  the VLA checking in C++ 98 mode doesn't seem like
a useful feature, but I didn't feel like reverting what was
a deliberate decision.

Martin