Re: [PATCH v2] c++: Fix convert_like in template [PR91465, PR93870, PR92031]

[Jason Merrill <jason@redhat.com>]

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
>