c++: Fix ICE with invalid array bounds [PR93789]

Message ID 20200226204439.547552-1-polacek@redhat.com
State New
Headers show
Series
  • c++: Fix ICE with invalid array bounds [PR93789]
Related show

Commit Message

Marek Polacek Feb. 26, 2020, 8:44 p.m.
r7-2111 introduced maybe_constant_value in cp_fully_fold.
maybe_constant_value uses cxx_eval_outermost_constant_expr, which
can clear TREE_CONSTANT:
6510   else if (non_constant_p && TREE_CONSTANT (r))
[...]
6529       TREE_CONSTANT (r) = false;

In this test the array size is '(long int) "h"'.  This used to be
TREE_CONSTANT but given the change above, the flag will be cleared
when we cp_fully_fold the array size in compute_array_index_type_loc.
That means we don't emit an error in the
10391   else if (TREE_CONSTANT (size)
block in the same function, and we go on.  Then we compute ITYPE
using cp_build_binary_op and use maybe_constant_value on it and
suddenly we have something that's TREE_CONSTANT again.  And then we
crash in reshape_init_array_1 in tree_to_uhwi, because what we have
doesn't fit in an unsigned HWI.

icc accepts this code, but since we used to reject it, I see no desire
to make this work, so I've added a check that triggers when we end up
with a constant that is not an INTEGER_CST.

Bootstrapped/regtested on x86_64-linux, ok for trunk?  Probably no need
to change released versions.

2020-02-26  Marek Polacek  <polacek@redhat.com>

	PR c++/93789 - ICE with invalid array bounds.
	* decl.c (compute_array_index_type_loc): Give an error
	when ITYPE ends up being a constant that is not an INTEGER_CST.

	* g++.dg/ext/vla22.C: New test.
---
 gcc/cp/decl.c                    |  7 +++++++
 gcc/testsuite/g++.dg/ext/vla22.C | 10 ++++++++++
 2 files changed, 17 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/ext/vla22.C


base-commit: 051b9873e78fe1acb1a3fecd0c6e5685b6c12fb3
-- 
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA

Comments

Jason Merrill Feb. 26, 2020, 10:54 p.m. | #1
On 2/26/20 3:44 PM, Marek Polacek wrote:
> r7-2111 introduced maybe_constant_value in cp_fully_fold.

> maybe_constant_value uses cxx_eval_outermost_constant_expr, which

> can clear TREE_CONSTANT:

> 6510   else if (non_constant_p && TREE_CONSTANT (r))

> [...]

> 6529       TREE_CONSTANT (r) = false;

> 

> In this test the array size is '(long int) "h"'.  This used to be

> TREE_CONSTANT but given the change above, the flag will be cleared

> when we cp_fully_fold the array size in compute_array_index_type_loc.


I wonder about giving an error at that point; if size is TREE_CONSTANT 
and folded is not, we're in a strange situation already.

> That means we don't emit an error in the

> 10391   else if (TREE_CONSTANT (size)

> block in the same function, and we go on.  Then we compute ITYPE

> using cp_build_binary_op and use maybe_constant_value on it and

> suddenly we have something that's TREE_CONSTANT again.


Why does maybe_constant_value consider (long)"h" - 1 to be a 
constant-expression?

> And then we

> crash in reshape_init_array_1 in tree_to_uhwi, because what we have

> doesn't fit in an unsigned HWI.

> 

> icc accepts this code, but since we used to reject it, I see no desire

> to make this work, so I've added a check that triggers when we end up

> with a constant that is not an INTEGER_CST.

> 

> Bootstrapped/regtested on x86_64-linux, ok for trunk?  Probably no need

> to change released versions.

> 

> 2020-02-26  Marek Polacek  <polacek@redhat.com>

> 

> 	PR c++/93789 - ICE with invalid array bounds.

> 	* decl.c (compute_array_index_type_loc): Give an error

> 	when ITYPE ends up being a constant that is not an INTEGER_CST.

> 

> 	* g++.dg/ext/vla22.C: New test.

> ---

>   gcc/cp/decl.c                    |  7 +++++++

>   gcc/testsuite/g++.dg/ext/vla22.C | 10 ++++++++++

>   2 files changed, 17 insertions(+)

>   create mode 100644 gcc/testsuite/g++.dg/ext/vla22.C

> 

> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c

> index 1947c4ddb7f..4ac29d99d04 100644

> --- a/gcc/cp/decl.c

> +++ b/gcc/cp/decl.c

> @@ -10473,6 +10473,13 @@ compute_array_index_type_loc (location_t name_loc, tree name, tree size,

>   	  error ("overflow in array dimension");

>   	  TREE_OVERFLOW (itype) = 0;

>   	}

> +      else if (TREE_CODE (itype) != INTEGER_CST)

> +	{

> +	  if (!(complain & tf_error))

> +	    return error_mark_node;

> +	  invalid_array_size_error (loc, cst_size_not_constant, itype, name);

> +	  itype = integer_one_node;

> +	}

>       }

>   

>     /* Create and return the appropriate index type.  */

> diff --git a/gcc/testsuite/g++.dg/ext/vla22.C b/gcc/testsuite/g++.dg/ext/vla22.C

> new file mode 100644

> index 00000000000..22e200b49cf

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/ext/vla22.C

> @@ -0,0 +1,10 @@

> +// PR c++/93789 - ICE with invalid array bounds.

> +// { dg-do compile }

> +// { dg-options "" }

> +

> +void

> +f ()

> +{

> +  const int tbl[(long) "h"] = { 12 }; // { dg-error "size of array .tbl. is not a constant expression" "" { target c++11 } }

> +// { dg-error "size of array .tbl. is not an integral constant-expression" "" { target c++98_only } .-1 }

> +}

> 

> base-commit: 051b9873e78fe1acb1a3fecd0c6e5685b6c12fb3

>
Martin Sebor Feb. 27, 2020, 1:01 a.m. | #2
On 2/26/20 1:44 PM, Marek Polacek wrote:
> r7-2111 introduced maybe_constant_value in cp_fully_fold.

> maybe_constant_value uses cxx_eval_outermost_constant_expr, which

> can clear TREE_CONSTANT:

> 6510   else if (non_constant_p && TREE_CONSTANT (r))

> [...]

> 6529       TREE_CONSTANT (r) = false;

> 

> In this test the array size is '(long int) "h"'.  This used to be

> TREE_CONSTANT but given the change above, the flag will be cleared

> when we cp_fully_fold the array size in compute_array_index_type_loc.

> That means we don't emit an error in the

> 10391   else if (TREE_CONSTANT (size)

> block in the same function, and we go on.  Then we compute ITYPE

> using cp_build_binary_op and use maybe_constant_value on it and

> suddenly we have something that's TREE_CONSTANT again.  And then we

> crash in reshape_init_array_1 in tree_to_uhwi, because what we have

> doesn't fit in an unsigned HWI.

> 

> icc accepts this code, but since we used to reject it, I see no desire

> to make this work, so I've added a check that triggers when we end up

> with a constant that is not an INTEGER_CST.


G++ accepts this equivalent:

   void f () {
     long n = ( long ) "h";
     const int tbl [n] = { 12 };
     ...
   }

and it doesn't look like the patch changes that.

I think all these cases of initialized VLAs ought to be handled
the same way: either they should all be rejected or they should
all be accepted.  The rather dated pr58646, also a regression,
is another example of a few ICEs in this area.  See also this
recently submitted patch:
   https://gcc.gnu.org/ml/gcc-patches/2020-02/msg01148.html

FWIW, at one point I got VLA initialization to work again (r234966),
after it had been removed due to the last minute removal of VLAs
from the standard, but it caused trouble for Java and I was forced
to revert the changes.  I've been hoping to revisit it but other
things have been getting in the way.

Martin


> 

> Bootstrapped/regtested on x86_64-linux, ok for trunk?  Probably no need

> to change released versions.

> 

> 2020-02-26  Marek Polacek  <polacek@redhat.com>

> 

> 	PR c++/93789 - ICE with invalid array bounds.

> 	* decl.c (compute_array_index_type_loc): Give an error

> 	when ITYPE ends up being a constant that is not an INTEGER_CST.

> 

> 	* g++.dg/ext/vla22.C: New test.

> ---

>   gcc/cp/decl.c                    |  7 +++++++

>   gcc/testsuite/g++.dg/ext/vla22.C | 10 ++++++++++

>   2 files changed, 17 insertions(+)

>   create mode 100644 gcc/testsuite/g++.dg/ext/vla22.C

> 

> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c

> index 1947c4ddb7f..4ac29d99d04 100644

> --- a/gcc/cp/decl.c

> +++ b/gcc/cp/decl.c

> @@ -10473,6 +10473,13 @@ compute_array_index_type_loc (location_t name_loc, tree name, tree size,

>   	  error ("overflow in array dimension");

>   	  TREE_OVERFLOW (itype) = 0;

>   	}

> +      else if (TREE_CODE (itype) != INTEGER_CST)

> +	{

> +	  if (!(complain & tf_error))

> +	    return error_mark_node;

> +	  invalid_array_size_error (loc, cst_size_not_constant, itype, name);

> +	  itype = integer_one_node;

> +	}

>       }

>   

>     /* Create and return the appropriate index type.  */

> diff --git a/gcc/testsuite/g++.dg/ext/vla22.C b/gcc/testsuite/g++.dg/ext/vla22.C

> new file mode 100644

> index 00000000000..22e200b49cf

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/ext/vla22.C

> @@ -0,0 +1,10 @@

> +// PR c++/93789 - ICE with invalid array bounds.

> +// { dg-do compile }

> +// { dg-options "" }

> +

> +void

> +f ()

> +{

> +  const int tbl[(long) "h"] = { 12 }; // { dg-error "size of array .tbl. is not a constant expression" "" { target c++11 } }

> +// { dg-error "size of array .tbl. is not an integral constant-expression" "" { target c++98_only } .-1 }

> +}

> 

> base-commit: 051b9873e78fe1acb1a3fecd0c6e5685b6c12fb3

>
Marek Polacek Feb. 27, 2020, 2:15 a.m. | #3
On Wed, Feb 26, 2020 at 06:01:30PM -0700, Martin Sebor wrote:
> On 2/26/20 1:44 PM, Marek Polacek wrote:

> > r7-2111 introduced maybe_constant_value in cp_fully_fold.

> > maybe_constant_value uses cxx_eval_outermost_constant_expr, which

> > can clear TREE_CONSTANT:

> > 6510   else if (non_constant_p && TREE_CONSTANT (r))

> > [...]

> > 6529       TREE_CONSTANT (r) = false;

> > 

> > In this test the array size is '(long int) "h"'.  This used to be

> > TREE_CONSTANT but given the change above, the flag will be cleared

> > when we cp_fully_fold the array size in compute_array_index_type_loc.

> > That means we don't emit an error in the

> > 10391   else if (TREE_CONSTANT (size)

> > block in the same function, and we go on.  Then we compute ITYPE

> > using cp_build_binary_op and use maybe_constant_value on it and

> > suddenly we have something that's TREE_CONSTANT again.  And then we

> > crash in reshape_init_array_1 in tree_to_uhwi, because what we have

> > doesn't fit in an unsigned HWI.

> > 

> > icc accepts this code, but since we used to reject it, I see no desire

> > to make this work, so I've added a check that triggers when we end up

> > with a constant that is not an INTEGER_CST.

> 

> G++ accepts this equivalent:

> 

>   void f () {

>     long n = ( long ) "h";

>     const int tbl [n] = { 12 };

>     ...

>   }

> 

> and it doesn't look like the patch changes that.


It does not, but g++ will reject the same code if you make 'n' const:
a) if 'n' is not const, we won't be able to fold 'n' and it's not
   TREE_CONSTANT -> we treat it as a VLA
b) if 'n' is const, we still can't fold it to an INTEGER_CST but it is
   TREE_CONSTANT -> we give an error
