* [PATCH] c++: Fix cp_tree_equal for template value args using dependent sizeof/alignof/noexcept expressions
@ 2021-08-21 4:55 Barrett Adair
2021-08-25 20:22 ` Jason Merrill
0 siblings, 1 reply; 2+ messages in thread
From: Barrett Adair @ 2021-08-21 4:55 UTC (permalink / raw)
To: gcc-patches
This patch fixes AST comparison for trailing return types using dependent
sizeof/alignof/noexcept expressions as template value arguments. I believe
this bug is over a decade old, hailing from GCC 4.6. I found it over 5
years ago and sat on the repro until I had time to fix it myself.
The new test cases demonstrate redeclarations mistakenly parsed as
ambiguous overloads. These fail with gcc, but pass with clang. The last
test case concerns a GNU extension for alignof (hence the -Wno-pedantic).
After applying this patch, bootstrap succeeds, the new tests pass, and my
native "make -k check" outputs look essentially similar to these based on
the same commit:
https://gcc.gnu.org/pipermail/gcc-testresults/2021-August/715032.html
(except I only waited for a few thousand of the libstdc++ tests to pass).
I think it makes sense to now split the switch-case blocks here since there
isn't much shared logic left, and I have a patch to do that if desired, but
I think the patch below is the cleanest for initial review.
I studied the history and churn of the comparing_specializations hacks.
While I am still not entirely certain this is the correct change to make
there, the change seems at least superficially reasonable to me. I haven't
been able to break it yet. Perhaps someone more familiar with this area of
the compiler can weigh in. See also the existing cp_unevaluated_operand
push/pop for these EXPR codes in cp_walk_tree.
I am awaiting a response to the copyright assignment request sent to
assign@gnu.org.
commit fe64ca143dfb9021b4c47abb6382827c13e1f0cd
Author: Barrett Adair <barrettellisadair@gmail.com>
Date: Fri Aug 20 15:37:36 2021 -0500
Fix cp_tree_equal for template value args using
dependent sizeof/alignof/noexcept expressions
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index 3c62dd74380..fce2a46cfa8 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -3903,40 +3903,41 @@ cp_tree_equal (tree t1, tree t2)
as being equivalent to anything. */
if (VAR_P (o1) && DECL_NAME (o1) == NULL_TREE
&& !DECL_RTL_SET_P (o1))
/*Nop*/;
else if (VAR_P (o2) && DECL_NAME (o2) == NULL_TREE
&& !DECL_RTL_SET_P (o2))
/*Nop*/;
else if (!cp_tree_equal (o1, o2))
return false;
return cp_tree_equal (TREE_OPERAND (t1, 1), TREE_OPERAND (t2, 1));
}
case PARM_DECL:
/* For comparing uses of parameters in late-specified return types
with an out-of-class definition of the function, but can also come
up for expressions that involve 'this' in a member function
template. */
if (comparing_specializations
+ && !cp_unevaluated_operand
&& DECL_CONTEXT (t1) != DECL_CONTEXT (t2))
/* When comparing hash table entries, only an exact match is
good enough; we don't want to replace 'this' with the
version from another function. But be more flexible
with parameters with identical contexts. */
return false;
if (same_type_p (TREE_TYPE (t1), TREE_TYPE (t2)))
{
if (DECL_ARTIFICIAL (t1) ^ DECL_ARTIFICIAL (t2))
return false;
if (CONSTRAINT_VAR_P (t1) ^ CONSTRAINT_VAR_P (t2))
return false;
if (DECL_ARTIFICIAL (t1)
|| (DECL_PARM_LEVEL (t1) == DECL_PARM_LEVEL (t2)
&& DECL_PARM_INDEX (t1) == DECL_PARM_INDEX (t2)))
return true;
}
return false;
@@ -3974,63 +3975,68 @@ cp_tree_equal (tree t1, tree t2)
return true;
case CONSTRAINT_INFO:
return cp_tree_equal (CI_ASSOCIATED_CONSTRAINTS (t1),
CI_ASSOCIATED_CONSTRAINTS (t2));
case CHECK_CONSTR:
return (CHECK_CONSTR_CONCEPT (t1) == CHECK_CONSTR_CONCEPT (t2)
&& comp_template_args (CHECK_CONSTR_ARGS (t1),
CHECK_CONSTR_ARGS (t2)));
case TREE_VEC:
/* These are template args. Really we should be getting the
caller to do this as it knows it to be true. */
if (!comp_template_args (t1, t2, NULL, NULL, false))
return false;
return true;
case SIZEOF_EXPR:
case ALIGNOF_EXPR:
+ case NOEXCEPT_EXPR:
{
tree o1 = TREE_OPERAND (t1, 0);
tree o2 = TREE_OPERAND (t2, 0);
if (code1 == SIZEOF_EXPR)
{
if (SIZEOF_EXPR_TYPE_P (t1))
o1 = TREE_TYPE (o1);
if (SIZEOF_EXPR_TYPE_P (t2))
o2 = TREE_TYPE (o2);
}
- else if (ALIGNOF_EXPR_STD_P (t1) != ALIGNOF_EXPR_STD_P (t2))
+ else if (code1 == ALIGNOF_EXPR
+ && ALIGNOF_EXPR_STD_P (t1) != ALIGNOF_EXPR_STD_P (t2))
return false;
if (TREE_CODE (o1) != TREE_CODE (o2))
return false;
- if (ARGUMENT_PACK_P (o1))
- return template_args_equal (o1, o2);
- else if (TYPE_P (o1))
- return same_type_p (o1, o2);
- else
- return cp_tree_equal (o1, o2);
+ {
+ cp_unevaluated ev;
+ if (code1 == SIZEOF_EXPR && ARGUMENT_PACK_P (o1))
+ return template_args_equal (o1, o2);
+ else if (code1 != NOEXCEPT_EXPR && TYPE_P (o1))
+ return same_type_p (o1, o2);
+ else
+ return cp_tree_equal (o1, o2);
+ }
}
case MODOP_EXPR:
{
tree t1_op1, t2_op1;
if (!cp_tree_equal (TREE_OPERAND (t1, 0), TREE_OPERAND (t2, 0)))
return false;
t1_op1 = TREE_OPERAND (t1, 1);
t2_op1 = TREE_OPERAND (t2, 1);
if (TREE_CODE (t1_op1) != TREE_CODE (t2_op1))
return false;
return cp_tree_equal (TREE_OPERAND (t1, 2), TREE_OPERAND (t2, 2));
}
case PTRMEM_CST:
/* Two pointer-to-members are the same if they point to the same
field or function in the same class. */
diff --git a/gcc/testsuite/g++.dg/template/canon-type-15.C
b/gcc/testsuite/g++.dg/template/canon-type-15.C
new file mode 100644
index 00000000000..064e14c510d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/canon-type-15.C
@@ -0,0 +1,5 @@
+// { dg-do compile { target c++11 } }
+template<unsigned u> struct size_c{ static constexpr unsigned value = u; };
+template<class T> auto return_size(T t) -> size_c<sizeof(t)>;
+template<class T> auto return_size(T t) -> size_c<sizeof(t)>;
+static_assert(decltype(return_size('a'))::value == 1u, "");
diff --git a/gcc/testsuite/g++.dg/template/canon-type-16.C
b/gcc/testsuite/g++.dg/template/canon-type-16.C
new file mode 100644
index 00000000000..99361cbac30
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/canon-type-16.C
@@ -0,0 +1,6 @@
+// { dg-do compile { target c++11 } }
+template<bool u> struct bool_c{ static constexpr bool value = u; };
+template<class T> auto noexcepty(T t) -> bool_c<noexcept(t())>;
+template<class T> auto noexcepty(T t) -> bool_c<noexcept(t())>;
+struct foo { void operator()() noexcept; };
+static_assert(decltype(noexcepty(foo{}))::value, "");
diff --git a/gcc/testsuite/g++.dg/template/canon-type-17.C
b/gcc/testsuite/g++.dg/template/canon-type-17.C
new file mode 100644
index 00000000000..0555c8d0a42
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/canon-type-17.C
@@ -0,0 +1,5 @@
+// { dg-do compile { target c++11 } }
+template<unsigned u> struct size_c{ static constexpr unsigned value = u; };
+template<class... T> auto return_size(T... t) -> size_c<sizeof...(t)>;
+template<class... T> auto return_size(T... t) -> size_c<sizeof...(t)>;
+static_assert(decltype(return_size('a'))::value == 1u, "");
diff --git a/gcc/testsuite/g++.dg/template/canon-type-18.C
b/gcc/testsuite/g++.dg/template/canon-type-18.C
new file mode 100644
index 00000000000..2510181725c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/template/canon-type-18.C
@@ -0,0 +1,6 @@
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wno-pedantic" }
+template<unsigned u> struct size_c{ static constexpr unsigned value = u; };
+template<class T> auto get_align(T t) -> size_c<alignof(t)>;
+template<class T> auto get_align(T t) -> size_c<alignof(t)>;
+static_assert(decltype(get_align('a'))::value == 1u, "");
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] c++: Fix cp_tree_equal for template value args using dependent sizeof/alignof/noexcept expressions
2021-08-21 4:55 [PATCH] c++: Fix cp_tree_equal for template value args using dependent sizeof/alignof/noexcept expressions Barrett Adair
@ 2021-08-25 20:22 ` Jason Merrill
0 siblings, 0 replies; 2+ messages in thread
From: Jason Merrill @ 2021-08-25 20:22 UTC (permalink / raw)
To: Barrett Adair, gcc-patches
On 8/21/21 12:55 AM, Barrett Adair via Gcc-patches wrote:
> This patch fixes AST comparison for trailing return types using dependent
> sizeof/alignof/noexcept expressions as template value arguments. I believe
> this bug is over a decade old, hailing from GCC 4.6. I found it over 5
> years ago and sat on the repro until I had time to fix it myself.
Thanks!
> The new test cases demonstrate redeclarations mistakenly parsed as
> ambiguous overloads. These fail with gcc, but pass with clang. The last
> test case concerns a GNU extension for alignof (hence the -Wno-pedantic).
> After applying this patch, bootstrap succeeds, the new tests pass, and my
> native "make -k check" outputs look essentially similar to these based on
> the same commit:
> https://gcc.gnu.org/pipermail/gcc-testresults/2021-August/715032.html
> (except I only waited for a few thousand of the libstdc++ tests to pass).
>
> I think it makes sense to now split the switch-case blocks here since there
> isn't much shared logic left, and I have a patch to do that if desired, but
> I think the patch below is the cleanest for initial review.
>
> I studied the history and churn of the comparing_specializations hacks.
> While I am still not entirely certain this is the correct change to make
> there, the change seems at least superficially reasonable to me. I haven't
> been able to break it yet. Perhaps someone more familiar with this area of
> the compiler can weigh in. See also the existing cp_unevaluated_operand
> push/pop for these EXPR codes in cp_walk_tree.
>
> I am awaiting a response to the copyright assignment request sent to
> assign@gnu.org.
Until that goes through, you can use DCO sign-off
(https://gcc.gnu.org/dco.html).
> commit fe64ca143dfb9021b4c47abb6382827c13e1f0cd
> Author: Barrett Adair <barrettellisadair@gmail.com>
> Date: Fri Aug 20 15:37:36 2021 -0500
Please generate your patch with git format-patch for easier application.
It also looks like the patch was corrupted by your email client; a
simple fix for that is to attach the patch instead of pasting it in the
body of your message. Or 'git help format-patch' has suggestions for
ways to make including the patch inline work.
I mostly use git send-email myself, or attachments when using send-email
would be too complicated.
> Fix cp_tree_equal for template value args using
> dependent sizeof/alignof/noexcept expressions
>
> diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
> index 3c62dd74380..fce2a46cfa8 100644
> --- a/gcc/cp/tree.c
> +++ b/gcc/cp/tree.c
> @@ -3903,40 +3903,41 @@ cp_tree_equal (tree t1, tree t2)
> as being equivalent to anything. */
> if (VAR_P (o1) && DECL_NAME (o1) == NULL_TREE
> && !DECL_RTL_SET_P (o1))
> /*Nop*/;
> else if (VAR_P (o2) && DECL_NAME (o2) == NULL_TREE
> && !DECL_RTL_SET_P (o2))
> /*Nop*/;
> else if (!cp_tree_equal (o1, o2))
> return false;
>
> return cp_tree_equal (TREE_OPERAND (t1, 1), TREE_OPERAND (t2, 1));
> }
>
> case PARM_DECL:
> /* For comparing uses of parameters in late-specified return types
> with an out-of-class definition of the function, but can also come
> up for expressions that involve 'this' in a member function
> template. */
> if (comparing_specializations
> + && !cp_unevaluated_operand
This looks dangerous; I would expect it to mean that in
template <int N> struct A { };
template <class T> void f(T t1) { A<sizeof(t1)> a; }
template <class T> void g(T t2) { A<sizeof(t2)> a; }
the two As are treated as equivalent; that's what the
comparing_specializations code is trying to prevent.
> && DECL_CONTEXT (t1) != DECL_CONTEXT (t2))
> /* When comparing hash table entries, only an exact match is
> good enough; we don't want to replace 'this' with the
> version from another function. But be more flexible
> with parameters with identical contexts. */
> return false;
>
> if (same_type_p (TREE_TYPE (t1), TREE_TYPE (t2)))
> {
> if (DECL_ARTIFICIAL (t1) ^ DECL_ARTIFICIAL (t2))
> return false;
> if (CONSTRAINT_VAR_P (t1) ^ CONSTRAINT_VAR_P (t2))
> return false;
> if (DECL_ARTIFICIAL (t1)
> || (DECL_PARM_LEVEL (t1) == DECL_PARM_LEVEL (t2)
> && DECL_PARM_INDEX (t1) == DECL_PARM_INDEX (t2)))
> return true;
> }
> return false;
>
> @@ -3974,63 +3975,68 @@ cp_tree_equal (tree t1, tree t2)
> return true;
>
> case CONSTRAINT_INFO:
> return cp_tree_equal (CI_ASSOCIATED_CONSTRAINTS (t1),
> CI_ASSOCIATED_CONSTRAINTS (t2));
>
> case CHECK_CONSTR:
> return (CHECK_CONSTR_CONCEPT (t1) == CHECK_CONSTR_CONCEPT (t2)
> && comp_template_args (CHECK_CONSTR_ARGS (t1),
> CHECK_CONSTR_ARGS (t2)));
>
> case TREE_VEC:
> /* These are template args. Really we should be getting the
> caller to do this as it knows it to be true. */
> if (!comp_template_args (t1, t2, NULL, NULL, false))
> return false;
> return true;
>
> case SIZEOF_EXPR:
> case ALIGNOF_EXPR:
> + case NOEXCEPT_EXPR:
> {
> tree o1 = TREE_OPERAND (t1, 0);
> tree o2 = TREE_OPERAND (t2, 0);
>
> if (code1 == SIZEOF_EXPR)
> {
> if (SIZEOF_EXPR_TYPE_P (t1))
> o1 = TREE_TYPE (o1);
> if (SIZEOF_EXPR_TYPE_P (t2))
> o2 = TREE_TYPE (o2);
> }
> - else if (ALIGNOF_EXPR_STD_P (t1) != ALIGNOF_EXPR_STD_P (t2))
> + else if (code1 == ALIGNOF_EXPR
> + && ALIGNOF_EXPR_STD_P (t1) != ALIGNOF_EXPR_STD_P (t2))
> return false;
>
> if (TREE_CODE (o1) != TREE_CODE (o2))
> return false;
>
> - if (ARGUMENT_PACK_P (o1))
> - return template_args_equal (o1, o2);
> - else if (TYPE_P (o1))
> - return same_type_p (o1, o2);
> - else
> - return cp_tree_equal (o1, o2);
> + {
> + cp_unevaluated ev;
> + if (code1 == SIZEOF_EXPR && ARGUMENT_PACK_P (o1))
> + return template_args_equal (o1, o2);
> + else if (code1 != NOEXCEPT_EXPR && TYPE_P (o1))
Are these code1 checks necessary? I don't mind them, but I would have
expected the code to work without them.
> + return same_type_p (o1, o2);
> + else
> + return cp_tree_equal (o1, o2);
> + }
> }
>
> case MODOP_EXPR:
> {
> tree t1_op1, t2_op1;
>
> if (!cp_tree_equal (TREE_OPERAND (t1, 0), TREE_OPERAND (t2, 0)))
> return false;
>
> t1_op1 = TREE_OPERAND (t1, 1);
> t2_op1 = TREE_OPERAND (t2, 1);
> if (TREE_CODE (t1_op1) != TREE_CODE (t2_op1))
> return false;
>
> return cp_tree_equal (TREE_OPERAND (t1, 2), TREE_OPERAND (t2, 2));
> }
>
> case PTRMEM_CST:
> /* Two pointer-to-members are the same if they point to the same
> field or function in the same class. */
> diff --git a/gcc/testsuite/g++.dg/template/canon-type-15.C
> b/gcc/testsuite/g++.dg/template/canon-type-15.C
> new file mode 100644
> index 00000000000..064e14c510d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/canon-type-15.C
> @@ -0,0 +1,5 @@
> +// { dg-do compile { target c++11 } }
> +template<unsigned u> struct size_c{ static constexpr unsigned value = u; };
> +template<class T> auto return_size(T t) -> size_c<sizeof(t)>;
> +template<class T> auto return_size(T t) -> size_c<sizeof(t)>;
> +static_assert(decltype(return_size('a'))::value == 1u, "");
> diff --git a/gcc/testsuite/g++.dg/template/canon-type-16.C
> b/gcc/testsuite/g++.dg/template/canon-type-16.C
> new file mode 100644
> index 00000000000..99361cbac30
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/canon-type-16.C
> @@ -0,0 +1,6 @@
> +// { dg-do compile { target c++11 } }
> +template<bool u> struct bool_c{ static constexpr bool value = u; };
> +template<class T> auto noexcepty(T t) -> bool_c<noexcept(t())>;
> +template<class T> auto noexcepty(T t) -> bool_c<noexcept(t())>;
> +struct foo { void operator()() noexcept; };
> +static_assert(decltype(noexcepty(foo{}))::value, "");
> diff --git a/gcc/testsuite/g++.dg/template/canon-type-17.C
> b/gcc/testsuite/g++.dg/template/canon-type-17.C
> new file mode 100644
> index 00000000000..0555c8d0a42
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/canon-type-17.C
> @@ -0,0 +1,5 @@
> +// { dg-do compile { target c++11 } }
> +template<unsigned u> struct size_c{ static constexpr unsigned value = u; };
> +template<class... T> auto return_size(T... t) -> size_c<sizeof...(t)>;
> +template<class... T> auto return_size(T... t) -> size_c<sizeof...(t)>;
> +static_assert(decltype(return_size('a'))::value == 1u, "");
> diff --git a/gcc/testsuite/g++.dg/template/canon-type-18.C
> b/gcc/testsuite/g++.dg/template/canon-type-18.C
> new file mode 100644
> index 00000000000..2510181725c
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/template/canon-type-18.C
> @@ -0,0 +1,6 @@
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-Wno-pedantic" }
> +template<unsigned u> struct size_c{ static constexpr unsigned value = u; };
> +template<class T> auto get_align(T t) -> size_c<alignof(t)>;
> +template<class T> auto get_align(T t) -> size_c<alignof(t)>;
> +static_assert(decltype(get_align('a'))::value == 1u, "");
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-08-25 20:22 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-21 4:55 [PATCH] c++: Fix cp_tree_equal for template value args using dependent sizeof/alignof/noexcept expressions Barrett Adair
2021-08-25 20:22 ` Jason Merrill
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).