On Tue, 21 May 2024, Jason Merrill wrote:
> On 5/21/24 21:55, Patrick Palka wrote:
> > 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.
>
> Sounds good.
>
> Given that, we probably still want to move the canonical_eh_spec up in
> build_cp_fntype_variant, and pass that to cp_check_qualified_type?
And compare the canonical spec directly from cp_check_qualified_type
instead of using comp_except_specs? Then IIUC for
void f() throw(int);
void g() throw(char);
we'd give g the same function type as f, which seems wrong?
>
> > 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";
>
>
