[PATCH] c++: ICE with template NEW_EXPR [PR105803]

[PATCH] c++: ICE with template NEW_EXPR [PR105803]

[Marek Polacek]

Here we ICE because value_dependent_expression_p gets a NEW_EXPR
whose operand is a type, and we go to the default case which just
calls v_d_e_p on each operand of the NEW_EXPR.  Since one of them
is a type, we crash on the new assert in t_d_e_p.

t_d_e_p has code to handle {,VEC_}NEW_EXPR, which at this point
was already performed, so I think we can handle these two codes
specifically and skip the second operand, which is always going
to be a type.

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

	PR c++/105803

gcc/cp/ChangeLog:

	* pt.cc (value_dependent_expression_p): Handle {,VEC_}NEW_EXPR
	in the switch.

gcc/testsuite/ChangeLog:

	* g++.dg/template/new13.C: New test.
---
 gcc/cp/pt.cc                          |  8 ++++++++
 gcc/testsuite/g++.dg/template/new13.C | 11 +++++++++++
 2 files changed, 19 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/template/new13.C

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 6de8e496859..836861e1039 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -27643,6 +27643,14 @@ value_dependent_expression_p (tree expression)
 	 under instantiate_non_dependent_expr; it can't be constant.  */
       return true;
 
+    case NEW_EXPR:
+    case VEC_NEW_EXPR:
+      /* The second operand is a type, which type_dependent_expression_p
+	 (and therefore value_dependent_expression_p) doesn't want to see.  */
+      return (value_dependent_expression_p (TREE_OPERAND (expression, 0))
+	      || value_dependent_expression_p (TREE_OPERAND (expression, 2))
+	      || value_dependent_expression_p (TREE_OPERAND (expression, 3)));
+
     default:
       /* A constant expression is value-dependent if any subexpression is
 	 value-dependent.  */
diff --git a/gcc/testsuite/g++.dg/template/new13.C b/gcc/testsuite/g++.dg/template/new13.C
new file mode 100644
index 00000000000..3168374b26d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/new13.C
@@ -0,0 +1,11 @@
+// PR c++/105803
+// { dg-do compile }
+// { dg-additional-options "-fchecking=2" }
+
+namespace std {
+template <typename> class shared_ptr;
+}
+struct S {};
+template <int> void build_matrices() {
+  std::shared_ptr<S>(new S);
+}

base-commit: 2d546ff69455f7deadab65309de89d19380a8864
-- 
2.36.1

Re: [PATCH] c++: ICE with template NEW_EXPR [PR105803]

[Patrick Palka]

On Wed, 1 Jun 2022, Marek Polacek via Gcc-patches wrote:

> Here we ICE because value_dependent_expression_p gets a NEW_EXPR
> whose operand is a type, and we go to the default case which just
> calls v_d_e_p on each operand of the NEW_EXPR.  Since one of them
> is a type, we crash on the new assert in t_d_e_p.

Looks like NEW_EXPR is considered to be not potentially constant
according to potential_constant_expression.  I thought we shouldn't
be calling value_dependent_expression_p on such exprs?

> 
> t_d_e_p has code to handle {,VEC_}NEW_EXPR, which at this point
> was already performed, so I think we can handle these two codes
> specifically and skip the second operand, which is always going
> to be a type.
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> 	PR c++/105803
> 
> gcc/cp/ChangeLog:
> 
> 	* pt.cc (value_dependent_expression_p): Handle {,VEC_}NEW_EXPR
> 	in the switch.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/template/new13.C: New test.
> ---
>  gcc/cp/pt.cc                          |  8 ++++++++
>  gcc/testsuite/g++.dg/template/new13.C | 11 +++++++++++
>  2 files changed, 19 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/template/new13.C
> 
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index 6de8e496859..836861e1039 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -27643,6 +27643,14 @@ value_dependent_expression_p (tree expression)
>  	 under instantiate_non_dependent_expr; it can't be constant.  */
>        return true;
>  
> +    case NEW_EXPR:
> +    case VEC_NEW_EXPR:
> +      /* The second operand is a type, which type_dependent_expression_p
> +	 (and therefore value_dependent_expression_p) doesn't want to see.  */
> +      return (value_dependent_expression_p (TREE_OPERAND (expression, 0))
> +	      || value_dependent_expression_p (TREE_OPERAND (expression, 2))
> +	      || value_dependent_expression_p (TREE_OPERAND (expression, 3)));
> +
>      default:
>        /* A constant expression is value-dependent if any subexpression is
>  	 value-dependent.  */
> diff --git a/gcc/testsuite/g++.dg/template/new13.C b/gcc/testsuite/g++.dg/template/new13.C
> new file mode 100644
> index 00000000000..3168374b26d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/new13.C
> @@ -0,0 +1,11 @@
> +// PR c++/105803
> +// { dg-do compile }
> +// { dg-additional-options "-fchecking=2" }
> +
> +namespace std {
> +template <typename> class shared_ptr;
> +}
> +struct S {};
> +template <int> void build_matrices() {
> +  std::shared_ptr<S>(new S);
> +}

