| Message ID | 20240521193629.4129787-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: canonicity of fn types w/ complex eh specs [PR115159] | expand |
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? > + bool complex_p = (cr && cr != noexcept_true_spec > + && !UNPARSED_NOEXCEPT_SPEC_P (cr)); Why treat unparsed specs differently from parsed ones? Jason
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. > > > + 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. > > Jason > >
On Tue, 21 May 2024, 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. > > > > > > + 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. FWIW if we do get rid of this case then I think in fixup_deferred_exception_variants we can assert TYPE_STRUCTURAL_EQUALITY_P is already set instead of having to set it. > > > > > Jason > > > > >
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. Jason
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";
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? > 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";
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"; > >
On 5/22/24 09:01, Patrick Palka wrote: > 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? Good point, I was confused about what canonical_eh_spec was doing. Your last patch is OK. Jason
diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc index 9d37d255d8d..7987c01520d 100644 --- a/gcc/cp/tree.cc +++ b/gcc/cp/tree.cc @@ -2761,16 +2761,27 @@ build_cp_fntype_variant (tree type, cp_ref_qualifier rqual, { cp_cv_quals type_quals = TYPE_QUALS (type); - if (cp_check_qualified_type (type, type, type_quals, rqual, raises, late)) - return type; + /* Canonicalize the exception specification. */ + tree cr = flag_noexcept_type ? canonical_eh_spec (raises) : NULL_TREE; + /* For a complex exception specification, always create a distinct + non-canonical variant for simplicity. This also prevents noexcept-specs + that are in terms of a function parameter from getting shared with an + another function. */ + bool complex_p = (cr && cr != noexcept_true_spec + && !UNPARSED_NOEXCEPT_SPEC_P (cr)); + if (!complex_p) + { + if (cp_check_qualified_type (type, type, type_quals, rqual, raises, late)) + return type; - tree v = TYPE_MAIN_VARIANT (type); - for (; v; v = TYPE_NEXT_VARIANT (v)) - if (cp_check_qualified_type (v, type, type_quals, rqual, raises, late)) - return v; + tree v = TYPE_MAIN_VARIANT (type); + for (; v; v = TYPE_NEXT_VARIANT (v)) + if (cp_check_qualified_type (v, type, type_quals, rqual, raises, late)) + return v; + } /* Need to build a new variant. */ - v = build_variant_type_copy (type); + tree v = build_variant_type_copy (type); if (!TYPE_DEPENDENT_P (v)) /* We no longer know that it's not type-dependent. */ TYPE_DEPENDENT_P_VALID (v) = false; @@ -2791,10 +2802,7 @@ build_cp_fntype_variant (tree type, cp_ref_qualifier rqual, break; } - /* Canonicalize the exception specification. */ - tree cr = flag_noexcept_type ? canonical_eh_spec (raises) : NULL_TREE; - - if (TYPE_STRUCTURAL_EQUALITY_P (type)) + if (TYPE_STRUCTURAL_EQUALITY_P (type) || complex_p) /* Propagate structural equality. */ SET_TYPE_STRUCTURAL_EQUALITY (v); else if (TYPE_CANONICAL (type) != type || cr != raises || late) @@ -2812,55 +2820,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/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";