c) if 'n' is constexpr, it's ill-formed since we're converting a pointer
   to integral type (more on this in my next mail)

I would reject this even in a), because we're likely allocating something
huge that's likely to overflow the stack, making the code very questionable.

> I think all these cases of initialized VLAs ought to be handled

> the same way: either they should all be rejected or they should

> all be accepted.  The rather dated pr58646, also a regression,

> is another example of a few ICEs in this area.  See also this

> recently submitted patch:

>   https://gcc.gnu.org/ml/gcc-patches/2020-02/msg01148.html


Yeah, it's a gift that keeps on giving :/.

> FWIW, at one point I got VLA initialization to work again (r234966),

> after it had been removed due to the last minute removal of VLAs

> from the standard, but it caused trouble for Java and I was forced

> to revert the changes.  I've been hoping to revisit it but other

> things have been getting in the way.


I think clang doesn't allow VLA initialization at all, so there's that.
I haven't reviewed our old correspondence on this topic but I think I
was, and still am, for restricting what we allow users to do with VLAs.

--
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
Marek Polacek Feb. 27, 2020, 2:31 a.m. | #4
On Wed, Feb 26, 2020 at 05:54:03PM -0500, Jason Merrill wrote:
> On 2/26/20 3:44 PM, Marek Polacek wrote:

