C++ PATCH for c++/69496 (ICE on VLA in constexpr function)

C++ PATCH for c++/69496 (ICE on VLA in constexpr function)

[Marek Polacek]

The (invalid) testcase was causing an ICE because we were passing the result
of array_type_nelts_top immediately into tree_int_cst_lt, but for VLAs, the
result doesn't have to be a constant.  Fixed by evaluating the bound of the
array so that we're able to give a proper out-of-bounds diagnostics.  And the
VERIFY_CONSTANT should ensure that if the array bound couldn't be reduced to
a constant, we bail out rather than evoke an ICE.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2016-01-26  Marek Polacek  <polacek@redhat.com>

	PR c++/69496
	* constexpr.c (cxx_eval_array_reference): Evaluate the number of
	elements of the array.

	* g++.dg/cpp1y/constexpr-vla1.C: New test.

diff --git gcc/cp/constexpr.c gcc/cp/constexpr.c
index 6b0e5a8..6324e45 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -1855,7 +1855,12 @@ cxx_eval_array_reference (const constexpr_ctx *ctx, tree t,
 
   if (!found)
     {
-      if (tree_int_cst_lt (index, array_type_nelts_top (TREE_TYPE (ary))))
+      tree nelts = array_type_nelts_top (TREE_TYPE (ary));
+      /* For VLAs, the number of elements won't be an integer constant.  */
+      nelts = cxx_eval_constant_expression (ctx, nelts, false, non_constant_p,
+					    overflow_p);
+      VERIFY_CONSTANT (nelts);
+      if (tree_int_cst_lt (index, nelts))
 	{
 	  if (TREE_CODE (ary) == CONSTRUCTOR
 	      && CONSTRUCTOR_NO_IMPLICIT_ZERO (ary))
diff --git gcc/testsuite/g++.dg/cpp1y/constexpr-vla1.C gcc/testsuite/g++.dg/cpp1y/constexpr-vla1.C
index e69de29..a5615bb 100644
--- gcc/testsuite/g++.dg/cpp1y/constexpr-vla1.C
+++ gcc/testsuite/g++.dg/cpp1y/constexpr-vla1.C
@@ -0,0 +1,30 @@
+// PR c++/69496
+// { dg-do compile { target c++14 } }
+
+constexpr int
+fn_ok (int n)
+{
+    __extension__ int a[n] = { };
+    int z = 0;
+
+    for (unsigned i = 0; i < sizeof (a) / sizeof (int); ++i)
+      z += a[i];
+
+    return z;
+}
+
+
+constexpr int
+fn_not_ok (int n)
+{
+    __extension__ int a[n] = { };
+    int z = 0;
+
+    for (unsigned i = 0; i < sizeof (a); ++i)
+      z += a[i];
+
+    return z;
+}
+
+constexpr int n1 = fn_ok (3);
+constexpr int n2 = fn_not_ok (3); // { dg-error "array subscript out of bound" }

	Marek

Re: C++ PATCH for c++/69496 (ICE on VLA in constexpr function)

[Martin Sebor]

On 01/26/2016 04:02 PM, Marek Polacek wrote:
> The (invalid) testcase was causing an ICE because we were passing the result
> of array_type_nelts_top immediately into tree_int_cst_lt, but for VLAs, the
> result doesn't have to be a constant.  Fixed by evaluating the bound of the
> array so that we're able to give a proper out-of-bounds diagnostics.  And the
> VERIFY_CONSTANT should ensure that if the array bound couldn't be reduced to
> a constant, we bail out rather than evoke an ICE.

Wow, you are quick! :)

I wonder if it might be better to instead reject VLAs in constexpr
functions altogether.  Not because they're not in C++, but because
C (or gcc) doesn't allow them to be initialized (and so accepting
an initialized VLA is a g++ extension of an extension), and
because in constexpr functions they are rejected without
initialization just like other uninitialized variables.

FWIW, it seems to me the fewer extensions we support the less risk
of something going wrong because of unforeseen interactions with
other features.

Martin

Re: C++ PATCH for c++/69496 (ICE on VLA in constexpr function)

[Marek Polacek]

On Tue, Jan 26, 2016 at 08:58:06PM -0700, Martin Sebor wrote:
> On 01/26/2016 04:02 PM, Marek Polacek wrote:
> >The (invalid) testcase was causing an ICE because we were passing the result
> >of array_type_nelts_top immediately into tree_int_cst_lt, but for VLAs, the
> >result doesn't have to be a constant.  Fixed by evaluating the bound of the
> >array so that we're able to give a proper out-of-bounds diagnostics.  And the
> >VERIFY_CONSTANT should ensure that if the array bound couldn't be reduced to
> >a constant, we bail out rather than evoke an ICE.
> 
> Wow, you are quick! :)
> 
> I wonder if it might be better to instead reject VLAs in constexpr
> functions altogether.  Not because they're not in C++, but because
> C (or gcc) doesn't allow them to be initialized (and so accepting
> an initialized VLA is a g++ extension of an extension), and
> because in constexpr functions they are rejected without
> initialization just like other uninitialized variables.