I think this testcase might be IFNDR since shared_ptr<S> is incomplete
at the point of its non-dependent use.

> 
> base-commit: 2d546ff69455f7deadab65309de89d19380a8864
> -- 
> 2.36.1
> 
> 

Re: [PATCH] c++: ICE with template NEW_EXPR [PR105803]

[Marek Polacek]

On Thu, Jun 02, 2022 at 08:42:36AM -0400, Patrick Palka wrote:
> On Wed, 1 Jun 2022, Marek Polacek via Gcc-patches wrote:
> 
> > Here we ICE because value_dependent_expression_p gets a NEW_EXPR
> > whose operand is a type, and we go to the default case which just
> > calls v_d_e_p on each operand of the NEW_EXPR.  Since one of them
> > is a type, we crash on the new assert in t_d_e_p.
> 
> Looks like NEW_EXPR is considered to be not potentially constant
> according to potential_constant_expression.  I thought we shouldn't
> be calling value_dependent_expression_p on such exprs?

You're correct.  This is non-obvious: instantiation_dependent_expression_p
calls p_c_e before v_d_e_p, but the expression is CAST_EXPR<[NEW_EXPR]>,
where the [] denotes a TREE_LIST, created in cp_parser_functional_cast.
This TREE_LIST has no type.  So p_c_e_1/CAST_EXPR goes to 
9183           /* If this is a dependent type, it could end up being a class
9184              with conversions.  */
9185           if (type == NULL_TREE || WILDCARD_TYPE_P (type))
9186             return true;
and returns true.

So we call v_d_e_p, which looks at the CAST_EXPR's op and sees a TREE_LIST,
so it calls any_value_dependent_elements_p, and we end up with a NEW_EXPR.

An alternative/more proper fix would be to fix p_c_e_1/CAST_EXPR.  Maybe
by calling any_type_dependent_elements_p (which currently has no uses).
Thoughts?

> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/template/new13.C
> > @@ -0,0 +1,11 @@
> > +// PR c++/105803
> > +// { dg-do compile }
> > +// { dg-additional-options "-fchecking=2" }
> > +
> > +namespace std {
> > +template <typename> class shared_ptr;
> > +}
> > +struct S {};
> > +template <int> void build_matrices() {
> > +  std::shared_ptr<S>(new S);
> > +}
> 
> I think this testcase might be IFNDR since shared_ptr<S> is incomplete
> at the point of its non-dependent use.

Ah, overreduced.  I've made shared_ptr complete.

Marek

Re: [PATCH] c++: ICE with template NEW_EXPR [PR105803]

[Jason Merrill]

On 6/2/22 10:03, Marek Polacek wrote:
> On Thu, Jun 02, 2022 at 08:42:36AM -0400, Patrick Palka wrote:
>> On Wed, 1 Jun 2022, Marek Polacek via Gcc-patches wrote:
>>
>>> Here we ICE because value_dependent_expression_p gets a NEW_EXPR
>>> whose operand is a type, and we go to the default case which just
>>> calls v_d_e_p on each operand of the NEW_EXPR.  Since one of them
>>> is a type, we crash on the new assert in t_d_e_p.
>>
>> Looks like NEW_EXPR is considered to be not potentially constant
>> according to potential_constant_expression.  I thought we shouldn't
>> be calling value_dependent_expression_p on such exprs?

Except in C++20 new-expressions are potentially constant, so the patch 
is OK, and we should adjust pce1 accordingly.

I notice we currently fail to handle

struct A
{
   int i;
   constexpr A(int *p): i(*p) { delete p; }
};

constexpr int i = A(new int(42)).i;

though it does work inside a function.

