* [PATCH] c++: Fix convert_like in template [PR91465, PR93870, PR92031]
@ 2020-02-29 19:34 Marek Polacek
2020-03-02 22:50 ` Jason Merrill
0 siblings, 1 reply; 4+ messages in thread
From: Marek Polacek @ 2020-02-29 19:34 UTC (permalink / raw)
To: GCC Patches, Jason Merrill
The point of this patch is to fix the recurring problem of trees
generated by convert_like while processing a template that break when
substituting. For instance, when convert_like creates a CALL_EXPR
while in a template, substituting such a call breaks in finish_call_expr
because we have two 'this' arguments. Another problem is that we
can create &TARGET_EXPR<> and then fail when substituting because we're
taking the address of an rvalue. I've analyzed some of the already fixed
PRs and also some of the currently open ones:
In c++/93870 we create EnumWrapper<E>::operator E(&operator~(E)).
In c++/87145 we create S::operator int (&{N}).
In c++/92031 we create &TARGET_EXPR <0>.
And so on. I'd like to fix it once and for all. I wanted something
that fixes all the existing cases, removes the ugly check in
convert_nontype_argument, and something suitable for stage4. I.e.,
I didn't implement any cleanups suggested in
<https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00832.html> regarding
the pattern in e.g. build_explicit_specifier.
The gist of the problem is when convert_like_real creates a call for
a ck_user or wraps a TARGET_EXPR in & in a template. So in these cases
use IMPLICIT_CONV_EXPR. In a template we shouldn't need to perform the
actual conversion, we only need it's result type. Is that something
that convert_like_real shouldn't do?
perform_direct_initialization_if_possible and perform_implicit_conversion_flags
can also create an IMPLICIT_CONV_EXPR.
Given the change above, build_converted_constant_expr can return an
IMPLICIT_CONV_EXPR so call fold_non_dependent_expr rather than
maybe_constant_value to deal with that. A problem with that is that now
we may instantiate something twice in a row (?). Handling all of it in
build_converted_constant_expr won't be that straightforward because we
sometimes call cxx_constant_value to give errors, or use manifestly_const_eval
which should be honored.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2020-02-29 Marek Polacek <polacek@redhat.com>
PR c++/92031 - bogus taking address of rvalue error.
PR c++/91465 - ICE with template codes in check_narrowing.
PR c++/93870 - wrong error when converting template non-type arg.
* call.c (convert_like_real) <case ck_user>: Return IMPLICIT_CONV_EXPR
in a template.
(convert_like_real) <case ck_ref_bind>: Likewise.
* decl.c (compute_array_index_type_loc): Call fold_non_dependent_expr
instead of maybe_constant_value.
* pt.c (convert_nontype_argument): Don't build IMPLICIT_CONV_EXPR.
Set IMPLICIT_CONV_EXPR_NONTYPE_ARG if that's what
build_converted_constant_expr returned.
* typeck2.c (check_narrowing): Call fold_non_dependent_expr instead
of maybe_constant_value.
* g++.dg/cpp0x/conv-tmpl2.C: New test.
* g++.dg/cpp0x/conv-tmpl3.C: New test.
* g++.dg/cpp0x/conv-tmpl4.C: New test.
* g++.dg/cpp1z/conv-tmpl1.C: New test.
---
gcc/cp/call.c | 12 +++++++++
gcc/cp/decl.c | 4 +--
gcc/cp/pt.c | 25 ++++---------------
gcc/cp/typeck2.c | 6 ++++-
gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C | 21 ++++++++++++++++
gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C | 16 ++++++++++++
gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C | 33 +++++++++++++++++++++++++
gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C | 10 ++++++++
8 files changed, 104 insertions(+), 23 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C
create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C
create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C
create mode 100644 gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 85bbd043a1d..4cb07b61695 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -7383,6 +7383,12 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
{
struct z_candidate *cand = convs->cand;
+ /* Creating &TARGET_EXPR<> in a template breaks when substituting,
+ and creating a CALL_EXPR in a template breaks in finish_call_expr
+ so use an IMPLICIT_CONV_EXPR for this conversion. */
+ if (processing_template_decl)
+ return build1 (IMPLICIT_CONV_EXPR, totype, expr);
+
if (cand == NULL)
/* We chose the surrogate function from add_conv_candidate, now we
actually need to build the conversion. */
@@ -7760,6 +7766,12 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
expr = convert_bitfield_to_declared_type (expr);
expr = fold_convert (type, expr);
}
+
+ /* Creating &TARGET_EXPR<> in a template would break when
+ tsubsting the expression, so use an IMPLICIT_CONV_EXPR
+ instead. */
+ if (processing_template_decl)
+ return build1 (IMPLICIT_CONV_EXPR, totype, expr);
expr = build_target_expr_with_type (expr, type, complain);
}
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index e3f4b435a49..d04b042f880 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -10281,8 +10281,8 @@ compute_array_index_type_loc (location_t name_loc, tree name, tree size,
/* Pedantically a constant expression is required here and so
__builtin_is_constant_evaluated () should fold to true if it
is successfully folded into a constant. */
- size = maybe_constant_value (size, NULL_TREE,
- /*manifestly_const_eval=*/true);
+ size = fold_non_dependent_expr (size, complain,
+ /*manifestly_const_eval=*/true);
if (!TREE_CONSTANT (size))
size = origsize;
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 622c70b352f..daeaec5ad93 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -7068,26 +7068,6 @@ convert_nontype_argument (tree type, tree expr, tsubst_flags_t complain)
else if (INTEGRAL_OR_ENUMERATION_TYPE_P (type)
|| cxx_dialect >= cxx17)
{
- /* Calling build_converted_constant_expr might create a call to
- a conversion function with a value-dependent argument, which
- could invoke taking the address of a temporary representing
- the result of the conversion. */
- if (!same_type_ignoring_top_level_qualifiers_p (type, expr_type)
- && ((COMPOUND_LITERAL_P (expr)
- && CONSTRUCTOR_IS_DEPENDENT (expr)
- && MAYBE_CLASS_TYPE_P (expr_type)
- && TYPE_HAS_CONVERSION (expr_type))
- /* Similarly, converting e.g. an integer to a class
- involves a constructor call. convert_like would
- create a TARGET_EXPR, but in a template we can't
- use AGGR_INIT_EXPR, and the TARGET_EXPR would lead
- to a bogus error. */
- || (val_dep_p && MAYBE_CLASS_TYPE_P (type))))
- {
- expr = build1 (IMPLICIT_CONV_EXPR, type, expr);
- IMPLICIT_CONV_EXPR_NONTYPE_ARG (expr) = true;
- return expr;
- }
/* C++17: A template-argument for a non-type template-parameter shall
be a converted constant expression (8.20) of the type of the
template-parameter. */
@@ -7096,6 +7076,11 @@ convert_nontype_argument (tree type, tree expr, tsubst_flags_t complain)
/* Make sure we return NULL_TREE only if we have really issued
an error, as described above. */
return (complain & tf_error) ? NULL_TREE : error_mark_node;
+ else if (TREE_CODE (expr) == IMPLICIT_CONV_EXPR)
+ {
+ IMPLICIT_CONV_EXPR_NONTYPE_ARG (expr) = true;
+ return expr;
+ }
expr = maybe_constant_value (expr, NULL_TREE,
/*manifestly_const_eval=*/true);
expr = convert_from_reference (expr);
diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c
index 48920894b3b..bff4ddbcf81 100644
--- a/gcc/cp/typeck2.c
+++ b/gcc/cp/typeck2.c
@@ -981,7 +981,11 @@ check_narrowing (tree type, tree init, tsubst_flags_t complain,
return ok;
}
- init = maybe_constant_value (init);
+ /* Even non-dependent expressions can still have template
+ codes like CAST_EXPR, so use *_non_dependent_expr to cope. */
+ init = fold_non_dependent_expr (init, complain);
+ if (init == error_mark_node)
+ return ok;
/* If we were asked to only check constants, return early. */
if (const_only && !TREE_CONSTANT (init))
diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C
new file mode 100644
index 00000000000..8a505769c3c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C
@@ -0,0 +1,21 @@
+// PR c++/92031 - bogus taking address of rvalue error.
+// { dg-do compile { target c++11 } }
+
+struct x { const int& l; };
+
+void a(const x&) {}
+
+template<class E>
+void f() {
+ a(x { 0 });
+}
+
+void g() {
+ a(x { 0 });
+}
+
+void
+test ()
+{
+ f<int>();
+}
diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C
new file mode 100644
index 00000000000..e2021aa13e1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C
@@ -0,0 +1,16 @@
+// PR c++/91465 - ICE with template codes in check_narrowing.
+// { dg-do compile { target c++11 } }
+
+enum class D { X };
+enum class S { Z };
+
+D foo(S) { return D{}; }
+D foo(double) { return D{}; }
+
+template <typename>
+struct Bar {
+ D baz(S s)
+ {
+ return D{foo(s)};
+ }
+};
diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C
new file mode 100644
index 00000000000..966a2e1ac9e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C
@@ -0,0 +1,33 @@
+// PR c++/93870 - wrong error when converting template non-type arg.
+// { dg-do compile { target c++11 } }
+
+template <typename ENUM> struct EnumWrapper
+{
+ ENUM value;
+
+ constexpr operator ENUM() const
+ {
+ return value;
+ }
+};
+
+enum E : int { V };
+
+constexpr EnumWrapper<E> operator ~(E a)
+{
+ return {E(~int(a))};
+}
+
+template <E X> struct R
+{
+ static void Func();
+};
+
+template <E X> struct S : R<~X>
+{
+};
+
+void Test()
+{
+ S<E::V>::Func();
+}
diff --git a/gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C b/gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C
new file mode 100644
index 00000000000..5b1205349d0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C
@@ -0,0 +1,10 @@
+// PR c++/91465 - ICE with template codes in check_narrowing.
+// { dg-do compile { target c++17 } }
+
+enum class E { Z };
+
+template <typename F>
+void foo(F)
+{
+ E{char(0)};
+}
base-commit: bd55ce63657b42e32040d1e09b4cd76fe3705474
--
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] c++: Fix convert_like in template [PR91465, PR93870, PR92031]
2020-02-29 19:34 [PATCH] c++: Fix convert_like in template [PR91465, PR93870, PR92031] Marek Polacek
@ 2020-03-02 22:50 ` Jason Merrill
2020-03-09 23:32 ` [PATCH v2] " Marek Polacek
0 siblings, 1 reply; 4+ messages in thread
From: Jason Merrill @ 2020-03-02 22:50 UTC (permalink / raw)
To: Marek Polacek, GCC Patches
On 2/29/20 2:32 PM, Marek Polacek wrote:
> The point of this patch is to fix the recurring problem of trees
> generated by convert_like while processing a template that break when
> substituting. For instance, when convert_like creates a CALL_EXPR
> while in a template, substituting such a call breaks in finish_call_expr
> because we have two 'this' arguments. Another problem is that we
> can create &TARGET_EXPR<> and then fail when substituting because we're
> taking the address of an rvalue. I've analyzed some of the already fixed
> PRs and also some of the currently open ones:
>
> In c++/93870 we create EnumWrapper<E>::operator E(&operator~(E)).
> In c++/87145 we create S::operator int (&{N}).
> In c++/92031 we create &TARGET_EXPR <0>.
>
> And so on. I'd like to fix it once and for all. I wanted something
> that fixes all the existing cases, removes the ugly check in
> convert_nontype_argument, and something suitable for stage4. I.e.,
> I didn't implement any cleanups suggested in
> <https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00832.html> regarding
> the pattern in e.g. build_explicit_specifier.
Hmm, it seems to me that addressing that pattern is an important part of
fixing this once and for all.
> The gist of the problem is when convert_like_real creates a call for
> a ck_user or wraps a TARGET_EXPR in & in a template. So in these cases
> use IMPLICIT_CONV_EXPR. In a template we shouldn't need to perform the
> actual conversion, we only need it's result type. Is that something
> that convert_like_real shouldn't do?
> perform_direct_initialization_if_possible and perform_implicit_conversion_flags
> can also create an IMPLICIT_CONV_EXPR.
This seems like a reasonable approach.
> Given the change above, build_converted_constant_expr can return an
> IMPLICIT_CONV_EXPR so call fold_non_dependent_expr rather than
> maybe_constant_value to deal with that. A problem with that is that now
> we may instantiate something twice in a row (?).
Right, and we must not do that.
> Handling all of it in
> build_converted_constant_expr won't be that straightforward because we
> sometimes call cxx_constant_value to give errors, or use manifestly_const_eval
> which should be honored.
Fair enough. The alternative is ensuring that it's OK to call
build_converted_constant_expr when processing_template_decl is true, and
that the result in that case is still template trees. I think that's
what you are doing.
With this approach, we can change
expr = instantiate_non_dependent_expr_sfinae (expr, complain);
/* Don't let convert_like_real create more template codes. */
processing_template_decl_sentinel s;
expr = build_converted_constant_bool_expr (expr, complain);
expr = cxx_constant_value (expr);
to
expr = build_converted_constant_bool_expr (expr, complain);
expr = instantiate_non_dependent_expr_sfinae (expr, complain);
expr = cxx_constant_value (expr);
Yes?
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2020-02-29 Marek Polacek <polacek@redhat.com>
>
> PR c++/92031 - bogus taking address of rvalue error.
> PR c++/91465 - ICE with template codes in check_narrowing.
> PR c++/93870 - wrong error when converting template non-type arg.
> * call.c (convert_like_real) <case ck_user>: Return IMPLICIT_CONV_EXPR
> in a template.
> (convert_like_real) <case ck_ref_bind>: Likewise.
> * decl.c (compute_array_index_type_loc): Call fold_non_dependent_expr
> instead of maybe_constant_value.
> * pt.c (convert_nontype_argument): Don't build IMPLICIT_CONV_EXPR.
> Set IMPLICIT_CONV_EXPR_NONTYPE_ARG if that's what
> build_converted_constant_expr returned.
> * typeck2.c (check_narrowing): Call fold_non_dependent_expr instead
> of maybe_constant_value.
>
> * g++.dg/cpp0x/conv-tmpl2.C: New test.
> * g++.dg/cpp0x/conv-tmpl3.C: New test.
> * g++.dg/cpp0x/conv-tmpl4.C: New test.
> * g++.dg/cpp1z/conv-tmpl1.C: New test.
> ---
> gcc/cp/call.c | 12 +++++++++
> gcc/cp/decl.c | 4 +--
> gcc/cp/pt.c | 25 ++++---------------
> gcc/cp/typeck2.c | 6 ++++-
> gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C | 21 ++++++++++++++++
> gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C | 16 ++++++++++++
> gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C | 33 +++++++++++++++++++++++++
> gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C | 10 ++++++++
> 8 files changed, 104 insertions(+), 23 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C
> create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C
> create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C
> create mode 100644 gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C
>
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index 85bbd043a1d..4cb07b61695 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -7383,6 +7383,12 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> {
> struct z_candidate *cand = convs->cand;
>
> + /* Creating &TARGET_EXPR<> in a template breaks when substituting,
> + and creating a CALL_EXPR in a template breaks in finish_call_expr
> + so use an IMPLICIT_CONV_EXPR for this conversion. */
> + if (processing_template_decl)
> + return build1 (IMPLICIT_CONV_EXPR, totype, expr);
> +
> if (cand == NULL)
> /* We chose the surrogate function from add_conv_candidate, now we
> actually need to build the conversion. */
> @@ -7760,6 +7766,12 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> expr = convert_bitfield_to_declared_type (expr);
> expr = fold_convert (type, expr);
> }
> +
> + /* Creating &TARGET_EXPR<> in a template would break when
> + tsubsting the expression, so use an IMPLICIT_CONV_EXPR
> + instead. */
> + if (processing_template_decl)
> + return build1 (IMPLICIT_CONV_EXPR, totype, expr);
> expr = build_target_expr_with_type (expr, type, complain);
> }
Don't you need the same thing for ck_list, ck_aggr, ck_base? Is there a
reason not to do this in one place for anything other than ck_identity,
at least if a class type is involved?
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index e3f4b435a49..d04b042f880 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -10281,8 +10281,8 @@ compute_array_index_type_loc (location_t name_loc, tree name, tree size,
> /* Pedantically a constant expression is required here and so
> __builtin_is_constant_evaluated () should fold to true if it
> is successfully folded into a constant. */
> - size = maybe_constant_value (size, NULL_TREE,
> - /*manifestly_const_eval=*/true);
> + size = fold_non_dependent_expr (size, complain,
> + /*manifestly_const_eval=*/true);
Here's that broken double-instantiation. Does it work if you also
remove the instantiate_non_dependent_expr before the call to
build_converted_constant_expr?
>
> if (!TREE_CONSTANT (size))
> size = origsize;
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index 622c70b352f..daeaec5ad93 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -7068,26 +7068,6 @@ convert_nontype_argument (tree type, tree expr, tsubst_flags_t complain)
> else if (INTEGRAL_OR_ENUMERATION_TYPE_P (type)
> || cxx_dialect >= cxx17)
> {
> - /* Calling build_converted_constant_expr might create a call to
> - a conversion function with a value-dependent argument, which
> - could invoke taking the address of a temporary representing
> - the result of the conversion. */
> - if (!same_type_ignoring_top_level_qualifiers_p (type, expr_type)
> - && ((COMPOUND_LITERAL_P (expr)
> - && CONSTRUCTOR_IS_DEPENDENT (expr)
> - && MAYBE_CLASS_TYPE_P (expr_type)
> - && TYPE_HAS_CONVERSION (expr_type))
> - /* Similarly, converting e.g. an integer to a class
> - involves a constructor call. convert_like would
> - create a TARGET_EXPR, but in a template we can't
> - use AGGR_INIT_EXPR, and the TARGET_EXPR would lead
> - to a bogus error. */
> - || (val_dep_p && MAYBE_CLASS_TYPE_P (type))))
> - {
> - expr = build1 (IMPLICIT_CONV_EXPR, type, expr);
> - IMPLICIT_CONV_EXPR_NONTYPE_ARG (expr) = true;
> - return expr;
> - }
> /* C++17: A template-argument for a non-type template-parameter shall
> be a converted constant expression (8.20) of the type of the
> template-parameter. */
> @@ -7096,6 +7076,11 @@ convert_nontype_argument (tree type, tree expr, tsubst_flags_t complain)
> /* Make sure we return NULL_TREE only if we have really issued
> an error, as described above. */
> return (complain & tf_error) ? NULL_TREE : error_mark_node;
> + else if (TREE_CODE (expr) == IMPLICIT_CONV_EXPR)
> + {
> + IMPLICIT_CONV_EXPR_NONTYPE_ARG (expr) = true;
> + return expr;
> + }
> expr = maybe_constant_value (expr, NULL_TREE,
> /*manifestly_const_eval=*/true);
> expr = convert_from_reference (expr);
> diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c
> index 48920894b3b..bff4ddbcf81 100644
> --- a/gcc/cp/typeck2.c
> +++ b/gcc/cp/typeck2.c
> @@ -981,7 +981,11 @@ check_narrowing (tree type, tree init, tsubst_flags_t complain,
> return ok;
> }
>
> - init = maybe_constant_value (init);
> + /* Even non-dependent expressions can still have template
> + codes like CAST_EXPR, so use *_non_dependent_expr to cope. */
> + init = fold_non_dependent_expr (init, complain);
> + if (init == error_mark_node)
> + return ok;
>
> /* If we were asked to only check constants, return early. */
> if (const_only && !TREE_CONSTANT (init))
> diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C
> new file mode 100644
> index 00000000000..8a505769c3c
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C
> @@ -0,0 +1,21 @@
> +// PR c++/92031 - bogus taking address of rvalue error.
> +// { dg-do compile { target c++11 } }
> +
> +struct x { const int& l; };
> +
> +void a(const x&) {}
> +
> +template<class E>
> +void f() {
> + a(x { 0 });
> +}
> +
> +void g() {
> + a(x { 0 });
> +}
> +
> +void
> +test ()
> +{
> + f<int>();
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C
> new file mode 100644
> index 00000000000..e2021aa13e1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C
> @@ -0,0 +1,16 @@
> +// PR c++/91465 - ICE with template codes in check_narrowing.
> +// { dg-do compile { target c++11 } }
> +
> +enum class D { X };
> +enum class S { Z };
> +
> +D foo(S) { return D{}; }
> +D foo(double) { return D{}; }
> +
> +template <typename>
> +struct Bar {
> + D baz(S s)
> + {
> + return D{foo(s)};
> + }
> +};
> diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C
> new file mode 100644
> index 00000000000..966a2e1ac9e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C
> @@ -0,0 +1,33 @@
> +// PR c++/93870 - wrong error when converting template non-type arg.
> +// { dg-do compile { target c++11 } }
> +
> +template <typename ENUM> struct EnumWrapper
> +{
> + ENUM value;
> +
> + constexpr operator ENUM() const
> + {
> + return value;
> + }
> +};
> +
> +enum E : int { V };
> +
> +constexpr EnumWrapper<E> operator ~(E a)
> +{
> + return {E(~int(a))};
> +}
> +
> +template <E X> struct R
> +{
> + static void Func();
> +};
> +
> +template <E X> struct S : R<~X>
> +{
> +};
> +
> +void Test()
> +{
> + S<E::V>::Func();
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C b/gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C
> new file mode 100644
> index 00000000000..5b1205349d0
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C
> @@ -0,0 +1,10 @@
> +// PR c++/91465 - ICE with template codes in check_narrowing.
> +// { dg-do compile { target c++17 } }
> +
> +enum class E { Z };
> +
> +template <typename F>
> +void foo(F)
> +{
> + E{char(0)};
> +}
>
> base-commit: bd55ce63657b42e32040d1e09b4cd76fe3705474
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] c++: Fix convert_like in template [PR91465, PR93870, PR92031]
2020-03-02 22:50 ` Jason Merrill
@ 2020-03-09 23:32 ` Marek Polacek
2020-03-09 23:51 ` Jason Merrill
0 siblings, 1 reply; 4+ messages in thread
From: Marek Polacek @ 2020-03-09 23:32 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
On Mon, Mar 02, 2020 at 05:50:01PM -0500, Jason Merrill wrote:
> On 2/29/20 2:32 PM, Marek Polacek wrote:
> > The point of this patch is to fix the recurring problem of trees
> > generated by convert_like while processing a template that break when
> > substituting. For instance, when convert_like creates a CALL_EXPR
> > while in a template, substituting such a call breaks in finish_call_expr
> > because we have two 'this' arguments. Another problem is that we
> > can create &TARGET_EXPR<> and then fail when substituting because we're
> > taking the address of an rvalue. I've analyzed some of the already fixed
> > PRs and also some of the currently open ones:
> >
> > In c++/93870 we create EnumWrapper<E>::operator E(&operator~(E)).
> > In c++/87145 we create S::operator int (&{N}).
> > In c++/92031 we create &TARGET_EXPR <0>.
> >
> > And so on. I'd like to fix it once and for all. I wanted something
> > that fixes all the existing cases, removes the ugly check in
> > convert_nontype_argument, and something suitable for stage4. I.e.,
> > I didn't implement any cleanups suggested in
> > <https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00832.html> regarding
> > the pattern in e.g. build_explicit_specifier.
>
> Hmm, it seems to me that addressing that pattern is an important part of
> fixing this once and for all.
>
> > The gist of the problem is when convert_like_real creates a call for
> > a ck_user or wraps a TARGET_EXPR in & in a template. So in these cases
> > use IMPLICIT_CONV_EXPR. In a template we shouldn't need to perform the
> > actual conversion, we only need it's result type. Is that something
> > that convert_like_real shouldn't do?
> > perform_direct_initialization_if_possible and perform_implicit_conversion_flags
> > can also create an IMPLICIT_CONV_EXPR.
>
> This seems like a reasonable approach.
Great.
> > Given the change above, build_converted_constant_expr can return an
> > IMPLICIT_CONV_EXPR so call fold_non_dependent_expr rather than
> > maybe_constant_value to deal with that. A problem with that is that now
> > we may instantiate something twice in a row (?).
>
> Right, and we must not do that.
Ack.
> > Handling all of it in
> > build_converted_constant_expr won't be that straightforward because we
> > sometimes call cxx_constant_value to give errors, or use manifestly_const_eval
> > which should be honored.
>
> Fair enough. The alternative is ensuring that it's OK to call
> build_converted_constant_expr when processing_template_decl is true, and
> that the result in that case is still template trees. I think that's what
> you are doing.
>
> With this approach, we can change
>
> expr = instantiate_non_dependent_expr_sfinae (expr, complain);
> /* Don't let convert_like_real create more template codes. */
> processing_template_decl_sentinel s;
> expr = build_converted_constant_bool_expr (expr, complain);
> expr = cxx_constant_value (expr);
>
> to
>
> expr = build_converted_constant_bool_expr (expr, complain);
> expr = instantiate_non_dependent_expr_sfinae (expr, complain);
> expr = cxx_constant_value (expr);
>
> Yes?
Yes, that seems to work. Changed in this patch per the above.
> > --- a/gcc/cp/call.c
> > +++ b/gcc/cp/call.c
> > @@ -7383,6 +7383,12 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> > {
> > struct z_candidate *cand = convs->cand;
> > + /* Creating &TARGET_EXPR<> in a template breaks when substituting,
> > + and creating a CALL_EXPR in a template breaks in finish_call_expr
> > + so use an IMPLICIT_CONV_EXPR for this conversion. */
> > + if (processing_template_decl)
> > + return build1 (IMPLICIT_CONV_EXPR, totype, expr);
> > +
> > if (cand == NULL)
> > /* We chose the surrogate function from add_conv_candidate, now we
> > actually need to build the conversion. */
> > @@ -7760,6 +7766,12 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> > expr = convert_bitfield_to_declared_type (expr);
> > expr = fold_convert (type, expr);
> > }
> > +
> > + /* Creating &TARGET_EXPR<> in a template would break when
> > + tsubsting the expression, so use an IMPLICIT_CONV_EXPR
> > + instead. */
> > + if (processing_template_decl)
> > + return build1 (IMPLICIT_CONV_EXPR, totype, expr);
> > expr = build_target_expr_with_type (expr, type, complain);
> > }
>
> Don't you need the same thing for ck_list, ck_aggr, ck_base? Is there a
> reason not to do this in one place for anything other than ck_identity, at
> least if a class type is involved?
I didn't think I needed it because it didn't look like we might create either
&TARGET_EXPR or CALL_EXPR there. But maybe it'd be safer to do it even for
ck_list, ck_aggr, ck_base, so changed.
But the ck_ref_bind hunk had to stay: we can create a TARGET_EXPR even when
there's no class involved in the conversion, e.g. converting and integer to
a reference type, when a temporary is needed.
> > diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> > index e3f4b435a49..d04b042f880 100644
> > --- a/gcc/cp/decl.c
> > +++ b/gcc/cp/decl.c
> > @@ -10281,8 +10281,8 @@ compute_array_index_type_loc (location_t name_loc, tree name, tree size,
> > /* Pedantically a constant expression is required here and so
> > __builtin_is_constant_evaluated () should fold to true if it
> > is successfully folded into a constant. */
> > - size = maybe_constant_value (size, NULL_TREE,
> > - /*manifestly_const_eval=*/true);
> > + size = fold_non_dependent_expr (size, complain,
> > + /*manifestly_const_eval=*/true);
>
> Here's that broken double-instantiation. Does it work if you also remove
> the instantiate_non_dependent_expr before the call to
> build_converted_constant_expr?
Yes! Since this code here is guarded with !type_dependent_expression_p,
I was wondering what would happen when we have a value-dependent expression;
added new test for that. fold_non_dependent_expr_template will see that the
expr is not is_nondependent_constant_expression, so it just returns it, it's
not TREE_CONSTANT, so we use origsize.
Meanwhile, PR94068 was opened, another instance of this problem. New test
added.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
-- >8 --
The point of this patch is to fix the recurring problem of trees
generated by convert_like while processing a template that break when
substituting. For instance, when convert_like creates a CALL_EXPR
while in a template, substituting such a call breaks in finish_call_expr
because we have two 'this' arguments. Another problem is that we
can create &TARGET_EXPR<> and then fail when substituting because we're
taking the address of an rvalue. I've analyzed some of the already fixed
PRs and also some of the currently open ones:
In c++/93870 we create EnumWrapper<E>::operator E(&operator~(E)).
In c++/87145 we create S::operator int (&{N}).
In c++/92031 we create &TARGET_EXPR <0>.
The gist of the problem is when convert_like_real creates a call for
a ck_user or wraps a TARGET_EXPR in & in a template. So in these cases
use IMPLICIT_CONV_EXPR. In a template we shouldn't need to perform the
actual conversion, we only need it's result type.
perform_direct_initialization_if_possible and
perform_implicit_conversion_flags can also create an IMPLICIT_CONV_EXPR.
Given the change above, build_converted_constant_expr can return an
IMPLICIT_CONV_EXPR so call fold_non_dependent_expr rather than
maybe_constant_value to deal with that.
To avoid the problem of instantiating something twice in a row I'm
removing a call to instantiate_non_dependent_expr_sfinae in
compute_array_index_type_loc. And the build_converted_constant_expr
pattern can now be simplified.
2020-03-09 Marek Polacek <polacek@redhat.com>
PR c++/92031 - bogus taking address of rvalue error.
PR c++/91465 - ICE with template codes in check_narrowing.
PR c++/93870 - wrong error when converting template non-type arg.
PR c++/94068 - ICE with template codes in check_narrowing.
* call.c (convert_like_real): Return IMPLICIT_CONV_EXPR
in a template when not ck_identity and we're dealing with a class.
(convert_like_real) <case ck_ref_bind>: Return IMPLICIT_CONV_EXPR
in a template if we need a temporary.
* decl.c (compute_array_index_type_loc): Remove
instantiate_non_dependent_expr_sfinae call. Call
fold_non_dependent_expr instead of maybe_constant_value.
(build_explicit_specifier): Don't instantiate or create a sentinel
before converting the expression.
* except.c (build_noexcept_spec): Likewise.
* pt.c (convert_nontype_argument): Don't build IMPLICIT_CONV_EXPR.
Set IMPLICIT_CONV_EXPR_NONTYPE_ARG if that's what
build_converted_constant_expr returned.
* typeck2.c (check_narrowing): Call fold_non_dependent_expr instead
of maybe_constant_value.
* g++.dg/cpp0x/conv-tmpl2.C: New test.
* g++.dg/cpp0x/conv-tmpl3.C: New test.
* g++.dg/cpp0x/conv-tmpl4.C: New test.
* g++.dg/cpp0x/conv-tmpl5.C: New test.
* g++.dg/cpp0x/conv-tmpl6.C: New test.
* g++.dg/cpp1z/conv-tmpl1.C: New test.
---
gcc/cp/call.c | 18 ++++++++++++++
gcc/cp/decl.c | 9 +++----
gcc/cp/except.c | 4 +--
gcc/cp/pt.c | 25 ++++---------------
gcc/cp/typeck2.c | 6 ++++-
gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C | 21 ++++++++++++++++
gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C | 16 ++++++++++++
gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C | 33 +++++++++++++++++++++++++
gcc/testsuite/g++.dg/cpp0x/conv-tmpl5.C | 13 ++++++++++
gcc/testsuite/g++.dg/cpp0x/conv-tmpl6.C | 16 ++++++++++++
gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C | 10 ++++++++
11 files changed, 141 insertions(+), 30 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C
create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C
create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C
create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl5.C
create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl6.C
create mode 100644 gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index c0340d96f3c..5767a8b8c4e 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -7377,6 +7377,16 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
if (issue_conversion_warnings && (complain & tf_warning))
conversion_null_warnings (totype, expr, fn, argnum);
+ /* Creating &TARGET_EXPR<> in a template breaks when substituting,
+ and creating a CALL_EXPR in a template breaks in finish_call_expr
+ so use an IMPLICIT_CONV_EXPR for this conversion. We would have
+ created such codes e.g. when calling a user-defined conversion
+ function. */
+ if (processing_template_decl
+ && convs->kind != ck_identity
+ && (CLASS_TYPE_P (totype) || CLASS_TYPE_P (TREE_TYPE (expr))))
+ return build1 (IMPLICIT_CONV_EXPR, totype, expr);
+
switch (convs->kind)
{
case ck_user:
@@ -7763,6 +7773,14 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
expr = convert_bitfield_to_declared_type (expr);
expr = fold_convert (type, expr);
}
+
+ /* Creating &TARGET_EXPR<> in a template would break when
+ tsubsting the expression, so use an IMPLICIT_CONV_EXPR
+ instead. This can happen even when there's no class
+ involved, e.g., when converting an integer to a reference
+ type. */
+ if (processing_template_decl)
+ return build1 (IMPLICIT_CONV_EXPR, totype, expr);
expr = build_target_expr_with_type (expr, type, complain);
}
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index e3f4b435a49..bb242743074 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -10276,13 +10276,12 @@ compute_array_index_type_loc (location_t name_loc, tree name, tree size,
NOP_EXPR with TREE_SIDE_EFFECTS; don't fold in that case. */;
else
{
- size = instantiate_non_dependent_expr_sfinae (size, complain);
size = build_converted_constant_expr (size_type_node, size, complain);
/* Pedantically a constant expression is required here and so
__builtin_is_constant_evaluated () should fold to true if it
is successfully folded into a constant. */
- size = maybe_constant_value (size, NULL_TREE,
- /*manifestly_const_eval=*/true);
+ size = fold_non_dependent_expr (size, complain,
+ /*manifestly_const_eval=*/true);
if (!TREE_CONSTANT (size))
size = origsize;
@@ -17607,10 +17606,8 @@ build_explicit_specifier (tree expr, tsubst_flags_t complain)
/* Wait for instantiation, tsubst_function_decl will handle it. */
return expr;
- expr = instantiate_non_dependent_expr_sfinae (expr, complain);
- /* Don't let convert_like_real create more template codes. */
- processing_template_decl_sentinel s;
expr = build_converted_constant_bool_expr (expr, complain);
+ expr = instantiate_non_dependent_expr_sfinae (expr, complain);
expr = cxx_constant_value (expr);
return expr;
}
diff --git a/gcc/cp/except.c b/gcc/cp/except.c
index 788b96de243..262ba5d309c 100644
--- a/gcc/cp/except.c
+++ b/gcc/cp/except.c
@@ -1299,10 +1299,8 @@ build_noexcept_spec (tree expr, tsubst_flags_t complain)
if (TREE_CODE (expr) != DEFERRED_NOEXCEPT
&& !value_dependent_expression_p (expr))
{
- expr = instantiate_non_dependent_expr_sfinae (expr, complain);
- /* Don't let convert_like_real create more template codes. */
- processing_template_decl_sentinel s;
expr = build_converted_constant_bool_expr (expr, complain);
+ expr = instantiate_non_dependent_expr_sfinae (expr, complain);
expr = cxx_constant_value (expr);
}
if (TREE_CODE (expr) == INTEGER_CST)
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 1c721b31176..49ee3920049 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -7068,26 +7068,6 @@ convert_nontype_argument (tree type, tree expr, tsubst_flags_t complain)
else if (INTEGRAL_OR_ENUMERATION_TYPE_P (type)
|| cxx_dialect >= cxx17)
{
- /* Calling build_converted_constant_expr might create a call to
- a conversion function with a value-dependent argument, which
- could invoke taking the address of a temporary representing
- the result of the conversion. */
- if (!same_type_ignoring_top_level_qualifiers_p (type, expr_type)
- && ((COMPOUND_LITERAL_P (expr)
- && CONSTRUCTOR_IS_DEPENDENT (expr)
- && MAYBE_CLASS_TYPE_P (expr_type)
- && TYPE_HAS_CONVERSION (expr_type))
- /* Similarly, converting e.g. an integer to a class
- involves a constructor call. convert_like would
- create a TARGET_EXPR, but in a template we can't
- use AGGR_INIT_EXPR, and the TARGET_EXPR would lead
- to a bogus error. */
- || (val_dep_p && MAYBE_CLASS_TYPE_P (type))))
- {
- expr = build1 (IMPLICIT_CONV_EXPR, type, expr);
- IMPLICIT_CONV_EXPR_NONTYPE_ARG (expr) = true;
- return expr;
- }
/* C++17: A template-argument for a non-type template-parameter shall
be a converted constant expression (8.20) of the type of the
template-parameter. */
@@ -7096,6 +7076,11 @@ convert_nontype_argument (tree type, tree expr, tsubst_flags_t complain)
/* Make sure we return NULL_TREE only if we have really issued
an error, as described above. */
return (complain & tf_error) ? NULL_TREE : error_mark_node;
+ else if (TREE_CODE (expr) == IMPLICIT_CONV_EXPR)
+ {
+ IMPLICIT_CONV_EXPR_NONTYPE_ARG (expr) = true;
+ return expr;
+ }
expr = maybe_constant_value (expr, NULL_TREE,
/*manifestly_const_eval=*/true);
expr = convert_from_reference (expr);
diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c
index 48920894b3b..bff4ddbcf81 100644
--- a/gcc/cp/typeck2.c
+++ b/gcc/cp/typeck2.c
@@ -981,7 +981,11 @@ check_narrowing (tree type, tree init, tsubst_flags_t complain,
return ok;
}
- init = maybe_constant_value (init);
+ /* Even non-dependent expressions can still have template
+ codes like CAST_EXPR, so use *_non_dependent_expr to cope. */
+ init = fold_non_dependent_expr (init, complain);
+ if (init == error_mark_node)
+ return ok;
/* If we were asked to only check constants, return early. */
if (const_only && !TREE_CONSTANT (init))
diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C
new file mode 100644
index 00000000000..8a505769c3c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C
@@ -0,0 +1,21 @@
+// PR c++/92031 - bogus taking address of rvalue error.
+// { dg-do compile { target c++11 } }
+
+struct x { const int& l; };
+
+void a(const x&) {}
+
+template<class E>
+void f() {
+ a(x { 0 });
+}
+
+void g() {
+ a(x { 0 });
+}
+
+void
+test ()
+{
+ f<int>();
+}
diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C
new file mode 100644
index 00000000000..e2021aa13e1
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C
@@ -0,0 +1,16 @@
+// PR c++/91465 - ICE with template codes in check_narrowing.
+// { dg-do compile { target c++11 } }
+
+enum class D { X };
+enum class S { Z };
+
+D foo(S) { return D{}; }
+D foo(double) { return D{}; }
+
+template <typename>
+struct Bar {
+ D baz(S s)
+ {
+ return D{foo(s)};
+ }
+};
diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C
new file mode 100644
index 00000000000..966a2e1ac9e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C
@@ -0,0 +1,33 @@
+// PR c++/93870 - wrong error when converting template non-type arg.
+// { dg-do compile { target c++11 } }
+
+template <typename ENUM> struct EnumWrapper
+{
+ ENUM value;
+
+ constexpr operator ENUM() const
+ {
+ return value;
+ }
+};
+
+enum E : int { V };
+
+constexpr EnumWrapper<E> operator ~(E a)
+{
+ return {E(~int(a))};
+}
+
+template <E X> struct R
+{
+ static void Func();
+};
+
+template <E X> struct S : R<~X>
+{
+};
+
+void Test()
+{
+ S<E::V>::Func();
+}
diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl5.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl5.C
new file mode 100644
index 00000000000..c83e6d83ed9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl5.C
@@ -0,0 +1,13 @@
+// PR c++/94068 - ICE with template codes in check_narrowing.
+// { dg-do compile { target c++11 } }
+
+enum class A { A1, A2 };
+A foo ();
+long foo (int);
+
+template <typename>
+void
+bar ()
+{
+ const auto c{foo ()};
+}
diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl6.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl6.C
new file mode 100644
index 00000000000..2df3a6cc129
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl6.C
@@ -0,0 +1,16 @@
+// { dg-do compile { target c++11 } }
+
+struct A
+{
+ constexpr A(int) { }
+ constexpr operator int() const { return 1; };
+};
+
+template <class T, int N>
+struct B
+{
+ static constexpr A a = A(N);
+ int ar[a];
+};
+
+B<int, 10> b;
diff --git a/gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C b/gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C
new file mode 100644
index 00000000000..5b1205349d0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C
@@ -0,0 +1,10 @@
+// PR c++/91465 - ICE with template codes in check_narrowing.
+// { dg-do compile { target c++17 } }
+
+enum class E { Z };
+
+template <typename F>
+void foo(F)
+{
+ E{char(0)};
+}
base-commit: 81fa6d7321dd9b645d86de4a8a6967c603f176e3
--
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] c++: Fix convert_like in template [PR91465, PR93870, PR92031]
2020-03-09 23:32 ` [PATCH v2] " Marek Polacek
@ 2020-03-09 23:51 ` Jason Merrill
0 siblings, 0 replies; 4+ messages in thread
From: Jason Merrill @ 2020-03-09 23:51 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
On 3/9/20 7:32 PM, Marek Polacek wrote:
> On Mon, Mar 02, 2020 at 05:50:01PM -0500, Jason Merrill wrote:
>> On 2/29/20 2:32 PM, Marek Polacek wrote:
>>> The point of this patch is to fix the recurring problem of trees
>>> generated by convert_like while processing a template that break when
>>> substituting. For instance, when convert_like creates a CALL_EXPR
>>> while in a template, substituting such a call breaks in finish_call_expr
>>> because we have two 'this' arguments. Another problem is that we
>>> can create &TARGET_EXPR<> and then fail when substituting because we're
>>> taking the address of an rvalue. I've analyzed some of the already fixed
>>> PRs and also some of the currently open ones:
>>>
>>> In c++/93870 we create EnumWrapper<E>::operator E(&operator~(E)).
>>> In c++/87145 we create S::operator int (&{N}).
>>> In c++/92031 we create &TARGET_EXPR <0>.
>>>
>>> And so on. I'd like to fix it once and for all. I wanted something
>>> that fixes all the existing cases, removes the ugly check in
>>> convert_nontype_argument, and something suitable for stage4. I.e.,
>>> I didn't implement any cleanups suggested in
>>> <https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00832.html> regarding
>>> the pattern in e.g. build_explicit_specifier.
>>
>> Hmm, it seems to me that addressing that pattern is an important part of
>> fixing this once and for all.
>>
>>> The gist of the problem is when convert_like_real creates a call for
>>> a ck_user or wraps a TARGET_EXPR in & in a template. So in these cases
>>> use IMPLICIT_CONV_EXPR. In a template we shouldn't need to perform the
>>> actual conversion, we only need it's result type. Is that something
>>> that convert_like_real shouldn't do?
>>> perform_direct_initialization_if_possible and perform_implicit_conversion_flags
>>> can also create an IMPLICIT_CONV_EXPR.
>>
>> This seems like a reasonable approach.
>
> Great.
>
>>> Given the change above, build_converted_constant_expr can return an
>>> IMPLICIT_CONV_EXPR so call fold_non_dependent_expr rather than
>>> maybe_constant_value to deal with that. A problem with that is that now
>>> we may instantiate something twice in a row (?).
>>
>> Right, and we must not do that.
>
> Ack.
>
>>> Handling all of it in
>>> build_converted_constant_expr won't be that straightforward because we
>>> sometimes call cxx_constant_value to give errors, or use manifestly_const_eval
>>> which should be honored.
>>
>> Fair enough. The alternative is ensuring that it's OK to call
>> build_converted_constant_expr when processing_template_decl is true, and
>> that the result in that case is still template trees. I think that's what
>> you are doing.
>>
>> With this approach, we can change
>>
>> expr = instantiate_non_dependent_expr_sfinae (expr, complain);
>> /* Don't let convert_like_real create more template codes. */
>> processing_template_decl_sentinel s;
>> expr = build_converted_constant_bool_expr (expr, complain);
>> expr = cxx_constant_value (expr);
>>
>> to
>>
>> expr = build_converted_constant_bool_expr (expr, complain);
>> expr = instantiate_non_dependent_expr_sfinae (expr, complain);
>> expr = cxx_constant_value (expr);
>>
>> Yes?
>
> Yes, that seems to work. Changed in this patch per the above.
>
>>> --- a/gcc/cp/call.c
>>> +++ b/gcc/cp/call.c
>>> @@ -7383,6 +7383,12 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
>>> {
>>> struct z_candidate *cand = convs->cand;
>>> + /* Creating &TARGET_EXPR<> in a template breaks when substituting,
>>> + and creating a CALL_EXPR in a template breaks in finish_call_expr
>>> + so use an IMPLICIT_CONV_EXPR for this conversion. */
>>> + if (processing_template_decl)
>>> + return build1 (IMPLICIT_CONV_EXPR, totype, expr);
>>> +
>>> if (cand == NULL)
>>> /* We chose the surrogate function from add_conv_candidate, now we
>>> actually need to build the conversion. */
>>> @@ -7760,6 +7766,12 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
>>> expr = convert_bitfield_to_declared_type (expr);
>>> expr = fold_convert (type, expr);
>>> }
>>> +
>>> + /* Creating &TARGET_EXPR<> in a template would break when
>>> + tsubsting the expression, so use an IMPLICIT_CONV_EXPR
>>> + instead. */
>>> + if (processing_template_decl)
>>> + return build1 (IMPLICIT_CONV_EXPR, totype, expr);
>>> expr = build_target_expr_with_type (expr, type, complain);
>>> }
>>
>> Don't you need the same thing for ck_list, ck_aggr, ck_base? Is there a
>> reason not to do this in one place for anything other than ck_identity, at
>> least if a class type is involved?
>
> I didn't think I needed it because it didn't look like we might create either
> &TARGET_EXPR or CALL_EXPR there. But maybe it'd be safer to do it even for
> ck_list, ck_aggr, ck_base, so changed.
>
> But the ck_ref_bind hunk had to stay: we can create a TARGET_EXPR even when
> there's no class involved in the conversion, e.g. converting and integer to
> a reference type, when a temporary is needed.
>
>>> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
>>> index e3f4b435a49..d04b042f880 100644
>>> --- a/gcc/cp/decl.c
>>> +++ b/gcc/cp/decl.c
>>> @@ -10281,8 +10281,8 @@ compute_array_index_type_loc (location_t name_loc, tree name, tree size,
>>> /* Pedantically a constant expression is required here and so
>>> __builtin_is_constant_evaluated () should fold to true if it
>>> is successfully folded into a constant. */
>>> - size = maybe_constant_value (size, NULL_TREE,
>>> - /*manifestly_const_eval=*/true);
>>> + size = fold_non_dependent_expr (size, complain,
>>> + /*manifestly_const_eval=*/true);
>>
>> Here's that broken double-instantiation. Does it work if you also remove
>> the instantiate_non_dependent_expr before the call to
>> build_converted_constant_expr?
>
> Yes! Since this code here is guarded with !type_dependent_expression_p,
> I was wondering what would happen when we have a value-dependent expression;
> added new test for that. fold_non_dependent_expr_template will see that the
> expr is not is_nondependent_constant_expression, so it just returns it, it's
> not TREE_CONSTANT, so we use origsize.
>
> Meanwhile, PR94068 was opened, another instance of this problem. New test
> added.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
OK.
> -- >8 --
> The point of this patch is to fix the recurring problem of trees
> generated by convert_like while processing a template that break when
> substituting. For instance, when convert_like creates a CALL_EXPR
> while in a template, substituting such a call breaks in finish_call_expr
> because we have two 'this' arguments. Another problem is that we
> can create &TARGET_EXPR<> and then fail when substituting because we're
> taking the address of an rvalue. I've analyzed some of the already fixed
> PRs and also some of the currently open ones:
>
> In c++/93870 we create EnumWrapper<E>::operator E(&operator~(E)).
> In c++/87145 we create S::operator int (&{N}).
> In c++/92031 we create &TARGET_EXPR <0>.
>
> The gist of the problem is when convert_like_real creates a call for
> a ck_user or wraps a TARGET_EXPR in & in a template. So in these cases
> use IMPLICIT_CONV_EXPR. In a template we shouldn't need to perform the
> actual conversion, we only need it's result type.
> perform_direct_initialization_if_possible and
> perform_implicit_conversion_flags can also create an IMPLICIT_CONV_EXPR.
>
> Given the change above, build_converted_constant_expr can return an
> IMPLICIT_CONV_EXPR so call fold_non_dependent_expr rather than
> maybe_constant_value to deal with that.
>
> To avoid the problem of instantiating something twice in a row I'm
> removing a call to instantiate_non_dependent_expr_sfinae in
> compute_array_index_type_loc. And the build_converted_constant_expr
> pattern can now be simplified.
>
> 2020-03-09 Marek Polacek <polacek@redhat.com>
>
> PR c++/92031 - bogus taking address of rvalue error.
> PR c++/91465 - ICE with template codes in check_narrowing.
> PR c++/93870 - wrong error when converting template non-type arg.
> PR c++/94068 - ICE with template codes in check_narrowing.
> * call.c (convert_like_real): Return IMPLICIT_CONV_EXPR
> in a template when not ck_identity and we're dealing with a class.
> (convert_like_real) <case ck_ref_bind>: Return IMPLICIT_CONV_EXPR
> in a template if we need a temporary.
> * decl.c (compute_array_index_type_loc): Remove
> instantiate_non_dependent_expr_sfinae call. Call
> fold_non_dependent_expr instead of maybe_constant_value.
> (build_explicit_specifier): Don't instantiate or create a sentinel
> before converting the expression.
> * except.c (build_noexcept_spec): Likewise.
> * pt.c (convert_nontype_argument): Don't build IMPLICIT_CONV_EXPR.
> Set IMPLICIT_CONV_EXPR_NONTYPE_ARG if that's what
> build_converted_constant_expr returned.
> * typeck2.c (check_narrowing): Call fold_non_dependent_expr instead
> of maybe_constant_value.
>
> * g++.dg/cpp0x/conv-tmpl2.C: New test.
> * g++.dg/cpp0x/conv-tmpl3.C: New test.
> * g++.dg/cpp0x/conv-tmpl4.C: New test.
> * g++.dg/cpp0x/conv-tmpl5.C: New test.
> * g++.dg/cpp0x/conv-tmpl6.C: New test.
> * g++.dg/cpp1z/conv-tmpl1.C: New test.
> ---
> gcc/cp/call.c | 18 ++++++++++++++
> gcc/cp/decl.c | 9 +++----
> gcc/cp/except.c | 4 +--
> gcc/cp/pt.c | 25 ++++---------------
> gcc/cp/typeck2.c | 6 ++++-
> gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C | 21 ++++++++++++++++
> gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C | 16 ++++++++++++
> gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C | 33 +++++++++++++++++++++++++
> gcc/testsuite/g++.dg/cpp0x/conv-tmpl5.C | 13 ++++++++++
> gcc/testsuite/g++.dg/cpp0x/conv-tmpl6.C | 16 ++++++++++++
> gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C | 10 ++++++++
> 11 files changed, 141 insertions(+), 30 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C
> create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C
> create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C
> create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl5.C
> create mode 100644 gcc/testsuite/g++.dg/cpp0x/conv-tmpl6.C
> create mode 100644 gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C
>
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index c0340d96f3c..5767a8b8c4e 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -7377,6 +7377,16 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> if (issue_conversion_warnings && (complain & tf_warning))
> conversion_null_warnings (totype, expr, fn, argnum);
>
> + /* Creating &TARGET_EXPR<> in a template breaks when substituting,
> + and creating a CALL_EXPR in a template breaks in finish_call_expr
> + so use an IMPLICIT_CONV_EXPR for this conversion. We would have
> + created such codes e.g. when calling a user-defined conversion
> + function. */
> + if (processing_template_decl
> + && convs->kind != ck_identity
> + && (CLASS_TYPE_P (totype) || CLASS_TYPE_P (TREE_TYPE (expr))))
> + return build1 (IMPLICIT_CONV_EXPR, totype, expr);
> +
> switch (convs->kind)
> {
> case ck_user:
> @@ -7763,6 +7773,14 @@ convert_like_real (conversion *convs, tree expr, tree fn, int argnum,
> expr = convert_bitfield_to_declared_type (expr);
> expr = fold_convert (type, expr);
> }
> +
> + /* Creating &TARGET_EXPR<> in a template would break when
> + tsubsting the expression, so use an IMPLICIT_CONV_EXPR
> + instead. This can happen even when there's no class
> + involved, e.g., when converting an integer to a reference
> + type. */
> + if (processing_template_decl)
> + return build1 (IMPLICIT_CONV_EXPR, totype, expr);
> expr = build_target_expr_with_type (expr, type, complain);
> }
>
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index e3f4b435a49..bb242743074 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -10276,13 +10276,12 @@ compute_array_index_type_loc (location_t name_loc, tree name, tree size,
> NOP_EXPR with TREE_SIDE_EFFECTS; don't fold in that case. */;
> else
> {
> - size = instantiate_non_dependent_expr_sfinae (size, complain);
> size = build_converted_constant_expr (size_type_node, size, complain);
> /* Pedantically a constant expression is required here and so
> __builtin_is_constant_evaluated () should fold to true if it
> is successfully folded into a constant. */
> - size = maybe_constant_value (size, NULL_TREE,
> - /*manifestly_const_eval=*/true);
> + size = fold_non_dependent_expr (size, complain,
> + /*manifestly_const_eval=*/true);
>
> if (!TREE_CONSTANT (size))
> size = origsize;
> @@ -17607,10 +17606,8 @@ build_explicit_specifier (tree expr, tsubst_flags_t complain)
> /* Wait for instantiation, tsubst_function_decl will handle it. */
> return expr;
>
> - expr = instantiate_non_dependent_expr_sfinae (expr, complain);
> - /* Don't let convert_like_real create more template codes. */
> - processing_template_decl_sentinel s;
> expr = build_converted_constant_bool_expr (expr, complain);
> + expr = instantiate_non_dependent_expr_sfinae (expr, complain);
> expr = cxx_constant_value (expr);
> return expr;
> }
> diff --git a/gcc/cp/except.c b/gcc/cp/except.c
> index 788b96de243..262ba5d309c 100644
> --- a/gcc/cp/except.c
> +++ b/gcc/cp/except.c
> @@ -1299,10 +1299,8 @@ build_noexcept_spec (tree expr, tsubst_flags_t complain)
> if (TREE_CODE (expr) != DEFERRED_NOEXCEPT
> && !value_dependent_expression_p (expr))
> {
> - expr = instantiate_non_dependent_expr_sfinae (expr, complain);
> - /* Don't let convert_like_real create more template codes. */
> - processing_template_decl_sentinel s;
> expr = build_converted_constant_bool_expr (expr, complain);
> + expr = instantiate_non_dependent_expr_sfinae (expr, complain);
> expr = cxx_constant_value (expr);
> }
> if (TREE_CODE (expr) == INTEGER_CST)
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index 1c721b31176..49ee3920049 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -7068,26 +7068,6 @@ convert_nontype_argument (tree type, tree expr, tsubst_flags_t complain)
> else if (INTEGRAL_OR_ENUMERATION_TYPE_P (type)
> || cxx_dialect >= cxx17)
> {
> - /* Calling build_converted_constant_expr might create a call to
> - a conversion function with a value-dependent argument, which
> - could invoke taking the address of a temporary representing
> - the result of the conversion. */
> - if (!same_type_ignoring_top_level_qualifiers_p (type, expr_type)
> - && ((COMPOUND_LITERAL_P (expr)
> - && CONSTRUCTOR_IS_DEPENDENT (expr)
> - && MAYBE_CLASS_TYPE_P (expr_type)
> - && TYPE_HAS_CONVERSION (expr_type))
> - /* Similarly, converting e.g. an integer to a class
> - involves a constructor call. convert_like would
> - create a TARGET_EXPR, but in a template we can't
> - use AGGR_INIT_EXPR, and the TARGET_EXPR would lead
> - to a bogus error. */
> - || (val_dep_p && MAYBE_CLASS_TYPE_P (type))))
> - {
> - expr = build1 (IMPLICIT_CONV_EXPR, type, expr);
> - IMPLICIT_CONV_EXPR_NONTYPE_ARG (expr) = true;
> - return expr;
> - }
> /* C++17: A template-argument for a non-type template-parameter shall
> be a converted constant expression (8.20) of the type of the
> template-parameter. */
> @@ -7096,6 +7076,11 @@ convert_nontype_argument (tree type, tree expr, tsubst_flags_t complain)
> /* Make sure we return NULL_TREE only if we have really issued
> an error, as described above. */
> return (complain & tf_error) ? NULL_TREE : error_mark_node;
> + else if (TREE_CODE (expr) == IMPLICIT_CONV_EXPR)
> + {
> + IMPLICIT_CONV_EXPR_NONTYPE_ARG (expr) = true;
> + return expr;
> + }
> expr = maybe_constant_value (expr, NULL_TREE,
> /*manifestly_const_eval=*/true);
> expr = convert_from_reference (expr);
> diff --git a/gcc/cp/typeck2.c b/gcc/cp/typeck2.c
> index 48920894b3b..bff4ddbcf81 100644
> --- a/gcc/cp/typeck2.c
> +++ b/gcc/cp/typeck2.c
> @@ -981,7 +981,11 @@ check_narrowing (tree type, tree init, tsubst_flags_t complain,
> return ok;
> }
>
> - init = maybe_constant_value (init);
> + /* Even non-dependent expressions can still have template
> + codes like CAST_EXPR, so use *_non_dependent_expr to cope. */
> + init = fold_non_dependent_expr (init, complain);
> + if (init == error_mark_node)
> + return ok;
>
> /* If we were asked to only check constants, return early. */
> if (const_only && !TREE_CONSTANT (init))
> diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C
> new file mode 100644
> index 00000000000..8a505769c3c
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl2.C
> @@ -0,0 +1,21 @@
> +// PR c++/92031 - bogus taking address of rvalue error.
> +// { dg-do compile { target c++11 } }
> +
> +struct x { const int& l; };
> +
> +void a(const x&) {}
> +
> +template<class E>
> +void f() {
> + a(x { 0 });
> +}
> +
> +void g() {
> + a(x { 0 });
> +}
> +
> +void
> +test ()
> +{
> + f<int>();
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C
> new file mode 100644
> index 00000000000..e2021aa13e1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl3.C
> @@ -0,0 +1,16 @@
> +// PR c++/91465 - ICE with template codes in check_narrowing.
> +// { dg-do compile { target c++11 } }
> +
> +enum class D { X };
> +enum class S { Z };
> +
> +D foo(S) { return D{}; }
> +D foo(double) { return D{}; }
> +
> +template <typename>
> +struct Bar {
> + D baz(S s)
> + {
> + return D{foo(s)};
> + }
> +};
> diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C
> new file mode 100644
> index 00000000000..966a2e1ac9e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl4.C
> @@ -0,0 +1,33 @@
> +// PR c++/93870 - wrong error when converting template non-type arg.
> +// { dg-do compile { target c++11 } }
> +
> +template <typename ENUM> struct EnumWrapper
> +{
> + ENUM value;
> +
> + constexpr operator ENUM() const
> + {
> + return value;
> + }
> +};
> +
> +enum E : int { V };
> +
> +constexpr EnumWrapper<E> operator ~(E a)
> +{
> + return {E(~int(a))};
> +}
> +
> +template <E X> struct R
> +{
> + static void Func();
> +};
> +
> +template <E X> struct S : R<~X>
> +{
> +};
> +
> +void Test()
> +{
> + S<E::V>::Func();
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl5.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl5.C
> new file mode 100644
> index 00000000000..c83e6d83ed9
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl5.C
> @@ -0,0 +1,13 @@
> +// PR c++/94068 - ICE with template codes in check_narrowing.
> +// { dg-do compile { target c++11 } }
> +
> +enum class A { A1, A2 };
> +A foo ();
> +long foo (int);
> +
> +template <typename>
> +void
> +bar ()
> +{
> + const auto c{foo ()};
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp0x/conv-tmpl6.C b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl6.C
> new file mode 100644
> index 00000000000..2df3a6cc129
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/conv-tmpl6.C
> @@ -0,0 +1,16 @@
> +// { dg-do compile { target c++11 } }
> +
> +struct A
> +{
> + constexpr A(int) { }
> + constexpr operator int() const { return 1; };
> +};
> +
> +template <class T, int N>
> +struct B
> +{
> + static constexpr A a = A(N);
> + int ar[a];
> +};
> +
> +B<int, 10> b;
> diff --git a/gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C b/gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C
> new file mode 100644
> index 00000000000..5b1205349d0
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1z/conv-tmpl1.C
> @@ -0,0 +1,10 @@
> +// PR c++/91465 - ICE with template codes in check_narrowing.
> +// { dg-do compile { target c++17 } }
> +
> +enum class E { Z };
> +
> +template <typename F>
> +void foo(F)
> +{
> + E{char(0)};
> +}
>
> base-commit: 81fa6d7321dd9b645d86de4a8a6967c603f176e3
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-03-09 23:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-29 19:34 [PATCH] c++: Fix convert_like in template [PR91465, PR93870, PR92031] Marek Polacek
2020-03-02 22:50 ` Jason Merrill
2020-03-09 23:32 ` [PATCH v2] " Marek Polacek
2020-03-09 23:51 ` 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).