I don't think we can do this at this time.  E.g. the following program works
even with GCC 5 and -std=c++14:

constexpr int
foo (int n)
{
    int a[n] = { 1, 2, 3 };
    int z = 0;
    for (unsigned i = 0; i < 3; ++i)
      z += a[i];
    return z;
}

int
main ()
{
  constexpr int n = foo (3);
  __builtin_printf ("%d\n", n);
}

So starting to reject such a code might broke working programs.  And we're
able to reject non-standard code: -pedantic-errors.
 
> FWIW, it seems to me the fewer extensions we support the less risk
> of something going wrong because of unforeseen interactions with
> other features.

True... (hi statement expressions!).

	Marek

Re: C++ PATCH for c++/69496 (ICE on VLA in constexpr function)

[Jason Merrill]

OK, but the testcase should go in ext/.

Jason

Re: C++ PATCH for c++/69496 (ICE on VLA in constexpr function)

[Marek Polacek]

On Wed, Jan 27, 2016 at 09:08:27AM -0500, Jason Merrill wrote:
> OK, but the testcase should go in ext/.

Oops, will move it there then, thanks!

	Marek

Re: C++ PATCH for c++/69496 (ICE on VLA in constexpr function)

[Martin Sebor]

>> I wonder if it might be better to instead reject VLAs in constexpr
>> functions altogether.  Not because they're not in C++, but because
>> C (or gcc) doesn't allow them to be initialized (and so accepting
>> an initialized VLA is a g++ extension of an extension), and
>> because in constexpr functions they are rejected without
>> initialization just like other uninitialized variables.
>
> I don't think we can do this at this time.  E.g. the following program works
> even with GCC 5 and -std=c++14:
>
> constexpr int
> foo (int n)
> {
>      int a[n] = { 1, 2, 3 };
>      int z = 0;
>      for (unsigned i = 0; i < 3; ++i)
>        z += a[i];
>      return z;
> }
>
> int
> main ()
> {
>    constexpr int n = foo (3);
>    __builtin_printf ("%d\n", n);
> }

This happens to work but I suspect it's only by accident.  When
the number of elements in the initializer is increased to exceed
the number of elements in the VLA GCC gets into infinite recursion.
(I opened bug 69516 with a test case).  The same error in a non-
constexpr function causes a SEGV at runtime (this is also
a regression WRT 4.9.3 -- I opened bug 69517 for it).

> So starting to reject such a code might broke working programs.  And we're
> able to reject non-standard code: -pedantic-errors.

I agree that there is some risk that it might break some working
programs.  I would expect the most common use of initialized VLAs
be to set all elements to zero using the "= { }" or "= { 0 }"
syntax.  Initializers with more elements are, IMO, likely to be
a bug where the user doesn't realize they defined a VLA rather
than an ordinary array.  Since VLAs are required to have at least
1 element, would diagnosing initializers with more than one element
more loudly (such as by default or with -Wall as opposed to with
-Wpedantic) be a good solution?

Martin

Re: C++ PATCH for c++/69496 (ICE on VLA in constexpr function)

[Jason Merrill]

On 01/27/2016 11:52 AM, Martin Sebor wrote:
> I agree that there is some risk that it might break some working
> programs.  I would expect the most common use of initialized VLAs
> be to set all elements to zero using the "= { }" or "= { 0 }"
> syntax.  Initializers with more elements are, IMO, likely to be
> a bug where the user doesn't realize they defined a VLA rather
> than an ordinary array.  Since VLAs are required to have at least
> 1 element, would diagnosing initializers with more than one element
> more loudly (such as by default or with -Wall as opposed to with
> -Wpedantic) be a good solution?

That makes sense to me.

Jason

Re: C++ PATCH for c++/69496 (ICE on VLA in constexpr function)

[Marek Polacek]

On Wed, Jan 27, 2016 at 09:52:13AM -0700, Martin Sebor wrote:
> This happens to work but I suspect it's only by accident.  When
> the number of elements in the initializer is increased to exceed
> the number of elements in the VLA GCC gets into infinite recursion.
> (I opened bug 69516 with a test case).  The same error in a non-
> constexpr function causes a SEGV at runtime (this is also
> a regression WRT 4.9.3 -- I opened bug 69517 for it).

FWIW, the patch I posted today should cure all those infinite recursions.

	Marek

Re: C++ PATCH for c++/69496 (ICE on VLA in constexpr function)

[Martin Sebor]

On 01/28/2016 03:27 PM, Marek Polacek wrote:
> On Wed, Jan 27, 2016 at 09:52:13AM -0700, Martin Sebor wrote:
>> This happens to work but I suspect it's only by accident.  When
>> the number of elements in the initializer is increased to exceed
>> the number of elements in the VLA GCC gets into infinite recursion.
>> (I opened bug 69516 with a test case).  The same error in a non-
>> constexpr function causes a SEGV at runtime (this is also
>> a regression WRT 4.9.3 -- I opened bug 69517 for it).
>
> FWIW, the patch I posted today should cure all those infinite recursions.

Sounds good, and thanks for the heads up.  I volunteered to add
a louder warning for the VLA initialization and I hope to find
some time to work on it as early as next week.

Martin