| Message ID | 20190320201213.GA26967@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | C++ PATCH for c++/87145 - bogus error converting class type in template argument list | expand |
Ping. On Wed, Mar 20, 2019 at 04:12:13PM -0400, Marek Polacek wrote: > The fix for 77656 caused us to call convert_nontype_argument even for > value-dependent arguments, to perform the conversion in order to avoid > a bogus warning. > > In this case, the argument is Pod{N}. The call to build_converted_constant_expr > in convert_nontype_argument produces Pod::operator Enum(&{N}). It doesn't crash > because we're in a template and build_address no longer crashes on CONSTRUCTORs > in a template. > > Then when instantiating the function foo we substitute its argument: &{N}. So > we're in tsubst_copy_and_build/ADDR_EXPR. The call to > tsubst_non_call_postfix_expression turns {N} into TARGET_EXPR <D.2329, {.val=2}>. > Then build_x_unary_op is supposed to put the ADDR_EXPR back. It calls > cp_build_addr_expr_strict. But it's *strict*, so the prvalue of class type > TARGET_EXPR <D.2329, {.val=2}> isn't allowed -> error. > > It's _strict since <https://gcc.gnu.org/ml/gcc-patches/2010-09/msg02144.html>, > that seem like a desirable change, and we had a warning for taking the address > of a TARGET_EXPR in build_x_unary_op even before that. > > So rather than messing with _strict, let's avoid this scenario altogether. > I checked whether we have a case in the testsuite that results in convert_like > getting a value-dependent CONSTRUCTOR, but found none. > > With this patch, we avoid it, and only call convert_nontype_argument after > substitution, at which point maybe_constant_value will be able to evaluate > the conversion to a constant. > > This problem doesn't occur when passing Pod{N} as an argument to a function, > or using it as an array dimension; seems we avoid converting the argument if > it's value-dependent. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2019-03-20 Marek Polacek <polacek@redhat.com> > > PR c++/87145 - bogus error converting class type in template arg list. > * pt.c (convert_template_argument): Don't call convert_nontype_argument > if it could involve calling a conversion function with a value-dependent > constructor as its argument. > > * g++.dg/cpp0x/constexpr-conv3.C: New test. > * g++.dg/cpp0x/constexpr-conv4.C: New test. > > diff --git gcc/cp/pt.c gcc/cp/pt.c > index 0acc16d1b92..6878583d99b 100644 > --- gcc/cp/pt.c > +++ gcc/cp/pt.c > @@ -8056,7 +8056,16 @@ convert_template_argument (tree parm, > t = canonicalize_type_argument (t, complain); > > if (!type_dependent_expression_p (orig_arg) > - && !uses_template_parms (t)) > + && !uses_template_parms (t) > + /* This might trigger calling a conversion function with > + a value-dependent argument, which could invoke taking > + the address of a temporary representing the result of > + the conversion. */ > + && !(COMPOUND_LITERAL_P (orig_arg) > + && MAYBE_CLASS_TYPE_P (TREE_TYPE (orig_arg)) > + && TYPE_HAS_CONVERSION (TREE_TYPE (orig_arg)) > + && INTEGRAL_OR_ENUMERATION_TYPE_P (t) > + && value_dependent_expression_p (orig_arg))) > /* We used to call digest_init here. However, digest_init > will report errors, which we don't want when complain > is zero. More importantly, digest_init will try too > @@ -8092,7 +8101,7 @@ convert_template_argument (tree parm, > && TREE_CODE (TREE_TYPE (innertype)) == FUNCTION_TYPE > && TREE_OPERAND_LENGTH (inner) > 0 > && reject_gcc_builtin (TREE_OPERAND (inner, 0))) > - return error_mark_node; > + return error_mark_node; > } > > if (TREE_CODE (val) == SCOPE_REF) > diff --git gcc/testsuite/g++.dg/cpp0x/constexpr-conv3.C gcc/testsuite/g++.dg/cpp0x/constexpr-conv3.C > new file mode 100644 > index 00000000000..3f47c58cd2a > --- /dev/null > +++ gcc/testsuite/g++.dg/cpp0x/constexpr-conv3.C > @@ -0,0 +1,25 @@ > +// PR c++/87145 > +// { dg-do compile { target c++11 } } > + > +template<typename T, T t> struct integral_constant { > + static constexpr T value = t; > +}; > + > +enum class Enum : unsigned {}; > + > +struct Pod { > + unsigned val; > + > + constexpr operator Enum() const { > + return static_cast<Enum>(val); > + } > +}; > + > +template<unsigned N> > +constexpr void foo() { > + using Foo = integral_constant<Enum, Pod{N}>; > +} > + > +int main() { > + foo<2>(); > +} > diff --git gcc/testsuite/g++.dg/cpp0x/constexpr-conv4.C gcc/testsuite/g++.dg/cpp0x/constexpr-conv4.C > new file mode 100644 > index 00000000000..f4e3f00a585 > --- /dev/null > +++ gcc/testsuite/g++.dg/cpp0x/constexpr-conv4.C > @@ -0,0 +1,25 @@ > +// PR c++/87145 > +// { dg-do compile { target c++11 } } > + > +struct S { > + int val; > + > + constexpr operator int() const { > + return static_cast<int>(val); > + } > +}; > + > +template<int N> > +struct F { }; > + > +template<unsigned N> > +constexpr void foo() { > + F<int{N}> f; > + F<S{N}> f2; > +} > + > +int > +main() > +{ > + foo<2>(); > +} Marek
On 3/20/19 4:12 PM, Marek Polacek wrote: > The fix for 77656 caused us to call convert_nontype_argument even for > value-dependent arguments, to perform the conversion in order to avoid > a bogus warning. > > In this case, the argument is Pod{N}. The call to build_converted_constant_expr > in convert_nontype_argument produces Pod::operator Enum(&{N}). It doesn't crash > because we're in a template and build_address no longer crashes on CONSTRUCTORs > in a template. Yeah, we shouldn't be preserving lower level codes like this ADDR_EXPR; we should probably return an IMPLICIT_CONV_EXPR rather than make the call explicit. Jason
On Thu, Mar 28, 2019 at 03:54:30PM -0400, Jason Merrill wrote: > On 3/20/19 4:12 PM, Marek Polacek wrote: > > The fix for 77656 caused us to call convert_nontype_argument even for > > value-dependent arguments, to perform the conversion in order to avoid > > a bogus warning. > > > > In this case, the argument is Pod{N}. The call to build_converted_constant_expr > > in convert_nontype_argument produces Pod::operator Enum(&{N}). It doesn't crash > > because we're in a template and build_address no longer crashes on CONSTRUCTORs > > in a template. > > Yeah, we shouldn't be preserving lower level codes like this ADDR_EXPR; we > should probably return an IMPLICIT_CONV_EXPR rather than make the call > explicit. I'm having trouble with this. Do we want build_converted_constant_expr_internal to build an IMPLICIT_CONV_EXPR in a template, much like perform_implicit_conversion? That seems to break a lot of stuff, and I'm nervous about that at this stage :/ We could probably handle this specially in convert_nontype_argument. Maybe build an IMPLICIT_CONV_EXPR for value-dependent CONSTRUCTORs? Marek
On 4/4/19 5:18 PM, Marek Polacek wrote: > On Thu, Mar 28, 2019 at 03:54:30PM -0400, Jason Merrill wrote: >> On 3/20/19 4:12 PM, Marek Polacek wrote: >>> The fix for 77656 caused us to call convert_nontype_argument even for >>> value-dependent arguments, to perform the conversion in order to avoid >>> a bogus warning. >>> >>> In this case, the argument is Pod{N}. The call to build_converted_constant_expr >>> in convert_nontype_argument produces Pod::operator Enum(&{N}). It doesn't crash >>> because we're in a template and build_address no longer crashes on CONSTRUCTORs >>> in a template. >> >> Yeah, we shouldn't be preserving lower level codes like this ADDR_EXPR; we >> should probably return an IMPLICIT_CONV_EXPR rather than make the call >> explicit. > > I'm having trouble with this. Do we want build_converted_constant_expr_internal > to build an IMPLICIT_CONV_EXPR in a template, much like > perform_implicit_conversion? That seems to break a lot of stuff, and I'm > nervous about that at this stage :/ We could probably handle this specially > in convert_nontype_argument. Maybe build an IMPLICIT_CONV_EXPR for > value-dependent CONSTRUCTORs? That sounds reasonable. Jason
On Fri, Apr 05, 2019 at 12:12:11AM -0400, Jason Merrill wrote: > On 4/4/19 5:18 PM, Marek Polacek wrote: > > On Thu, Mar 28, 2019 at 03:54:30PM -0400, Jason Merrill wrote: > > > On 3/20/19 4:12 PM, Marek Polacek wrote: > > > > The fix for 77656 caused us to call convert_nontype_argument even for > > > > value-dependent arguments, to perform the conversion in order to avoid > > > > a bogus warning. > > > > > > > > In this case, the argument is Pod{N}. The call to build_converted_constant_expr > > > > in convert_nontype_argument produces Pod::operator Enum(&{N}). It doesn't crash > > > > because we're in a template and build_address no longer crashes on CONSTRUCTORs > > > > in a template. > > > > > > Yeah, we shouldn't be preserving lower level codes like this ADDR_EXPR; we > > > should probably return an IMPLICIT_CONV_EXPR rather than make the call > > > explicit. > > > > I'm having trouble with this. Do we want build_converted_constant_expr_internal > > to build an IMPLICIT_CONV_EXPR in a template, much like > > perform_implicit_conversion? That seems to break a lot of stuff, and I'm > > nervous about that at this stage :/ We could probably handle this specially > > in convert_nontype_argument. Maybe build an IMPLICIT_CONV_EXPR for > > value-dependent CONSTRUCTORs? > > That sounds reasonable. Great, thanks. Something like this, then? Bootstrapped/regtested on x86_64-linux, ok for trunk? 2019-04-05 Marek Polacek <polacek@redhat.com> PR c++/87145 - bogus error converting class type in template arg list. * pt.c (convert_nontype_argument): Don't call build_converted_constant_expr if it could involve calling a conversion function with a instantiation-dependent constructor as its argument. * g++.dg/cpp0x/constexpr-conv3.C: New test. * g++.dg/cpp0x/constexpr-conv4.C: New test. diff --git gcc/cp/pt.c gcc/cp/pt.c index 40d954d1e8a..f8001317bda 100644 --- gcc/cp/pt.c +++ gcc/cp/pt.c @@ -6837,6 +6837,19 @@ 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 (COMPOUND_LITERAL_P (expr) + && CONSTRUCTOR_IS_DEPENDENT (expr) + && MAYBE_CLASS_TYPE_P (expr_type) + && TYPE_HAS_CONVERSION (expr_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. */ diff --git gcc/testsuite/g++.dg/cpp0x/constexpr-conv3.C gcc/testsuite/g++.dg/cpp0x/constexpr-conv3.C new file mode 100644 index 00000000000..3f47c58cd2a --- /dev/null +++ gcc/testsuite/g++.dg/cpp0x/constexpr-conv3.C @@ -0,0 +1,25 @@ +// PR c++/87145 +// { dg-do compile { target c++11 } } + +template<typename T, T t> struct integral_constant { + static constexpr T value = t; +}; + +enum class Enum : unsigned {}; + +struct Pod { + unsigned val; + + constexpr operator Enum() const { + return static_cast<Enum>(val); + } +}; + +template<unsigned N> +constexpr void foo() { + using Foo = integral_constant<Enum, Pod{N}>; +} + +int main() { + foo<2>(); +} diff --git gcc/testsuite/g++.dg/cpp0x/constexpr-conv4.C gcc/testsuite/g++.dg/cpp0x/constexpr-conv4.C new file mode 100644 index 00000000000..f4e3f00a585 --- /dev/null +++ gcc/testsuite/g++.dg/cpp0x/constexpr-conv4.C @@ -0,0 +1,25 @@ +// PR c++/87145 +// { dg-do compile { target c++11 } } + +struct S { + int val; + + constexpr operator int() const { + return static_cast<int>(val); + } +}; + +template<int N> +struct F { }; + +template<unsigned N> +constexpr void foo() { + F<int{N}> f; + F<S{N}> f2; +} + +int +main() +{ + foo<2>(); +}
On 4/5/19 4:50 PM, Marek Polacek wrote: > On Fri, Apr 05, 2019 at 12:12:11AM -0400, Jason Merrill wrote: >> On 4/4/19 5:18 PM, Marek Polacek wrote: >>> On Thu, Mar 28, 2019 at 03:54:30PM -0400, Jason Merrill wrote: >>>> On 3/20/19 4:12 PM, Marek Polacek wrote: >>>>> The fix for 77656 caused us to call convert_nontype_argument even for >>>>> value-dependent arguments, to perform the conversion in order to avoid >>>>> a bogus warning. >>>>> >>>>> In this case, the argument is Pod{N}. The call to build_converted_constant_expr >>>>> in convert_nontype_argument produces Pod::operator Enum(&{N}). It doesn't crash >>>>> because we're in a template and build_address no longer crashes on CONSTRUCTORs >>>>> in a template. >>>> >>>> Yeah, we shouldn't be preserving lower level codes like this ADDR_EXPR; we >>>> should probably return an IMPLICIT_CONV_EXPR rather than make the call >>>> explicit. >>> >>> I'm having trouble with this. Do we want build_converted_constant_expr_internal >>> to build an IMPLICIT_CONV_EXPR in a template, much like >>> perform_implicit_conversion? That seems to break a lot of stuff, and I'm >>> nervous about that at this stage :/ We could probably handle this specially >>> in convert_nontype_argument. Maybe build an IMPLICIT_CONV_EXPR for >>> value-dependent CONSTRUCTORs? >> >> That sounds reasonable. > > Great, thanks. Something like this, then? > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2019-04-05 Marek Polacek <polacek@redhat.com> > > PR c++/87145 - bogus error converting class type in template arg list. > * pt.c (convert_nontype_argument): Don't call > build_converted_constant_expr if it could involve calling a conversion > function with a instantiation-dependent constructor as its argument. OK. Jason
diff --git gcc/cp/pt.c gcc/cp/pt.c index 0acc16d1b92..6878583d99b 100644 --- gcc/cp/pt.c +++ gcc/cp/pt.c @@ -8056,7 +8056,16 @@ convert_template_argument (tree parm, t = canonicalize_type_argument (t, complain); if (!type_dependent_expression_p (orig_arg) - && !uses_template_parms (t)) + && !uses_template_parms (t) + /* This might trigger calling a conversion function with + a value-dependent argument, which could invoke taking + the address of a temporary representing the result of + the conversion. */ + && !(COMPOUND_LITERAL_P (orig_arg) + && MAYBE_CLASS_TYPE_P (TREE_TYPE (orig_arg)) + && TYPE_HAS_CONVERSION (TREE_TYPE (orig_arg)) + && INTEGRAL_OR_ENUMERATION_TYPE_P (t) + && value_dependent_expression_p (orig_arg))) /* We used to call digest_init here. However, digest_init will report errors, which we don't want when complain is zero. More importantly, digest_init will try too @@ -8092,7 +8101,7 @@ convert_template_argument (tree parm, && TREE_CODE (TREE_TYPE (innertype)) == FUNCTION_TYPE && TREE_OPERAND_LENGTH (inner) > 0 && reject_gcc_builtin (TREE_OPERAND (inner, 0))) - return error_mark_node; + return error_mark_node; } if (TREE_CODE (val) == SCOPE_REF) diff --git gcc/testsuite/g++.dg/cpp0x/constexpr-conv3.C gcc/testsuite/g++.dg/cpp0x/constexpr-conv3.C new file mode 100644 index 00000000000..3f47c58cd2a --- /dev/null +++ gcc/testsuite/g++.dg/cpp0x/constexpr-conv3.C @@ -0,0 +1,25 @@ +// PR c++/87145 +// { dg-do compile { target c++11 } } + +template<typename T, T t> struct integral_constant { + static constexpr T value = t; +}; + +enum class Enum : unsigned {}; + +struct Pod { + unsigned val; + + constexpr operator Enum() const { + return static_cast<Enum>(val); + } +}; + +template<unsigned N> +constexpr void foo() { + using Foo = integral_constant<Enum, Pod{N}>; +} + +int main() { + foo<2>(); +} diff --git gcc/testsuite/g++.dg/cpp0x/constexpr-conv4.C gcc/testsuite/g++.dg/cpp0x/constexpr-conv4.C new file mode 100644 index 00000000000..f4e3f00a585 --- /dev/null +++ gcc/testsuite/g++.dg/cpp0x/constexpr-conv4.C @@ -0,0 +1,25 @@ +// PR c++/87145 +// { dg-do compile { target c++11 } } + +struct S { + int val; + + constexpr operator int() const { + return static_cast<int>(val); + } +}; + +template<int N> +struct F { }; + +template<unsigned N> +constexpr void foo() { + F<int{N}> f; + F<S{N}> f2; +} + +int +main() +{ + foo<2>(); +}
The fix for 77656 caused us to call convert_nontype_argument even for value-dependent arguments, to perform the conversion in order to avoid a bogus warning. In this case, the argument is Pod{N}. The call to build_converted_constant_expr in convert_nontype_argument produces Pod::operator Enum(&{N}). It doesn't crash because we're in a template and build_address no longer crashes on CONSTRUCTORs in a template. Then when instantiating the function foo we substitute its argument: &{N}. So we're in tsubst_copy_and_build/ADDR_EXPR. The call to tsubst_non_call_postfix_expression turns {N} into TARGET_EXPR <D.2329, {.val=2}>. Then build_x_unary_op is supposed to put the ADDR_EXPR back. It calls cp_build_addr_expr_strict. But it's *strict*, so the prvalue of class type TARGET_EXPR <D.2329, {.val=2}> isn't allowed -> error. It's _strict since <https://gcc.gnu.org/ml/gcc-patches/2010-09/msg02144.html>, that seem like a desirable change, and we had a warning for taking the address of a TARGET_EXPR in build_x_unary_op even before that. So rather than messing with _strict, let's avoid this scenario altogether. I checked whether we have a case in the testsuite that results in convert_like getting a value-dependent CONSTRUCTOR, but found none. With this patch, we avoid it, and only call convert_nontype_argument after substitution, at which point maybe_constant_value will be able to evaluate the conversion to a constant. This problem doesn't occur when passing Pod{N} as an argument to a function, or using it as an array dimension; seems we avoid converting the argument if it's value-dependent. Bootstrapped/regtested on x86_64-linux, ok for trunk? 2019-03-20 Marek Polacek <polacek@redhat.com> PR c++/87145 - bogus error converting class type in template arg list. * pt.c (convert_template_argument): Don't call convert_nontype_argument if it could involve calling a conversion function with a value-dependent constructor as its argument. * g++.dg/cpp0x/constexpr-conv3.C: New test. * g++.dg/cpp0x/constexpr-conv4.C: New test.