> You're correct.  This is non-obvious: instantiation_dependent_expression_p
> calls p_c_e before v_d_e_p, but the expression is CAST_EXPR<[NEW_EXPR]>,
> where the [] denotes a TREE_LIST, created in cp_parser_functional_cast.
> This TREE_LIST has no type.  So p_c_e_1/CAST_EXPR goes to
> 9183           /* If this is a dependent type, it could end up being a class
> 9184              with conversions.  */
> 9185           if (type == NULL_TREE || WILDCARD_TYPE_P (type))
> 9186             return true;
> and returns true.
> 
> So we call v_d_e_p, which looks at the CAST_EXPR's op and sees a TREE_LIST,
> so it calls any_value_dependent_elements_p, and we end up with a NEW_EXPR.
> 
> An alternative/more proper fix would be to fix p_c_e_1/CAST_EXPR.  Maybe
> by calling any_type_dependent_elements_p (which currently has no uses).
> Thoughts?
> 
>>> --- /dev/null
>>> +++ b/gcc/testsuite/g++.dg/template/new13.C
>>> @@ -0,0 +1,11 @@
>>> +// PR c++/105803
>>> +// { dg-do compile }
>>> +// { dg-additional-options "-fchecking=2" }
>>> +
>>> +namespace std {
>>> +template <typename> class shared_ptr;
>>> +}
>>> +struct S {};
>>> +template <int> void build_matrices() {
>>> +  std::shared_ptr<S>(new S);
>>> +}
>>
>> I think this testcase might be IFNDR since shared_ptr<S> is incomplete
>> at the point of its non-dependent use.
> 
> Ah, overreduced.  I've made shared_ptr complete.
> 
> Marek
> 

Re: [PATCH] c++: ICE with template NEW_EXPR [PR105803]

[Marek Polacek]

On Thu, Jun 02, 2022 at 03:42:15PM -0400, Jason Merrill wrote:
> On 6/2/22 10:03, Marek Polacek wrote:
> > On Thu, Jun 02, 2022 at 08:42:36AM -0400, Patrick Palka wrote:
> > > On Wed, 1 Jun 2022, Marek Polacek via Gcc-patches wrote:
> > > 
> > > > Here we ICE because value_dependent_expression_p gets a NEW_EXPR
> > > > whose operand is a type, and we go to the default case which just
> > > > calls v_d_e_p on each operand of the NEW_EXPR.  Since one of them
> > > > is a type, we crash on the new assert in t_d_e_p.
> > > 
> > > Looks like NEW_EXPR is considered to be not potentially constant
> > > according to potential_constant_expression.  I thought we shouldn't
> > > be calling value_dependent_expression_p on such exprs?
> 
> Except in C++20 new-expressions are potentially constant, so the patch is

Thanks, pushed.

> OK, and we should adjust pce1 accordingly.

Is the attached patch OK then?  So far dg.exp passed.  Though it won't help
with...
 
> I notice we currently fail to handle
> 
> struct A
> {
>   int i;
>   constexpr A(int *p): i(*p) { delete p; }
> };
> 
> constexpr int i = A(new int(42)).i;
> 
> though it does work inside a function.

...this test (it complains about a TARGET_EXPR's slot variable not being
declared constexpr), so I'm going to open a PR.

From cf70354894bc31cc542ed8df40633bea2427fee7 Mon Sep 17 00:00:00 2001
From: Marek Polacek <polacek@redhat.com>
Date: Thu, 2 Jun 2022 15:56:18 -0400
Subject: [PATCH] c++: new-expression is potentially constant in C++20

... so adjust p_c_e accordingly.

gcc/cp/ChangeLog:

	* constexpr.cc (potential_constant_expression_1): Treat
	{,VEC_}NEW_EXPR as potentially constant in C++20.
---
 gcc/cp/constexpr.cc | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
index 1346a1d4c10..2bbd8785627 100644
--- a/gcc/cp/constexpr.cc
+++ b/gcc/cp/constexpr.cc
@@ -9039,10 +9039,18 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
 		  "before C++17");
       return false;
 
-    case DYNAMIC_CAST_EXPR:
-    case PSEUDO_DTOR_EXPR:
     case NEW_EXPR:
     case VEC_NEW_EXPR:
+      if (cxx_dialect >= cxx20)
+	/* In C++20, new-expressions are potentially constant.  */
+	return true;
+      else if (flags & tf_error)
+	error_at (loc, "new-expression is not a constant expression "
+		  "before C++20");
+      return false;
+
+    case DYNAMIC_CAST_EXPR:
+    case PSEUDO_DTOR_EXPR:
     case DELETE_EXPR:
     case VEC_DELETE_EXPR:
     case THROW_EXPR:

base-commit: 7b98910406b5000a6429c188b0c6cc14e3140637
-- 
2.36.1

Re: [PATCH] c++: ICE with template NEW_EXPR [PR105803]

[Jason Merrill]

