[PATCH] c++: bad ggc_free in try_class_unification [PR109556]

[PATCH] c++: bad ggc_free in try_class_unification [PR109556]

[Patrick Palka]

Aside from correcting how try_class_unification copies multi-dimensional
'targs', r13-377-g3e948d645bc908 also made it ggc_free this copy as an
optimization.  But this is potentially wrong since the call to unify
within might've captured the args in persistent memory such as the
satisfaction cache (during constrained auto deduction).

Bootstrapped and regtested on x86_64-pc-linux, does this look OK for
trunk/13?  No testcase yet since the reduction is still in progress.
The plan would be to push this with a reduced testcase, but I figured
I'd send the actual fix for review now.  Would this be OK for 13.1 or
shall it wait until 13.2?

gcc/cp/ChangeLog:

	* pt.cc (try_class_unification): Don't ggc_free the copy of
	'targs'.
---
 gcc/cp/pt.cc | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index e065ace5c55..68a056acf8b 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -23895,11 +23895,6 @@ try_class_unification (tree tparms, tree targs, tree parm, tree arg,
     err = unify (tparms, targs, CLASSTYPE_TI_ARGS (parm),
 		 CLASSTYPE_TI_ARGS (arg), UNIFY_ALLOW_NONE, explain_p);
 
-  if (TMPL_ARGS_HAVE_MULTIPLE_LEVELS (targs))
-    for (tree level : tree_vec_range (targs))
-      ggc_free (level);
-  ggc_free (targs);
-
   return err ? NULL_TREE : arg;
 }
 
-- 
2.40.0.352.g667fcf4e15

Re: [PATCH] c++: bad ggc_free in try_class_unification [PR109556]

[Patrick Palka]

On Wed, 19 Apr 2023, Patrick Palka wrote:

> Aside from correcting how try_class_unification copies multi-dimensional
> 'targs', r13-377-g3e948d645bc908 also made it ggc_free this copy as an
> optimization.  But this is potentially wrong since the call to unify
> within might've captured the args in persistent memory such as the
> satisfaction cache (during constrained auto deduction).
> 
> Bootstrapped and regtested on x86_64-pc-linux, does this look OK for
> trunk/13?  No testcase yet since the reduction is still in progress.
> The plan would be to push this with a reduced testcase, but I figured
> I'd send the actual fix for review now.  Would this be OK for 13.1 or
> shall it wait until 13.2?

Now with a reduced testcase:

-- >8 --

Subject: [PATCH] c++: bad ggc_free in try_class_unification [PR109556]

Aside from correcting how try_class_unification copies multi-dimensional
'targs', r13-377-g3e948d645bc908 also made it ggc_free this copy as an
optimization.  But this is potentially wrong since the call to unify
within might've captured the args in persistent memory such as the
satisfaction cache (during constrained auto deduction).

gcc/cp/ChangeLog:

	* pt.cc (try_class_unification): Don't ggc_free the copy of
	'targs'.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp2a/concepts-placeholder13.C: New test.
---
 gcc/cp/pt.cc                                     |  5 -----
 .../g++.dg/cpp2a/concepts-placeholder13.C        | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-placeholder13.C

diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index e065ace5c55..68a056acf8b 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -23895,11 +23895,6 @@ try_class_unification (tree tparms, tree targs, tree parm, tree arg,
     err = unify (tparms, targs, CLASSTYPE_TI_ARGS (parm),
 		 CLASSTYPE_TI_ARGS (arg), UNIFY_ALLOW_NONE, explain_p);
 
-  if (TMPL_ARGS_HAVE_MULTIPLE_LEVELS (targs))
-    for (tree level : tree_vec_range (targs))
-      ggc_free (level);
-  ggc_free (targs);
-
   return err ? NULL_TREE : arg;
 }
 
diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder13.C b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder13.C
new file mode 100644
index 00000000000..fd4a05c05e1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder13.C
@@ -0,0 +1,16 @@
+// PR c++/109556
+// { dg-do compile { target c++20 } }
+
+template<typename T, auto N>
+concept C = (N != 0);
+
+template<auto N, auto M>
+struct A { };
+
+template<auto N, C<N> auto M>
+void f(A<N, M>);
+
+int main() {
+  f(A<1, 42>{});
+  f(A<2, 42>{});
+}
-- 
2.40.0.352.g667fcf4e15

Re: [PATCH] c++: bad ggc_free in try_class_unification [PR109556]

[Jason Merrill]

On 4/19/23 12:05, Patrick Palka wrote:
> On Wed, 19 Apr 2023, Patrick Palka wrote:
> 
>> Aside from correcting how try_class_unification copies multi-dimensional
>> 'targs', r13-377-g3e948d645bc908 also made it ggc_free this copy as an
>> optimization.  But this is potentially wrong since the call to unify
>> within might've captured the args in persistent memory such as the
>> satisfaction cache (during constrained auto deduction).
>>
>> Bootstrapped and regtested on x86_64-pc-linux, does this look OK for
>> trunk/13?

OK.

>> No testcase yet since the reduction is still in progress.
>> The plan would be to push this with a reduced testcase, but I figured
>> I'd send the actual fix for review now.  Would this be OK for 13.1 or
>> shall it wait until 13.2?

Jakub's call, but this regression seems like a blocker to me.

