[PATCH v2] c++: Fix cp_tree_equal for template value args using dependent sizeof/alignof/noexcept expressions

[PATCH v2] c++: Fix cp_tree_equal for template value args using dependent sizeof/alignof/noexcept expressions

[Barrett Adair]

[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]

Thanks for the feedback, Jason. Coming back to this today, The problem
appears much deeper than I realized. I've attached another WIP version of
the patch, including a couple of new test cases based on your feedback (for
now, please excuse any misformatted dg-error comments).

The dependent-name16.C case demonstrates an error message regression caused
by this patch; I'm trying to fix the regression. When compiling
dependent-name16.C and breaking at cp/tree.c:3922, DECL_CONTEXT(t2) is NULL
because the "g" declaration is still being parsed near the top of
cp_parser_init_declarator, and context is only set later during that
function. DECL_CONTEXT(t1), on the other hand, is set because the "f"
declaration was already parsed.

I'm beginning to believe that a proper solution to this problem would
require decorating the function template type parameter nodes with more
function information (e.g. at least scope and name) prior to parsing the
trailing return type, if not somehow setting the DECL_CONTEXT earlier in
some form -- am I missing something?

Also, in the first place, I'm a little confused why we insert dependent-arg
instantiations into the specialization/instantiation hash tables before any
top-level instantiation occurs. From a bird's eye view, the
benefit/necessity of this design is unclear. Can anyone point me to some
background reading here?

Thanks,
Barrett

