From: Jason Merrill <jason@redhat.com>
To: Patrick Palka <ppalka@redhat.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] c++: canonicity of fn types w/ complex eh specs [PR115159]
Date: Tue, 21 May 2024 22:58:55 -0400 [thread overview]
Message-ID: <d39f3d56-71e0-4a38-8885-3831baee58ea@redhat.com> (raw)
In-Reply-To: <9c0b3ede-9457-aa5c-0934-b82f7a44291f@idea>
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";
next prev parent reply other threads:[~2024-05-22 2:59 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-21 19:36 Patrick Palka
2024-05-21 21:09 ` Jason Merrill
2024-05-21 21:27 ` Patrick Palka
2024-05-21 21:31 ` Patrick Palka
2024-05-21 21:36 ` Jason Merrill
2024-05-22 1:55 ` Patrick Palka
2024-05-22 2:58 ` Jason Merrill [this message]
2024-05-22 13:01 ` Patrick Palka
2024-05-22 13:38 ` Jason Merrill
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d39f3d56-71e0-4a38-8885-3831baee58ea@redhat.com \
--to=jason@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=ppalka@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).