From: Jason Merrill <jason@redhat.com>
To: Patrick Palka <ppalka@redhat.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] c++: Fix ICE in tsubst_default_argument [PR92010]
Date: Mon, 30 Mar 2020 16:42:57 -0400 [thread overview]
Message-ID: <0c702b38-1fca-d0ac-c1ee-f7e080d9367c@redhat.com> (raw)
In-Reply-To: <alpine.DEB.2.22.413.2003301553220.3094475@idea>
On 3/30/20 3:58 PM, Patrick Palka wrote:
> On Thu, 26 Mar 2020, Jason Merrill wrote:
>
>> On 3/22/20 9:21 PM, Patrick Palka wrote:
>>> This patch relaxes an assertion in tsubst_default_argument that exposes a
>>> latent
>>> bug in how we substitute an array type into a cv-qualified wildcard function
>>> parameter type. Concretely, the latent bug is that given the function
>>> template
>>>
>>> template<typename T> void foo(const T t);
>>>
>>> one would expect the type of foo<int[]> to be void(const int*), but we
>>> (seemingly prematurely) strip function parameter types of their top-level
>>> cv-qualifiers when building the function's TYPE_ARG_TYPES, and instead end
>>> up
>>> obtaining void(int*) as the type of foo<int[]> after substitution and
>>> decaying.
>>>
>>> We still however correctly substitute into and decay the formal parameter
>>> type,
>>> obtaining const int* as the type of t after substitution. But this then
>>> leads
>>> to us tripping over the assert in tsubst_default_argument that verifies the
>>> formal parameter type and the function type are consistent.
>>>
>>> Assuming it's too late at this stage to fix the substitution bug, we can
>>> still
>>> relax the assertion like so. Tested on x86_64-pc-linux-gnu, does this look
>>> OK?
>>
>> This is core issues 1001/1322, which have not been resolved. Clang does the
>> substitution the way you suggest; EDG rejects the testcase because the two
>> substitutions produce different results. I think it would make sense to
>> follow the EDG behavior until this issue is actually resolved.
>
> Here is what I have so far towards that end. When substituting into the
> PARM_DECLs of a function decl, we now additionally check if the
> aforementioned Core issues are relevant and issue a (fatal) diagnostic
> if so. This patch checks this in tsubst_decl <case PARM_DECL> rather
> than in tsubst_function_decl for efficiency reasons, so that we don't
> have to perform another traversal over the DECL_ARGUMENTS /
> TYPE_ARG_TYPES just to implement this check.
Hmm, this seems like writing more complicated code for a very marginal
optimization; how many function templates have so many parameters that
walking over them once to compare types will have any effect on compile
time?
> Is something like this what you have in mind?
>
> -- >8 --
>
> Subject: [PATCH] c++: Reject some instantiations that depend on resolution of
> Core issues 1001/1322
>
> gcc/cp/ChangeLog:
>
> Core issues 1001 and 1322
> PR c++/92010
> * pt.c (tsubst_decl) <case PARM_DECL>: Detect and reject the case where
> the function type depends on whether we strip top-level qualifiers from
> the type of T before or after substitution.
>
> gcc/testsuite/ChangeLog:
>
> Core issues 1001 and 1322
> PR c++/92010
> * g++.dg/template/array33.C: New test.
> * g++.dg/template/array34.C: New test.
> ---
> gcc/cp/pt.c | 70 ++++++++++++++++++++++++-
> gcc/testsuite/g++.dg/template/array33.C | 39 ++++++++++++++
> gcc/testsuite/g++.dg/template/array34.C | 63 ++++++++++++++++++++++
> 3 files changed, 171 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/g++.dg/template/array33.C
> create mode 100644 gcc/testsuite/g++.dg/template/array34.C
>
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index 8564eb11df4..fd99053df36 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -14072,9 +14072,77 @@ tsubst_decl (tree t, tree args, tsubst_flags_t complain)
> /* We're dealing with a normal parameter. */
> type = tsubst (TREE_TYPE (t), args, complain, in_decl);
>
> + const int type_quals = cp_type_quals (type);
> +
> + /* Determine whether the function type after substitution into the
> + type of the function parameter T depends on the resolution of
> + Core issues 1001/1322, which have not been resolved. In
> + particular, the following detects the case where the resulting
> + function type depends on whether we strip top-level qualifiers
> + from the type of T before or after substitution.
> +
> + This can happen only when the dependent type of T is a
> + cv-qualified wildcard type, and substitution yields a
> + (cv-qualified) array type before array-to-pointer conversion. */
> + tree unqual_expanded_types = NULL_TREE;
> + if (TREE_CODE (type) == ARRAY_TYPE
> + && (type_quals & (TYPE_QUAL_CONST|TYPE_QUAL_VOLATILE))
> + && DECL_FUNCTION_SCOPE_P (t)
> + && !DECL_TEMPLATE_PARM_P (t))
> + {
> + tree type_pattern = (PACK_EXPANSION_P (TREE_TYPE (t))
> + ? PACK_EXPANSION_PATTERN (TREE_TYPE (t))
> + : TREE_TYPE (t));
> + if (WILDCARD_TYPE_P (type_pattern)
> + && cv_qualified_p (type_pattern))
> + {
> + /* Substitute into the corresponding wildcard type that is
> + stripped of its own top-level cv-qualifiers. */
> + tree unqual_type;
> + if (PACK_EXPANSION_P (TREE_TYPE (t)))
> + {
> + if (unqual_expanded_types == NULL_TREE)
> + {
> + tree unqual_pack_expansion
> + = copy_node (TREE_TYPE (t));
> + PACK_EXPANSION_PATTERN (unqual_pack_expansion)
> + = cv_unqualified (type_pattern);
> + unqual_expanded_types
> + = tsubst_pack_expansion (unqual_pack_expansion,
> + args, tf_none, in_decl);
> + }
> + unqual_type = TREE_VEC_ELT (unqual_expanded_types, i);
> + }
> + else
> + {
> + tree unqual_type_pattern = cv_unqualified (type_pattern);
> + unqual_type = tsubst (unqual_type_pattern, args,
> + tf_none, in_decl);
> + }
> + /* Check if the top-level cv-qualifiers on the wildcard type
> + make a difference in the resulting type. */
> + int unqual_type_quals = cp_type_quals (unqual_type);
> + if (type_quals & ~unqual_type_quals
> + & (TYPE_QUAL_CONST|TYPE_QUAL_VOLATILE))
> + {
> + sorry ("the type of function %qD after substitution "
> + "depends on the resolution of Core issues "
> + "1001 and 1332",
> + DECL_NAME (DECL_CONTEXT (t)));
> + if (PACK_EXPANSION_P (TREE_TYPE (t)))
> + inform (input_location, "when substituting into the "
> + "function parameter pack %q#D", t);
> + else
> + inform (input_location, "when substituting into the "
> + "function parameter %q#D", t);
> + type = error_mark_node;
> + }
> + }
> + }
> +
> type = type_decays_to (type);
> TREE_TYPE (r) = type;
> - cp_apply_type_quals_to_decl (cp_type_quals (type), r);
> + cp_apply_type_quals_to_decl (type_quals, r);
>
> if (DECL_INITIAL (r))
> {
> diff --git a/gcc/testsuite/g++.dg/template/array33.C b/gcc/testsuite/g++.dg/template/array33.C
> new file mode 100644
> index 00000000000..20b005bc865
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/array33.C
> @@ -0,0 +1,39 @@
> +// Reject some instantiations whose result depend on the resolution of //
> +// Core issues 1001 and 1332, which are not yet resolved.
> +// { dg-do compile }
> +// { dg-additional-options "-Wno-volatile" }
> +
> +template<typename T>
> +void foo0(T t = 0); // { dg-bogus "" }
> +
> +template<typename T>
> +void foo1(const T = 0); // { dg-message "sorry, unimplemented" }
> +
> +template<typename T>
> +void foo2(volatile T t = 0); // { dg-message "sorry, unimplemented" }
> +
> +template<typename T>
> +void foo3(const volatile T t = 0); // { dg-message "sorry, unimplemented" }
> +
> +int main()
> +{
> + foo0<char[]>(); // { dg-bogus "" }
> + foo0<const char[]>(); // { dg-bogus "" }
> + foo0<volatile char[]>(); // { dg-bogus "" }
> + foo0<const volatile char[]>(); // { dg-bogus "" }
> +
> + foo1<char[]>(); // { dg-message "required from here" }
> + foo1<const char[]>(); // { dg-bogus "" }
> + foo1<volatile char[]>(); // { dg-message "required from here" }
> + foo1<const volatile char[]>(); // { dg-bogus "" }
> +
> + foo2<char[]>(); // { dg-message "required from here" }
> + foo2<const char[]>(); // { dg-message "required from here" }
> + foo2<volatile char[]>(); // { dg-bogus "" }
> + foo2<const volatile char[]>(); // { dg-bogus "" }
> +
> + foo3<char[]>(); // { dg-message "required from here" }
> + foo3<const char[]>(); // { dg-message "required from here" }
> + foo3<volatile char[]>(); // { dg-message "required from here" }
> + foo3<const volatile char[]>(); // { dg-bogus "" }
> +}
> diff --git a/gcc/testsuite/g++.dg/template/array34.C b/gcc/testsuite/g++.dg/template/array34.C
> new file mode 100644
> index 00000000000..316ea2f6407
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/array34.C
> @@ -0,0 +1,63 @@
> +// Reject some instantiations whose result depend on the resolution of
> +// Core issues 1001 and 1332, which are not yet resolved.
> +// { dg-do compile { target c++11 } }
> +// { dg-additional-options "-Wno-volatile" }
> +
> +template<typename... Ts>
> +void foo0(Ts... t); // { dg-bogus "" }
> +
> +template<typename... Ts>
> +void foo1(const Ts... t); // { dg-message "sorry, unimplemented" }
> +
> +template<typename... Ts>
> +void foo2(volatile Ts... t); // { dg-message "sorry, unimplemented" }
> +
> +template<typename... Ts>
> +void foo3(const volatile Ts... t); // { dg-message "sorry, unimplemented" }
> +
> +template<typename... Ts>
> +void bar0()
> +{
> + foo0<int, Ts..., int>(0, 0, 0);
> +}
> +
> +template<typename... Ts>
> +void bar1()
> +{
> + foo1<int, Ts..., int>(0, 0, 0);
> +}
> +
> +template<typename... Ts>
> +void bar2()
> +{
> + foo2<int, Ts..., int>(0, 0, 0);
> +}
> +
> +template<typename... Ts>
> +void bar3()
> +{
> + foo3<int, Ts..., int>(0, 0, 0);
> +}
> +
> +int main()
> +{
> + bar0<char[]>(); // { dg-bogus "" }
> + bar0<const char[]>(); // { dg-bogus "" }
> + bar0<volatile char[]>(); // { dg-bogus "" }
> + bar0<const volatile char[]>(); // { dg-bogus "" }
> +
> + bar1<char[]>(); // { dg-message "required from here" }
> + bar1<const char[]>(); // { dg-bogus "" }
> + bar1<volatile char[]>(); // { dg-message "required from here" }
> + bar1<const volatile char[]>(); // { dg-bogus "" }
> +
> + bar2<char[]>(); // { dg-message "required from here" }
> + bar2<const char[]>(); // { dg-message "required from here" }
> + bar2<volatile char[]>(); // { dg-bogus "" }
> + bar2<const volatile char[]>(); // { dg-bogus "" }
> +
> + bar3<char[]>(); // { dg-message "required from here" }
> + bar3<const char[]>(); // { dg-message "required from here" }
> + bar3<volatile char[]>(); // { dg-message "required from here" }
> + bar3<const volatile char[]>(); // { dg-bogus "" }
> +}
>
next prev parent reply other threads:[~2020-03-30 20:43 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-23 1:21 Patrick Palka
2020-03-23 3:11 ` Patrick Palka
2020-03-26 19:29 ` Jason Merrill
2020-03-30 19:58 ` Patrick Palka
2020-03-30 20:15 ` Patrick Palka
2020-03-30 20:42 ` Jason Merrill [this message]
2020-03-30 22:46 ` Patrick Palka
2020-03-31 17:13 ` Jason Merrill
2020-03-31 19:50 ` Patrick Palka
2020-04-01 22:29 ` Jason Merrill
2020-04-01 22:37 ` Jason Merrill
2020-04-06 15:45 ` Patrick Palka
2020-04-06 21:33 ` Jason Merrill
2020-04-07 17:40 ` Patrick Palka
2020-04-07 20:26 ` Patrick Palka
2020-04-07 21:21 ` Jason Merrill
2020-04-08 14:18 ` Patrick Palka
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=0c702b38-1fca-d0ac-c1ee-f7e080d9367c@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).