On 6/2/22 16:10, Marek Polacek wrote:
> On Thu, Jun 02, 2022 at 03:42:15PM -0400, Jason Merrill wrote:
>> On 6/2/22 10:03, Marek Polacek wrote:
>>> On Thu, Jun 02, 2022 at 08:42:36AM -0400, Patrick Palka wrote:
>>>> On Wed, 1 Jun 2022, Marek Polacek via Gcc-patches wrote:
>>>>
>>>>> Here we ICE because value_dependent_expression_p gets a NEW_EXPR
>>>>> whose operand is a type, and we go to the default case which just
>>>>> calls v_d_e_p on each operand of the NEW_EXPR.  Since one of them
>>>>> is a type, we crash on the new assert in t_d_e_p.
>>>>
>>>> Looks like NEW_EXPR is considered to be not potentially constant
>>>> according to potential_constant_expression.  I thought we shouldn't
>>>> be calling value_dependent_expression_p on such exprs?
>>
>> Except in C++20 new-expressions are potentially constant, so the patch is
> 
> Thanks, pushed.
> 
>> OK, and we should adjust pce1 accordingly.
> 
> Is the attached patch OK then?  So far dg.exp passed.  Though it won't help
> with...
>   
>> I notice we currently fail to handle
>>
>> struct A
>> {
>>    int i;
>>    constexpr A(int *p): i(*p) { delete p; }
>> };
>>
>> constexpr int i = A(new int(42)).i;
>>
>> though it does work inside a function.
> 
> ...this test (it complains about a TARGET_EXPR's slot variable not being
> declared constexpr), so I'm going to open a PR.
> 
>  From cf70354894bc31cc542ed8df40633bea2427fee7 Mon Sep 17 00:00:00 2001
> From: Marek Polacek <polacek@redhat.com>
> Date: Thu, 2 Jun 2022 15:56:18 -0400
> Subject: [PATCH] c++: new-expression is potentially constant in C++20
> 
> ... so adjust p_c_e accordingly.
> 
> gcc/cp/ChangeLog:
> 
> 	* constexpr.cc (potential_constant_expression_1): Treat
> 	{,VEC_}NEW_EXPR as potentially constant in C++20.
> ---
>   gcc/cp/constexpr.cc | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> index 1346a1d4c10..2bbd8785627 100644
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> @@ -9039,10 +9039,18 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
>   		  "before C++17");
>         return false;
>   
> -    case DYNAMIC_CAST_EXPR:
> -    case PSEUDO_DTOR_EXPR:
>       case NEW_EXPR:
>       case VEC_NEW_EXPR:
> +      if (cxx_dialect >= cxx20)
> +	/* In C++20, new-expressions are potentially constant.  */
> +	return true;
> +      else if (flags & tf_error)
> +	error_at (loc, "new-expression is not a constant expression "
> +		  "before C++20");
> +      return false;
> +
> +    case DYNAMIC_CAST_EXPR:
> +    case PSEUDO_DTOR_EXPR:
>       case DELETE_EXPR:
>       case VEC_DELETE_EXPR:

Delete, too.

>       case THROW_EXPR:
> 
> base-commit: 7b98910406b5000a6429c188b0c6cc14e3140637

Re: [PATCH] c++: ICE with template NEW_EXPR [PR105803]

[Marek Polacek]

On Thu, Jun 02, 2022 at 04:26:27PM -0400, Jason Merrill wrote:
> On 6/2/22 16:10, Marek Polacek wrote:
> > index 1346a1d4c10..2bbd8785627 100644
> > --- a/gcc/cp/constexpr.cc
> > +++ b/gcc/cp/constexpr.cc
> > @@ -9039,10 +9039,18 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
> >   		  "before C++17");
> >         return false;
> > -    case DYNAMIC_CAST_EXPR:
> > -    case PSEUDO_DTOR_EXPR:
> >       case NEW_EXPR:
> >       case VEC_NEW_EXPR:
> > +      if (cxx_dialect >= cxx20)
> > +	/* In C++20, new-expressions are potentially constant.  */
> > +	return true;
> > +      else if (flags & tf_error)
> > +	error_at (loc, "new-expression is not a constant expression "
> > +		  "before C++20");
> > +      return false;
> > +
> > +    case DYNAMIC_CAST_EXPR:
> > +    case PSEUDO_DTOR_EXPR:
> >       case DELETE_EXPR:
> >       case VEC_DELETE_EXPR:
> 
> Delete, too.

Duh.  Fixed:

From 2423f6548405185e256036df3d0ef3c13fd996c5 Mon Sep 17 00:00:00 2001
From: Marek Polacek <polacek@redhat.com>
Date: Thu, 2 Jun 2022 15:56:18 -0400
Subject: [PATCH] c++: new-expression is potentially constant in C++20