[-- Attachment #2: 0001-Fix-cp_tree_equal-for-template-value-args-using.patch --]
[-- Type: text/x-patch, Size: 5845 bytes --]

From 90baf57f388a0aed45f72cac5b1df920ec61b6ab Mon Sep 17 00:00:00 2001
From: Barrett Adair <barrettellisadair@gmail.com>
Date: Fri, 20 Aug 2021 15:37:36 -0500
Subject: [PATCH] Fix cp_tree_equal for template value args using dependent
 sizeof/alignof/noexcept expressions

---
 gcc/cp/tree.c                                  | 12 ++++++++++++
 gcc/testsuite/g++.dg/template/canon-type-15.C  |  7 +++++++
 gcc/testsuite/g++.dg/template/canon-type-16.C  |  6 ++++++
 gcc/testsuite/g++.dg/template/canon-type-17.C  |  5 +++++
 gcc/testsuite/g++.dg/template/canon-type-18.C  |  6 ++++++
 .../g++.dg/template/dependent-name15.C         | 18 ++++++++++++++++++
 .../g++.dg/template/dependent-name16.C         | 16 ++++++++++++++++
 7 files changed, 70 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/template/canon-type-15.C
 create mode 100644 gcc/testsuite/g++.dg/template/canon-type-16.C
 create mode 100644 gcc/testsuite/g++.dg/template/canon-type-17.C
 create mode 100644 gcc/testsuite/g++.dg/template/canon-type-18.C
 create mode 100644 gcc/testsuite/g++.dg/template/dependent-name15.C
 create mode 100644 gcc/testsuite/g++.dg/template/dependent-name16.C

diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index 3c62dd74380..72e616a4eb7 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -3920,6 +3920,8 @@ cp_tree_equal (tree t1, tree t2)
 	 template.  */
 
       if (comparing_specializations
+          && DECL_CONTEXT (t1)
+          && DECL_CONTEXT (t2)
 	  && DECL_CONTEXT (t1) != DECL_CONTEXT (t2))
 	/* When comparing hash table entries, only an exact match is
 	   good enough; we don't want to replace 'this' with the
@@ -4015,6 +4017,16 @@ cp_tree_equal (tree t1, tree t2)
 	else
 	  return cp_tree_equal (o1, o2);
       }
+    case NOEXCEPT_EXPR:
+      {
+	tree o1 = TREE_OPERAND (t1, 0);
+	tree o2 = TREE_OPERAND (t2, 0);
+
+	if (TREE_CODE (o1) != TREE_CODE (o2))
+	  return false;
+
+	return cp_tree_equal (o1, o2);
+      }
 
     case MODOP_EXPR:
       {
diff --git a/gcc/testsuite/g++.dg/template/canon-type-15.C b/gcc/testsuite/g++.dg/template/canon-type-15.C
new file mode 100644
index 00000000000..b001b5c841d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/canon-type-15.C
@@ -0,0 +1,7 @@
+// { dg-do compile { target c++11 } }
+template<unsigned u> struct size_c{ static constexpr unsigned value = u; };
+namespace g {
+template<class T> auto return_size(T t) -> size_c<sizeof(t)>;
+template<class T> auto return_size(T t) -> size_c<sizeof(t)>;
+}
+static_assert(decltype(g::return_size('a'))::value == 1u, "");
diff --git a/gcc/testsuite/g++.dg/template/canon-type-16.C b/gcc/testsuite/g++.dg/template/canon-type-16.C
new file mode 100644
index 00000000000..99361cbac30
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/canon-type-16.C
@@ -0,0 +1,6 @@
+// { dg-do compile { target c++11 } }
+template<bool u> struct bool_c{ static constexpr bool value = u; };
+template<class T> auto noexcepty(T t) -> bool_c<noexcept(t())>;
+template<class T> auto noexcepty(T t) -> bool_c<noexcept(t())>;
+struct foo { void operator()() noexcept; };
+static_assert(decltype(noexcepty(foo{}))::value, "");
diff --git a/gcc/testsuite/g++.dg/template/canon-type-17.C b/gcc/testsuite/g++.dg/template/canon-type-17.C
new file mode 100644
index 00000000000..0555c8d0a42
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/canon-type-17.C
@@ -0,0 +1,5 @@
+// { dg-do compile { target c++11 } }
+template<unsigned u> struct size_c{ static constexpr unsigned value = u; };
+template<class... T> auto return_size(T... t) -> size_c<sizeof...(t)>;
+template<class... T> auto return_size(T... t) -> size_c<sizeof...(t)>;
+static_assert(decltype(return_size('a'))::value == 1u, "");
diff --git a/gcc/testsuite/g++.dg/template/canon-type-18.C b/gcc/testsuite/g++.dg/template/canon-type-18.C
new file mode 100644
index 00000000000..2510181725c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/canon-type-18.C
@@ -0,0 +1,6 @@
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wno-pedantic" }
+template<unsigned u> struct size_c{ static constexpr unsigned value = u; };
+template<class T> auto get_align(T t) -> size_c<alignof(t)>;
+template<class T> auto get_align(T t) -> size_c<alignof(t)>;
+static_assert(decltype(get_align('a'))::value == 1u, "");
diff --git a/gcc/testsuite/g++.dg/template/dependent-name15.C b/gcc/testsuite/g++.dg/template/dependent-name15.C
new file mode 100644
index 00000000000..cb9f5a56ec2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/dependent-name15.C
@@ -0,0 +1,18 @@
+// { dg-do compile { target c++11 } }
+template <int N> struct A { static void foo(){} };
+template <> struct A<sizeof(char)> { using foo = int; };
+
+template <class T> void f(T t1) { 
+    A<sizeof(t1)>::foo();
+}
+
+template <class T> void g(T t2) { 
+    /* if the comparing_specializations check breaks in cp_tree_equal
+    case PARM_DECL, the error will incorrectly report A<sizeof (t1)> */
+    A<sizeof(t2)>::foo(); // { dg-error "name ‘A<sizeof (t2)>::foo’" }
+}
+
+void h() {
+    f(0);
+    g('0');
+}
diff --git a/gcc/testsuite/g++.dg/template/dependent-name16.C b/gcc/testsuite/g++.dg/template/dependent-name16.C
new file mode 100644
index 00000000000..b7661db2f7a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/dependent-name16.C
@@ -0,0 +1,16 @@
+// { dg-do compile { target c++11 } }
+template <int N> struct A { static void foo(){} };
+template <> struct A<sizeof(char)> { using foo = int; };
+
+template<class T1> auto f(T1 t1) -> decltype(A<sizeof(t1)>::foo());
+
+/* if the comparing_specializations check breaks in cp_tree_equal
+case PARM_DECL, the error will incorrectly report A<sizeof (t1)> */
+// { dg-error "name ‘A<sizeof (t2)>::foo’" }
+template<class T2> auto g(T2 t2) -> decltype(A<sizeof(t2)>::foo());
+
+void h() {
+    f(0);
+// { dg-error "no matching function" }
+    g('0');
+}
-- 
2.30.2

