Re: [PATCH] c++: canonicity of fn types w/ complex eh specs [PR115159]

[Patrick Palka <ppalka@redhat.com>]

On Tue, 21 May 2024, Jason Merrill wrote:

> On 5/21/24 17:27, Patrick Palka wrote:
> > On Tue, 21 May 2024, Jason Merrill wrote:
> > 
> > > On 5/21/24 15:36, Patrick Palka wrote:
> > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
> > > > OK for trunk?
> > > > 
> > > > Alternatively, I considered fixing this by incrementing
> > > > comparing_specializations around the call to comp_except_specs in
> > > > cp_check_qualified_type, but generally for types whose identity
> > > > depends on whether comparing_specializations is set we need to
> > > > use structural equality anyway IIUC.
> > > 
> > > Why not both?
> > 
> > I figured the latter change isn't necessary/observable since
> > comparing_specializations would only make a difference for complex
> > exception specifications, and with this patch we won't even call
> > cp_check_qualified_type on a complex eh spec.
> 
> My concern is that if we're building a function type multiple times with the
> same noexcept-spec, this patch would mean creating multiple equivalent
> function types instead of reusing one already created for the same function.
> 
> > > > +  bool complex_p = (cr && cr != noexcept_true_spec
> > > > +		    && !UNPARSED_NOEXCEPT_SPEC_P (cr));
> > > 
> > > Why treat unparsed specs differently from parsed ones?
> > 
> > Unparsed specs are unique according to cp_tree_equal, so in turn
> > function types with unparsed specs are unique, so it should be safe to
> > treat such types as canonical.  I'm not sure if this optimization
> > matters though; I'm happy to remove this case.
> 
> The idea that this optimization could make a difference raised the concern
> above.

Aha, makes sense.  To that end it seems we could strengthen the ce_exact
in comp_except_specs to require == instead of cp_tree_equal equality
when comparing two noexcept-specs; the only ce_exact callers are
cp_check_qualified_type and cxx_type_hash_eq, which should be fine with
that strengthening.  This way, we at least do try to reuse a variant if
the (complex or unparsed) noexcept-spec is exactly the same.

Like so?

-- >8 --

Subject: [PATCH] c++: canonicity of fn types w/ complex eh specs [PR115159]

Here the member functions QList::g and QList::h are given the same
function type since their exception specifications are equivalent
according to cp_tree_equal.  In doing so however this means that the
type of QList::h refers to a function parameter from QList::g, which
ends up confusing modules streaming.

I'm not sure if modules can be fixed to handle this situation, but
regardless it seems weird in principle that a function parameter can
escape in such a way.  The analogous situation with a trailing return
type and decltype

  auto g(QList &other) -> decltype(f(other));
  auto h(QList &other) -> decltype(f(other));

behaves better because we don't canonicalize decltype, and so the
function types of g and h are non-canonical and therefore not shared.

In light of this, it seems natural to treat function types with complex
eh specs as non-canonical as well so that each such function declaration
is given a unique function/method type node.  The main benefit of type
canonicalization is to speed up repeated type comparisons, but it should
rare for us to repeatedly compare two otherwise compatible function
types with complex exception specifications, so foregoing canonicalization
should not cause any problems.

To that end, this patch strengthens the ce_exact case of comp_except_specs
to require identity instead of equivalence of the exception specification
so that build_cp_fntype_variant doesn't reuse a variant when it shouldn't.
And in build_cp_fntype_variant we need to use structural equality for types
with a complex eh spec.  In turn we could simplify the code responsible
for adjusting unparsed eh spec variants.

	PR c++/115159

gcc/cp/ChangeLog:

	* tree.cc (build_cp_fntype_variant): Always use structural
	equality for types with a complex exception specification.
	(fixup_deferred_exception_variants): Always use structural
	equality for adjusted variants.
	* typeck.cc (comp_except_specs): Require == instead of
	cp_tree_equal for noexcept-spec comparison in the ce_exact case.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/noexcept-2_a.H: New test.
	* g++.dg/modules/noexcept-2_b.C: New test.
---
 gcc/cp/tree.cc                              | 47 +++++----------------
 gcc/cp/typeck.cc                            |  4 +-
 gcc/testsuite/g++.dg/modules/noexcept-2_a.H | 24 +++++++++++
 gcc/testsuite/g++.dg/modules/noexcept-2_b.C |  4 ++
 4 files changed, 41 insertions(+), 38 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/noexcept-2_a.H
 create mode 100644 gcc/testsuite/g++.dg/modules/noexcept-2_b.C

diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
index 9d37d255d8d..93a64322418 100644
--- a/gcc/cp/tree.cc
+++ b/gcc/cp/tree.cc
@@ -2794,7 +2794,12 @@ build_cp_fntype_variant (tree type, cp_ref_qualifier rqual,
   /* Canonicalize the exception specification.  */
   tree cr = flag_noexcept_type ? canonical_eh_spec (raises) : NULL_TREE;
 
-  if (TYPE_STRUCTURAL_EQUALITY_P (type))
+  /* Always use structural equality for function types with a complex
+     exception specification since their identity may depend on e.g.
+     whether comparing_specializations is set.  */
+  bool complex_eh_spec_p = (cr && cr != noexcept_true_spec
+			    && !UNPARSED_NOEXCEPT_SPEC_P (cr));
+  if (TYPE_STRUCTURAL_EQUALITY_P (type) || complex_eh_spec_p)
     /* Propagate structural equality. */
     SET_TYPE_STRUCTURAL_EQUALITY (v);
   else if (TYPE_CANONICAL (type) != type || cr != raises || late)
@@ -2812,55 +2817,23 @@ build_cp_fntype_variant (tree type, cp_ref_qualifier rqual,
 /* TYPE is a function or method type with a deferred exception
    specification that has been parsed to RAISES.  Fixup all the type
    variants that are affected in place.  Via decltype &| noexcept
-   tricks, the unparsed spec could have escaped into the type system.
-   The general case is hard to fixup canonical types for.  */
+   tricks, the unparsed spec could have escaped into the type system.  */
 
 void
 fixup_deferred_exception_variants (tree type, tree raises)
 {
   tree original = TYPE_RAISES_EXCEPTIONS (type);
-  tree cr = flag_noexcept_type ? canonical_eh_spec (raises) : NULL_TREE;
 
   gcc_checking_assert (UNPARSED_NOEXCEPT_SPEC_P (original));
 
-  /* Though sucky, this walk will process the canonical variants
-     first.  */
-  tree prev = NULL_TREE;
   for (tree variant = TYPE_MAIN_VARIANT (type);
-       variant; prev = variant, variant = TYPE_NEXT_VARIANT (variant))
+       variant; variant = TYPE_NEXT_VARIANT (variant))
     if (TYPE_RAISES_EXCEPTIONS (variant) == original)
       {
 	gcc_checking_assert (variant != TYPE_MAIN_VARIANT (type));
 
-	if (!TYPE_STRUCTURAL_EQUALITY_P (variant))
-	  {
-	    cp_cv_quals var_quals = TYPE_QUALS (variant);
-	    cp_ref_qualifier rqual = type_memfn_rqual (variant);
-
-	    /* If VARIANT would become a dup (cp_check_qualified_type-wise)
-	       of an existing variant in the variant list of TYPE after its
-	       exception specification has been parsed, elide it.  Otherwise,
-	       build_cp_fntype_variant could use it, leading to "canonical
-	       types differ for identical types."  */
-	    tree v = TYPE_MAIN_VARIANT (type);
-	    for (; v; v = TYPE_NEXT_VARIANT (v))
-	      if (cp_check_qualified_type (v, variant, var_quals,
-					   rqual, cr, false))
-		{
-		  /* The main variant will not match V, so PREV will never
-		     be null.  */
-		  TYPE_NEXT_VARIANT (prev) = TYPE_NEXT_VARIANT (variant);
-		  break;
-		}
-	    TYPE_RAISES_EXCEPTIONS (variant) = raises;
-
-	    if (!v)
-	      v = build_cp_fntype_variant (TYPE_CANONICAL (variant),
-					   rqual, cr, false);
-	    TYPE_CANONICAL (variant) = TYPE_CANONICAL (v);
-	  }
-	else
-	  TYPE_RAISES_EXCEPTIONS (variant) = raises;
+	SET_TYPE_STRUCTURAL_EQUALITY (variant);
+	TYPE_RAISES_EXCEPTIONS (variant) = raises;
 
 	if (!TYPE_DEPENDENT_P (variant))
 	  /* We no longer know that it's not type-dependent.  */
diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index 75b696e32e0..d535746fd43 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -1227,7 +1227,9 @@ comp_except_specs (const_tree t1, const_tree t2, int exact)
   if ((t1 && TREE_PURPOSE (t1))
       || (t2 && TREE_PURPOSE (t2)))
     return (t1 && t2
-	    && cp_tree_equal (TREE_PURPOSE (t1), TREE_PURPOSE (t2)));
+	    && (exact == ce_exact
+		? TREE_PURPOSE (t1) == TREE_PURPOSE (t2)
+		: cp_tree_equal (TREE_PURPOSE (t1), TREE_PURPOSE (t2))));
 
   if (t1 == NULL_TREE)			   /* T1 is ...  */
     return t2 == NULL_TREE || exact == ce_derived;
diff --git a/gcc/testsuite/g++.dg/modules/noexcept-2_a.H b/gcc/testsuite/g++.dg/modules/noexcept-2_a.H
new file mode 100644
index 00000000000..b7144f42d7e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/noexcept-2_a.H
@@ -0,0 +1,24 @@
+// PR c++/115159
+// { dg-additional-options -fmodule-header }
+// { dg-module-cmi {} }
+
+struct QDebug;
+
+template<class T> void f(T);
+
+template<class T> struct QList {
+  QDebug g(QList &other) noexcept(noexcept(f(other)));
+  QDebug h(QList &other) noexcept(noexcept(f(other)));
+};
+
+struct QObjectData {
+  QList<int> children;
+};
+
+struct QIODevice {
+  QObjectData d_ptr;
+};
+
+struct QDebug {
+  QDebug(QIODevice);
+};
diff --git a/gcc/testsuite/g++.dg/modules/noexcept-2_b.C b/gcc/testsuite/g++.dg/modules/noexcept-2_b.C
new file mode 100644
index 00000000000..d34c63add10
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/noexcept-2_b.C
@@ -0,0 +1,4 @@
+// PR c++/115159
+// { dg-additional-options "-fmodules-ts -fno-module-lazy" }
+
+import "noexcept-2_a.H";
-- 
2.45.1.216.g4365c6fcf9