> Now with a reduced testcase:
> 
> -- >8 --
> 
> Subject: [PATCH] c++: bad ggc_free in try_class_unification [PR109556]
> 
> Aside from correcting how try_class_unification copies multi-dimensional
> 'targs', r13-377-g3e948d645bc908 also made it ggc_free this copy as an
> optimization.  But this is potentially wrong since the call to unify
> within might've captured the args in persistent memory such as the
> satisfaction cache (during constrained auto deduction).
> 
> gcc/cp/ChangeLog:
> 
> 	* pt.cc (try_class_unification): Don't ggc_free the copy of
> 	'targs'.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp2a/concepts-placeholder13.C: New test.
> ---
>   gcc/cp/pt.cc                                     |  5 -----
>   .../g++.dg/cpp2a/concepts-placeholder13.C        | 16 ++++++++++++++++
>   2 files changed, 16 insertions(+), 5 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-placeholder13.C
> 
> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> index e065ace5c55..68a056acf8b 100644
> --- a/gcc/cp/pt.cc
> +++ b/gcc/cp/pt.cc
> @@ -23895,11 +23895,6 @@ try_class_unification (tree tparms, tree targs, tree parm, tree arg,
>       err = unify (tparms, targs, CLASSTYPE_TI_ARGS (parm),
>   		 CLASSTYPE_TI_ARGS (arg), UNIFY_ALLOW_NONE, explain_p);
>   
> -  if (TMPL_ARGS_HAVE_MULTIPLE_LEVELS (targs))
> -    for (tree level : tree_vec_range (targs))
> -      ggc_free (level);
> -  ggc_free (targs);
> -
>     return err ? NULL_TREE : arg;
>   }
>   
> diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder13.C b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder13.C
> new file mode 100644
> index 00000000000..fd4a05c05e1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder13.C
> @@ -0,0 +1,16 @@
> +// PR c++/109556
> +// { dg-do compile { target c++20 } }
> +
> +template<typename T, auto N>
> +concept C = (N != 0);
> +
> +template<auto N, auto M>
> +struct A { };
> +
> +template<auto N, C<N> auto M>
> +void f(A<N, M>);
> +
> +int main() {
> +  f(A<1, 42>{});
> +  f(A<2, 42>{});
> +}

Re: [PATCH] c++: bad ggc_free in try_class_unification [PR109556]

[Jakub Jelinek]

On Wed, Apr 19, 2023 at 12:52:50PM -0400, Jason Merrill wrote:
> On 4/19/23 12:05, Patrick Palka wrote:
> > On Wed, 19 Apr 2023, Patrick Palka wrote:
> > 
> > > Aside from correcting how try_class_unification copies multi-dimensional
> > > 'targs', r13-377-g3e948d645bc908 also made it ggc_free this copy as an
> > > optimization.  But this is potentially wrong since the call to unify
> > > within might've captured the args in persistent memory such as the
> > > satisfaction cache (during constrained auto deduction).
> > > 
> > > Bootstrapped and regtested on x86_64-pc-linux, does this look OK for
> > > trunk/13?
> 
> OK.
> 
> > > No testcase yet since the reduction is still in progress.
> > > The plan would be to push this with a reduced testcase, but I figured
> > > I'd send the actual fix for review now.  Would this be OK for 13.1 or
> > > shall it wait until 13.2?
> 
> Jakub's call, but this regression seems like a blocker to me.

Not doing ggc_free shouldn't really break stuff except increase memory
consumption, so I think this is ok for 13.1.

> > Now with a reduced testcase:
> > 
> > -- >8 --
> > 
> > Subject: [PATCH] c++: bad ggc_free in try_class_unification [PR109556]
> > 
> > Aside from correcting how try_class_unification copies multi-dimensional
> > 'targs', r13-377-g3e948d645bc908 also made it ggc_free this copy as an
> > optimization.  But this is potentially wrong since the call to unify
> > within might've captured the args in persistent memory such as the
> > satisfaction cache (during constrained auto deduction).
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* pt.cc (try_class_unification): Don't ggc_free the copy of
> > 	'targs'.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/cpp2a/concepts-placeholder13.C: New test.
> > ---
> >   gcc/cp/pt.cc                                     |  5 -----
> >   .../g++.dg/cpp2a/concepts-placeholder13.C        | 16 ++++++++++++++++
> >   2 files changed, 16 insertions(+), 5 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/cpp2a/concepts-placeholder13.C
> > 
> > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > index e065ace5c55..68a056acf8b 100644
> > --- a/gcc/cp/pt.cc
> > +++ b/gcc/cp/pt.cc
> > @@ -23895,11 +23895,6 @@ try_class_unification (tree tparms, tree targs, tree parm, tree arg,
> >       err = unify (tparms, targs, CLASSTYPE_TI_ARGS (parm),
> >   		 CLASSTYPE_TI_ARGS (arg), UNIFY_ALLOW_NONE, explain_p);
> > -  if (TMPL_ARGS_HAVE_MULTIPLE_LEVELS (targs))
> > -    for (tree level : tree_vec_range (targs))
> > -      ggc_free (level);
> > -  ggc_free (targs);
> > -
> >     return err ? NULL_TREE : arg;
> >   }
> > diff --git a/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder13.C b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder13.C
> > new file mode 100644
> > index 00000000000..fd4a05c05e1
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/cpp2a/concepts-placeholder13.C
> > @@ -0,0 +1,16 @@
> > +// PR c++/109556
> > +// { dg-do compile { target c++20 } }
> > +
> > +template<typename T, auto N>
> > +concept C = (N != 0);
> > +
> > +template<auto N, auto M>
> > +struct A { };
> > +
> > +template<auto N, C<N> auto M>
> > +void f(A<N, M>);
> > +
> > +int main() {
> > +  f(A<1, 42>{});
> > +  f(A<2, 42>{});
> > +}

	Jakub