Re: [PATCH v2] c++: Fix cp_tree_equal for template value args using dependent sizeof/alignof/noexcept expressions

[Jason Merrill]

On 9/2/21 8:10 PM, Barrett Adair wrote:
> Thanks for the feedback, Jason. Coming back to this today, The problem 
> appears much deeper than I realized. I've attached another WIP version 
> of the patch, including a couple of new test cases based on your 
> feedback (for now, please excuse any misformatted dg-error comments).
> 
> The dependent-name16.C case demonstrates an error message regression 
> caused by this patch; I'm trying to fix the regression. When compiling 
> dependent-name16.C and breaking at cp/tree.c:3922, DECL_CONTEXT(t2) is 
> NULL because the "g" declaration is still being parsed near the top of 
> cp_parser_init_declarator, and context is only set later during that 
> function. DECL_CONTEXT(t1), on the other hand, is set because the "f" 
> declaration was already parsed.
> 
> I'm beginning to believe that a proper solution to this problem would 
> require decorating the function template type parameter nodes with more 
> function information (e.g. at least scope and name) prior to parsing the 
> trailing return type, if not somehow setting the DECL_CONTEXT earlier in 
> some form -- am I missing something?

Perhaps comparing DECL_SOURCE_LOCATION would be useful?

But actually, I suspect you're approaching this the wrong way: the 
problem in canon-type-15.C is "internal compiler error: canonical types 
differ for identical types ‘size_c<sizeof (t)>’ and ‘size_c<sizeof (t)>’".

The intent is that template arguments involving with distinct but 
equivalent arguments should themselves be distinct but equivalent: 
different tree nodes, but the same TYPE_CANONICAL.

That isn't happening here: they're different tree nodes and have 
different TYPE_CANONICAL, so they're considered non-equivalent, but 
structurally compare as equivalent, so we get the ICE above.  We need to 
fix TYPE_CANONICAL to match.

I think a straightforward approach would be to make 
any_template_arguments_need_structural_equality_p return true if one of 
the template arguments involves a function parameter, so that 
TYPE_CANONICAL gets set to 0 and we always do structural comparison in 
that context.

> Also, in the first place, I'm a little confused why we insert 
> dependent-arg instantiations into the specialization/instantiation hash 
> tables before any top-level instantiation occurs. From a bird's eye 
> view, the benefit/necessity of this design is unclear. Can anyone point 
> me to some background reading here?

So that whenever we write e.g. A<T> we get the same type node.

Jason

Re: [PATCH v2] c++: Fix cp_tree_equal for template value args using dependent sizeof/alignof/noexcept expressions

[Jason Merrill]

By the way, please avoid using non-ASCII characters in testcases unless 
that's specifically what you're testing:

> +// { dg-error "name ‘A<sizeof (t2)>::foo’" }

Here, change the smart quotes to . to match whatever the output locale 
uses for quotes.  And the error comments in dependent-name16.C seem to 
be on the wrong lines.

Jason

Re: [PATCH v2] c++: Fix cp_tree_equal for template value args using dependent sizeof/alignof/noexcept expressions

[Jakub Jelinek]

On Tue, Sep 07, 2021 at 04:01:41PM -0400, Jason Merrill via Gcc-patches wrote:
> By the way, please avoid using non-ASCII characters in testcases unless
> that's specifically what you're testing:
> 
> > +// { dg-error "name ‘A<sizeof (t2)>::foo’" }
> 
> Here, change the smart quotes to . to match whatever the output locale uses
> for quotes.  And the error comments in dependent-name16.C seem to be on the
> wrong lines.

In the testsuite only 's will match, so it is safe to use those, but . is
fine too.
And the ()s probably need some backslashes, I think 3.

	Jakub