> > r7-2111 introduced maybe_constant_value in cp_fully_fold.

> > maybe_constant_value uses cxx_eval_outermost_constant_expr, which

> > can clear TREE_CONSTANT:

> > 6510   else if (non_constant_p && TREE_CONSTANT (r))

> > [...]

> > 6529       TREE_CONSTANT (r) = false;

> > 

> > In this test the array size is '(long int) "h"'.  This used to be

> > TREE_CONSTANT but given the change above, the flag will be cleared

> > when we cp_fully_fold the array size in compute_array_index_type_loc.

> 

> I wonder about giving an error at that point; if size is TREE_CONSTANT and

> folded is not, we're in a strange situation already.


That works as well; how about the attached patch?

> > That means we don't emit an error in the

> > 10391   else if (TREE_CONSTANT (size)

> > block in the same function, and we go on.  Then we compute ITYPE

> > using cp_build_binary_op and use maybe_constant_value on it and

> > suddenly we have something that's TREE_CONSTANT again.

> 

> Why does maybe_constant_value consider (long)"h" - 1 to be a

> constant-expression?


That's because this check in cxx_eval_outermost_constant_expr:
  if (CONVERT_EXPR_CODE_P (TREE_CODE (r)) 
      && ARITHMETIC_TYPE_P (TREE_TYPE (r)) 
      && TREE_CODE (TREE_OPERAND (r, 0)) == ADDR_EXPR)
    {    
      if (!allow_non_constant)
        error ("conversion from pointer type %qT "
               "to arithmetic type %qT in a constant expression",
               TREE_TYPE (TREE_OPERAND (r, 0)), TREE_TYPE (r));
      non_constant_p = true;
    }
doesn't work for all subexpressions, as the comment says.  As a consequence,
this test

constexpr long int
foo ()
{
  return (long int) "foo"
#ifdef FOO
    - 1
#endif
    ;
}

constexpr long int l = foo ();

is accepted with -DFOO but rejected otherwise.  Converting a pointer to an
integral type must be done via a reinterpret_cast and that can't be part of
a core constant expression.  Do you want a PR for this?

-- >8 --
r7-2111 introduced maybe_constant_value in cp_fully_fold.
maybe_constant_value uses cxx_eval_outermost_constant_expr, which
can clear TREE_CONSTANT:
6510   else if (non_constant_p && TREE_CONSTANT (r))
[...]
6529       TREE_CONSTANT (r) = false;

In this test the array size is '(long int) "h"'.  This used to be
TREE_CONSTANT but given the change above, the flag will be cleared
when we cp_fully_fold the array size in compute_array_index_type_loc.
That means we don't emit an error in the
10391   else if (TREE_CONSTANT (size)
block in the same function, and we go on.  Then we compute ITYPE
using cp_build_binary_op and use maybe_constant_value on it and
suddenly we have something that's TREE_CONSTANT again.  And then we
crash in reshape_init_array_1 in tree_to_uhwi, because what we have
doesn't fit in an unsigned HWI.

icc accepts this code, but since we used to reject it, I see no desire
to make this work, so don't use the folded result when we've lost
the TREE_CONSTANT flag while evaluating the size.

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

2020-02-26  Marek Polacek  <polacek@redhat.com>

	PR c++/93789 - ICE with invalid array bounds.
	* decl.c (compute_array_index_type_loc): Don't use the folded
	size when folding cleared TREE_CONSTANT.

	* g++.dg/ext/vla22.C: New test.
---
 gcc/cp/decl.c                    | 11 ++++++++---
 gcc/testsuite/g++.dg/ext/vla22.C |  9 +++++++++
 2 files changed, 17 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/ext/vla22.C

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 1947c4ddb7f..e3f4b435a49 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -10338,9 +10338,14 @@ compute_array_index_type_loc (location_t name_loc, tree name, tree size,
 	    pedwarn (loc, OPT_Wpedantic,
 		     "size of array is not an integral constant-expression");
 	}
-      /* Use the folded result for VLAs, too; it will have resolved
-	 SIZEOF_EXPR.  */
-      size = folded;
+      if (TREE_CONSTANT (size) && !TREE_CONSTANT (folded))
+	/* We might have lost the TREE_CONSTANT flag e.g. when we are
+	   folding a conversion from a pointer to integral type.  In that
+	   case issue an error below and don't treat this as a VLA.  */;
+      else
+	/* Use the folded result for VLAs, too; it will have resolved
+	   SIZEOF_EXPR.  */
+	size = folded;
     }
 
   /* Normally, the array-bound will be a constant.  */
diff --git a/gcc/testsuite/g++.dg/ext/vla22.C b/gcc/testsuite/g++.dg/ext/vla22.C
new file mode 100644
index 00000000000..2308ee748df
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/vla22.C
@@ -0,0 +1,9 @@
+// PR c++/93789 - ICE with invalid array bounds.
+// { dg-do compile }
+// { dg-options "" }
+
+void
+f ()
+{
+  const int tbl[(long) "h"] = { 12 }; // { dg-error "size of array .tbl. is not an integral constant-expression" }
+}

base-commit: 89f759ac2ebb9f09ce5655ce5d791793922c612d
-- 
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
Jason Merrill Feb. 27, 2020, 2:41 a.m. | #5
On 2/26/20 9:31 PM, Marek Polacek wrote:
> On Wed, Feb 26, 2020 at 05:54:03PM -0500, Jason Merrill wrote:

>> On 2/26/20 3:44 PM, Marek Polacek wrote:

>>> r7-2111 introduced maybe_constant_value in cp_fully_fold.

>>> maybe_constant_value uses cxx_eval_outermost_constant_expr, which

>>> can clear TREE_CONSTANT:

>>> 6510   else if (non_constant_p && TREE_CONSTANT (r))

>>> [...]

>>> 6529       TREE_CONSTANT (r) = false;

>>>

>>> In this test the array size is '(long int) "h"'.  This used to be

>>> TREE_CONSTANT but given the change above, the flag will be cleared

>>> when we cp_fully_fold the array size in compute_array_index_type_loc.

>>

>> I wonder about giving an error at that point; if size is TREE_CONSTANT and

>> folded is not, we're in a strange situation already.

> 

> That works as well; how about the attached patch?


OK.

>>> That means we don't emit an error in the

>>> 10391   else if (TREE_CONSTANT (size)

>>> block in the same function, and we go on.  Then we compute ITYPE

>>> using cp_build_binary_op and use maybe_constant_value on it and

>>> suddenly we have something that's TREE_CONSTANT again.

>>

>> Why does maybe_constant_value consider (long)"h" - 1 to be a

>> constant-expression?

> 

> That's because this check in cxx_eval_outermost_constant_expr:

>    if (CONVERT_EXPR_CODE_P (TREE_CODE (r))

>        && ARITHMETIC_TYPE_P (TREE_TYPE (r))

>        && TREE_CODE (TREE_OPERAND (r, 0)) == ADDR_EXPR)

>      {

>        if (!allow_non_constant)

>          error ("conversion from pointer type %qT "

>                 "to arithmetic type %qT in a constant expression",

>                 TREE_TYPE (TREE_OPERAND (r, 0)), TREE_TYPE (r));

>        non_constant_p = true;

>      }

> doesn't work for all subexpressions, as the comment says.  As a consequence,

> this test

> 

> constexpr long int

> foo ()

> {

>    return (long int) "foo"

> #ifdef FOO

>      - 1

> #endif

>      ;

> }

> 

> constexpr long int l = foo ();

> 

> is accepted with -DFOO but rejected otherwise.  Converting a pointer to an

> integral type must be done via a reinterpret_cast and that can't be part of

> a core constant expression.  Do you want a PR for this?


Please.

> -- >8 --

> r7-2111 introduced maybe_constant_value in cp_fully_fold.

> maybe_constant_value uses cxx_eval_outermost_constant_expr, which

> can clear TREE_CONSTANT:

> 6510   else if (non_constant_p && TREE_CONSTANT (r))

> [...]

> 6529       TREE_CONSTANT (r) = false;

> 

> In this test the array size is '(long int) "h"'.  This used to be

> TREE_CONSTANT but given the change above, the flag will be cleared

> when we cp_fully_fold the array size in compute_array_index_type_loc.

> That means we don't emit an error in the

> 10391   else if (TREE_CONSTANT (size)

> block in the same function, and we go on.  Then we compute ITYPE

> using cp_build_binary_op and use maybe_constant_value on it and

> suddenly we have something that's TREE_CONSTANT again.  And then we

> crash in reshape_init_array_1 in tree_to_uhwi, because what we have

> doesn't fit in an unsigned HWI.

> 

> icc accepts this code, but since we used to reject it, I see no desire

> to make this work, so don't use the folded result when we've lost

> the TREE_CONSTANT flag while evaluating the size.

> 

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

> 

> 2020-02-26  Marek Polacek  <polacek@redhat.com>

> 

> 	PR c++/93789 - ICE with invalid array bounds.

> 	* decl.c (compute_array_index_type_loc): Don't use the folded

> 	size when folding cleared TREE_CONSTANT.

> 

> 	* g++.dg/ext/vla22.C: New test.

> ---

>   gcc/cp/decl.c                    | 11 ++++++++---

>   gcc/testsuite/g++.dg/ext/vla22.C |  9 +++++++++

>   2 files changed, 17 insertions(+), 3 deletions(-)

>   create mode 100644 gcc/testsuite/g++.dg/ext/vla22.C

> 

> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c

> index 1947c4ddb7f..e3f4b435a49 100644

> --- a/gcc/cp/decl.c

> +++ b/gcc/cp/decl.c

> @@ -10338,9 +10338,14 @@ compute_array_index_type_loc (location_t name_loc, tree name, tree size,

>   	    pedwarn (loc, OPT_Wpedantic,

>   		     "size of array is not an integral constant-expression");

>   	}

> -      /* Use the folded result for VLAs, too; it will have resolved

> -	 SIZEOF_EXPR.  */

> -      size = folded;

> +      if (TREE_CONSTANT (size) && !TREE_CONSTANT (folded))

> +	/* We might have lost the TREE_CONSTANT flag e.g. when we are

> +	   folding a conversion from a pointer to integral type.  In that

> +	   case issue an error below and don't treat this as a VLA.  */;

> +      else

> +	/* Use the folded result for VLAs, too; it will have resolved

> +	   SIZEOF_EXPR.  */

> +	size = folded;

>       }

>   

>     /* Normally, the array-bound will be a constant.  */

> diff --git a/gcc/testsuite/g++.dg/ext/vla22.C b/gcc/testsuite/g++.dg/ext/vla22.C

> new file mode 100644

> index 00000000000..2308ee748df

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/ext/vla22.C

> @@ -0,0 +1,9 @@

> +// PR c++/93789 - ICE with invalid array bounds.

> +// { dg-do compile }

> +// { dg-options "" }

> +

> +void

> +f ()

> +{

> +  const int tbl[(long) "h"] = { 12 }; // { dg-error "size of array .tbl. is not an integral constant-expression" }

> +}

> 

> base-commit: 89f759ac2ebb9f09ce5655ce5d791793922c612d

>
Marek Polacek Feb. 27, 2020, 2:51 a.m. | #6
On Wed, Feb 26, 2020 at 09:41:14PM -0500, Jason Merrill wrote:
> On 2/26/20 9:31 PM, Marek Polacek wrote:

> > On Wed, Feb 26, 2020 at 05:54:03PM -0500, Jason Merrill wrote:

> > > On 2/26/20 3:44 PM, Marek Polacek wrote:

> > > > r7-2111 introduced maybe_constant_value in cp_fully_fold.

> > > > maybe_constant_value uses cxx_eval_outermost_constant_expr, which

> > > > can clear TREE_CONSTANT:

> > > > 6510   else if (non_constant_p && TREE_CONSTANT (r))

> > > > [...]

> > > > 6529       TREE_CONSTANT (r) = false;

> > > > 

> > > > In this test the array size is '(long int) "h"'.  This used to be

> > > > TREE_CONSTANT but given the change above, the flag will be cleared

> > > > when we cp_fully_fold the array size in compute_array_index_type_loc.

> > > 

> > > I wonder about giving an error at that point; if size is TREE_CONSTANT and

> > > folded is not, we're in a strange situation already.

> > 

> > That works as well; how about the attached patch?

> 

> OK.


Thanks.

> > > > That means we don't emit an error in the

> > > > 10391   else if (TREE_CONSTANT (size)

> > > > block in the same function, and we go on.  Then we compute ITYPE

> > > > using cp_build_binary_op and use maybe_constant_value on it and

> > > > suddenly we have something that's TREE_CONSTANT again.

> > > 

> > > Why does maybe_constant_value consider (long)"h" - 1 to be a

> > > constant-expression?

> > 

> > That's because this check in cxx_eval_outermost_constant_expr:

> >    if (CONVERT_EXPR_CODE_P (TREE_CODE (r))

> >        && ARITHMETIC_TYPE_P (TREE_TYPE (r))

> >        && TREE_CODE (TREE_OPERAND (r, 0)) == ADDR_EXPR)

> >      {

> >        if (!allow_non_constant)

> >          error ("conversion from pointer type %qT "

> >                 "to arithmetic type %qT in a constant expression",

> >                 TREE_TYPE (TREE_OPERAND (r, 0)), TREE_TYPE (r));

> >        non_constant_p = true;

> >      }

> > doesn't work for all subexpressions, as the comment says.  As a consequence,

> > this test

> > 

> > constexpr long int

> > foo ()

> > {

> >    return (long int) "foo"

> > #ifdef FOO

> >      - 1

> > #endif

> >      ;

> > }

> > 

> > constexpr long int l = foo ();

> > 

> > is accepted with -DFOO but rejected otherwise.  Converting a pointer to an

> > integral type must be done via a reinterpret_cast and that can't be part of

> > a core constant expression.  Do you want a PR for this?

> 

> Please.


https://gcc.gnu.org/PR93955

--
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA

Patch

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 1947c4ddb7f..4ac29d99d04 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -10473,6 +10473,13 @@  compute_array_index_type_loc (location_t name_loc, tree name, tree size,
 	  error ("overflow in array dimension");
 	  TREE_OVERFLOW (itype) = 0;
 	}
+      else if (TREE_CODE (itype) != INTEGER_CST)
+	{
+	  if (!(complain & tf_error))
+	    return error_mark_node;
+	  invalid_array_size_error (loc, cst_size_not_constant, itype, name);
+	  itype = integer_one_node;
+	}
     }
 
   /* Create and return the appropriate index type.  */
diff --git a/gcc/testsuite/g++.dg/ext/vla22.C b/gcc/testsuite/g++.dg/ext/vla22.C
new file mode 100644
index 00000000000..22e200b49cf
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/vla22.C
@@ -0,0 +1,10 @@ 
+// PR c++/93789 - ICE with invalid array bounds.
+// { dg-do compile }
+// { dg-options "" }
+
+void
+f ()
+{
+  const int tbl[(long) "h"] = { 12 }; // { dg-error "size of array .tbl. is not a constant expression" "" { target c++11 } }
+// { dg-error "size of array .tbl. is not an integral constant-expression" "" { target c++98_only } .-1 }
+}