... so adjust p_c_e accordingly.

gcc/cp/ChangeLog:

	* constexpr.cc (potential_constant_expression_1): Treat
	{,VEC_}NEW_EXPR and {,VEC_}DELETE_EXPRas potentially constant in C++20.
---
 gcc/cp/constexpr.cc | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
index 1346a1d4c10..684238883dc 100644
--- a/gcc/cp/constexpr.cc
+++ b/gcc/cp/constexpr.cc
@@ -9039,12 +9039,20 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
 		  "before C++17");
       return false;
 
-    case DYNAMIC_CAST_EXPR:
-    case PSEUDO_DTOR_EXPR:
     case NEW_EXPR:
     case VEC_NEW_EXPR:
     case DELETE_EXPR:
     case VEC_DELETE_EXPR:
+      if (cxx_dialect >= cxx20)
+	/* In C++20, new-expressions are potentially constant.  */
+	return true;
+      else if (flags & tf_error)
+	error_at (loc, "new-expression is not a constant expression "
+		  "before C++20");
+      return false;
+
+    case DYNAMIC_CAST_EXPR:
+    case PSEUDO_DTOR_EXPR:
     case THROW_EXPR:
     case OMP_PARALLEL:
     case OMP_TASK:

base-commit: 7b98910406b5000a6429c188b0c6cc14e3140637
-- 
2.36.1

Re: [PATCH] c++: ICE with template NEW_EXPR [PR105803]

[Jason Merrill]

On 6/2/22 16:33, Marek Polacek wrote:
> On Thu, Jun 02, 2022 at 04:26:27PM -0400, Jason Merrill wrote:
>> On 6/2/22 16:10, Marek Polacek wrote:
>>> index 1346a1d4c10..2bbd8785627 100644
>>> --- a/gcc/cp/constexpr.cc
>>> +++ b/gcc/cp/constexpr.cc
>>> @@ -9039,10 +9039,18 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
>>>    		  "before C++17");
>>>          return false;
>>> -    case DYNAMIC_CAST_EXPR:
>>> -    case PSEUDO_DTOR_EXPR:
>>>        case NEW_EXPR:
>>>        case VEC_NEW_EXPR:
>>> +      if (cxx_dialect >= cxx20)
>>> +	/* In C++20, new-expressions are potentially constant.  */
>>> +	return true;
>>> +      else if (flags & tf_error)
>>> +	error_at (loc, "new-expression is not a constant expression "
>>> +		  "before C++20");
>>> +      return false;
>>> +
>>> +    case DYNAMIC_CAST_EXPR:
>>> +    case PSEUDO_DTOR_EXPR:
>>>        case DELETE_EXPR:
>>>        case VEC_DELETE_EXPR:
>>
>> Delete, too.
> 
> Duh.  Fixed:

OK, thanks.

>  From 2423f6548405185e256036df3d0ef3c13fd996c5 Mon Sep 17 00:00:00 2001
> From: Marek Polacek <polacek@redhat.com>
> Date: Thu, 2 Jun 2022 15:56:18 -0400
> Subject: [PATCH] c++: new-expression is potentially constant in C++20
> 
> ... so adjust p_c_e accordingly.
> 
> gcc/cp/ChangeLog:
> 
> 	* constexpr.cc (potential_constant_expression_1): Treat
> 	{,VEC_}NEW_EXPR and {,VEC_}DELETE_EXPRas potentially constant in C++20.
> ---
>   gcc/cp/constexpr.cc | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> index 1346a1d4c10..684238883dc 100644
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> @@ -9039,12 +9039,20 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
>   		  "before C++17");
>         return false;
>   
> -    case DYNAMIC_CAST_EXPR:
> -    case PSEUDO_DTOR_EXPR:
>       case NEW_EXPR:
>       case VEC_NEW_EXPR:
>       case DELETE_EXPR:
>       case VEC_DELETE_EXPR:
> +      if (cxx_dialect >= cxx20)
> +	/* In C++20, new-expressions are potentially constant.  */
> +	return true;
> +      else if (flags & tf_error)
> +	error_at (loc, "new-expression is not a constant expression "
> +		  "before C++20");
> +      return false;
> +
> +    case DYNAMIC_CAST_EXPR:
> +    case PSEUDO_DTOR_EXPR:
>       case THROW_EXPR:
>       case OMP_PARALLEL:
>       case OMP_TASK:
> 
> base-commit: 7b98910406b5000a6429c188b0c6cc14e3140637