Re: [PATCH v4] c++: implement P2564, consteval needs to propagate up [PR107687]

[Jason Merrill <jason@redhat.com>]

On 11/6/23 17:34, Marek Polacek wrote:
> On Fri, Nov 03, 2023 at 01:51:07PM -0400, Jason Merrill wrote:
>> On 11/2/23 11:28, Marek Polacek wrote:
>>> On Sat, Oct 14, 2023 at 12:56:11AM -0400, Jason Merrill wrote:
>>>> On 10/10/23 13:20, Marek Polacek wrote:
>>>>> I suppose some
>>>>> functions cannot possibly be promoted because they don't contain
>>>>> any CALL_EXPRs.  So we may be able to rule them out while doing
>>>>> cp_fold_r early.
>>>>
>>>> Yes.  Or, the only immediate-escalating functions referenced have already
>>>> been checked.
>>
>> It looks like you haven't pursued this yet?  One implementation thought:
> 
> Oops, I'd forgotten to address that.
> 
>> maybe_store_cfun... could stop skipping immediate_escalating_function_p
>> (current_function_decl), and after we're done folding if the current
>> function isn't in the hash_set we can go ahead and set
>> DECL_ESCALATION_CHECKED_P?
> 
> Clever, I see what you mean.  IOW, we store c_f_d iff the function contains
> an i-e expr.  If not, it can't possibly become consteval.  I've added that
> into cp_fold_function, and it seems to work well...
> 
> ...except it revealed a different problem: cp_fold_r -> cp_fold will, since
> https://gcc.gnu.org/pipermail/gcc-patches/2016-March/443993.html, remove
> UNARY_PLUS_EXPR, leading us into this problem:
> 
>    // stmt = +id(i)
>    cp_fold (...);
>    // stmt = id(i)
> 
> and the subsequent tree walk walks the CALL_EXPR's operands, so
> cp_fold_immediate_r will never see the CALL_EXPR, so we miss an i-e expr.
> 
> Perhaps a better solution than the kludge I added would be to only call
> cp_fold_immediate_r after cp_fold.  Or potentially before /and/ after if
> cp_fold changes the expression?

Or walk everything with cp_fold_immediate_r before walking again with 
cp_fold_r?

>>>> It also seems odd that the ADDR_EXPR case calls vec_safe_push
>>>> (deferred_escalating_exprs, while the CALL_EXPR case calls
>>>> maybe_store_cfun_for_late_checking, why the different handling?
>>>
>>> maybe_store_cfun_for_late_checking saves current_function_decl
>>> so that we can check:
>>>
>>> void g (int i) {
>>>     fn (i); // error if fn promotes to consteval
>>> }
>>
>> Yes, but why don't we want the same handling for ADDR_EXPR?
> 
> The handling can't be exactly the same due to global vars like
> 
>    auto p1 = &f5<int>;
> 
> ...but it's wrong to only save the ADDR_EXPR if it's enclosed in
> a function, because the ADDR_EXPR could be inside a consteval if
> block, in which case I think we're not supposed to error.  Tested
> in consteval-prop20.C.  Thanks,

And we don't need the !current_function_decl handling for CALL_EXPR?

The only significant difference I see between &f and f() for escalation 
is that the latter might be an immediate invocation.  Once we've 
determined that it's not, so we are in fact looking at an 
immediate-escalating expression, I'd expect the promotion handling to be 
identical.

> +  /* Whether cp_fold_immediate_r is looking for immediate-escalating
> +     expressions.  */

Isn't that always what it's doing?

The uses of ff_escalating in maybe_explain_promoted_consteval and 
maybe_escalate_decl_and_cfun seem to have different purposes that I'm 
having trouble following.

For the former, it seems to control returning the offending expression 
rather than error_mark_node.  Why don't we always do that?

For the latter, it seems to control recursion, which seems redundant 
with the recursion in that latter function itself.  And the use of the 
flag seems redundant with at_eof.

> +/* Remember that the current function declaration contains a call to
> +   a function that might be promoted to consteval later.  */
> +
> +static void
> +maybe_store_cfun_for_late_checking ()

This name could say more about escalation?  Maybe 
...for_escalation_checking?

Or, better, merge this with maybe_store_immediate_escalating_fn?

> +/* Figure out if DECL should be promoted to consteval and if so, maybe also
> +   promote the function we are in currently.  CALL is the CALL_EXPR of DECL.
> +   EVALP is where we may store the result of cxx_constant_value so that we
> +   don't have to evaluate the same tree again in cp_fold_immediate_r.  */
> +
> +static void
> +maybe_escalate_decl_and_cfun (tree decl, tree call, tree *evalp)
> +{
> +  if (cp_unevaluated_operand)
> +    return;
> +
> +  /* What we're calling is not a consteval function but it may become
> +     one.  This requires recursing; DECL may be promoted to consteval
> +     because it contains an escalating expression E, but E itself may
> +     have to be promoted first, etc.  */
> +  if (unchecked_immediate_escalating_function_p (decl))
> +    {
> +      cp_fold_data data (ff_escalating, decl);
> +      cp_walk_tree (&DECL_SAVED_TREE (decl), cp_fold_immediate_r,
> +		    &data, nullptr);
> +      DECL_ESCALATION_CHECKED_P (decl) = true;

Why recurse both here and in cp_fold_immediate_r?

> +    }
> +
> +  /* In turn, maybe promote the function we find ourselves in...  */
> +  if (DECL_IMMEDIATE_FUNCTION_P (decl)
> +      /* ...but not if the call to DECL was constant; that is the
> +	 "an immediate invocation that is not a constant expression"
> +	 case.  We do this here and not in cp_fold_immediate_r,
> +	 because DECL could have already been consteval and we'd
> +	 never call cp_fold_immediate_r.  */
> +      && (*evalp = cxx_constant_value (call, tf_none),
> +	  *evalp == error_mark_node))

Why check this both here and immediately after the call in 
cp_fold_immediate_r?

> @@ -1041,65 +1217,129 @@ cp_fold_immediate_r (tree *stmt_p, int *walk_subtrees, void *data_)
>     /* The purpose of this is not to emit errors for mce_unknown.  */
>     const tsubst_flags_t complain = (data->flags & ff_mce_false
>   				   ? tf_error : tf_none);
> +  const tree_code code = TREE_CODE (stmt);
> +  tree decl;
> +  tree call = NULL_TREE;
>   
>     /* No need to look into types or unevaluated operands.
>        NB: This affects cp_fold_r as well.  */
> -  if (TYPE_P (stmt) || unevaluated_p (TREE_CODE (stmt)))
> +  if (TYPE_P (stmt) || unevaluated_p (code))
>       {
>         *walk_subtrees = 0;
>         return NULL_TREE;
>       }
>   
> -  switch (TREE_CODE (stmt))
> +  switch (code)
>       {
>       case PTRMEM_CST:
> -      if (TREE_CODE (PTRMEM_CST_MEMBER (stmt)) == FUNCTION_DECL
> -	  && DECL_IMMEDIATE_FUNCTION_P (PTRMEM_CST_MEMBER (stmt)))
> -	{
> -	  if (!data->pset.add (stmt) && (complain & tf_error))
> -	    {
> -	      error_at (PTRMEM_CST_LOCATION (stmt),
> -			"taking address of an immediate function %qD",
> -			PTRMEM_CST_MEMBER (stmt));
> -	      *stmt_p = build_zero_cst (TREE_TYPE (stmt));
> -	    }
> -	  return error_mark_node;
> -	}
> -      break;
> +    case ADDR_EXPR:
> +      {
> +	decl = (code == PTRMEM_CST ? PTRMEM_CST_MEMBER (stmt)
> +		: TREE_OPERAND (stmt, 0));
> +	if (TREE_CODE (decl) != FUNCTION_DECL)
> +	  break;
> +	if (code == ADDR_EXPR && ADDR_EXPR_DENOTES_CALL_P (stmt))
> +	  break;
> +	if (data->flags & ff_escalating)
> +	  goto escalate;
> +	if (immediate_invocation_p (decl))
> +	  {
> +	    if (maybe_promote_function_to_consteval (current_function_decl))
> +	      break;
> +	    if (complain & tf_error)
> +	      {
> +		if (!data->pset.add (stmt))
> +		  {
> +		    taking_address_of_imm_fn_error (stmt, decl);
> +		    *stmt_p = build_zero_cst (TREE_TYPE (stmt));
> +		  }
> +		/* If we're giving hard errors, continue the walk rather than
> +		   bailing out after the first error.  */
> +		break;
> +	      }
> +	    return error_mark_node;
> +	  }
> +	/* Not consteval yet, but could become one, in which case it's invalid
> +	   to take its address.  */
> +	else if (unchecked_immediate_escalating_function_p (decl))
> +	  {
> +	    /* auto p = &f<int>; in the global scope won't be ensconced in
> +	       a function we could store for later at this point.  */
> +	    if (!current_function_decl)
> +	      remember_escalating_expr (stmt);
> +	    else
> +	      maybe_store_cfun_for_late_checking ();
> +	  }
> +	break;
> +      }
>   
>       /* Expand immediate invocations.  */
>       case CALL_EXPR:
>       case AGGR_INIT_EXPR:
>         if (tree fn = cp_get_callee (stmt))
>   	if (TREE_CODE (fn) != ADDR_EXPR || ADDR_EXPR_DENOTES_CALL_P (fn))
> -	  if (tree fndecl = cp_get_fndecl_from_callee (fn, /*fold*/false))
> -	    if (DECL_IMMEDIATE_FUNCTION_P (fndecl))
> -	      {
> -		stmt = cxx_constant_value (stmt, complain);
> -		if (stmt == error_mark_node)
> -		  {
> -		    if (complain & tf_error)
> -		      *stmt_p = error_mark_node;
> -		    return error_mark_node;
> -		  }
> -		*stmt_p = stmt;
> -	      }
> -      break;
> -
> -    case ADDR_EXPR:
> -      if (TREE_CODE (TREE_OPERAND (stmt, 0)) == FUNCTION_DECL
> -	  && DECL_IMMEDIATE_FUNCTION_P (TREE_OPERAND (stmt, 0))
> -	  && !ADDR_EXPR_DENOTES_CALL_P (stmt))
> -	{
> -	  if (complain & tf_error)
> +	  if ((decl = cp_get_fndecl_from_callee (fn, /*fold*/false)))
>   	    {
> -	      error_at (EXPR_LOCATION (stmt),
> -			"taking address of an immediate function %qD",
> -			TREE_OPERAND (stmt, 0));
> -	      *stmt_p = build_zero_cst (TREE_TYPE (stmt));
> +	      if (data->flags & ff_escalating)
> +		{
> +		  call = stmt;
> +		  goto escalate;
> +		}
> +
> +	      tree eval = NULL_TREE;
> +	      /* Escalate once all templates have been instantiated.  */
> +	      if (at_eof > 1)
> +		maybe_escalate_decl_and_cfun (decl, stmt, &eval);
> +
> +	      /* [expr.const]p16 "An expression or conversion is
> +		 immediate-escalating if it is not initially in an immediate
> +		 function context and it is either
> +		 -- an immediate invocation that is not a constant expression
> +		 and is not a subexpression of an immediate invocation."
> +
> +		 If we are in an immediate-escalating function, the
> +		 immediate-escalating expression or conversion makes it an
> +		 immediate function.  So STMT does not need to produce
> +		 a constant expression.  */
> +	      if (immediate_invocation_p (decl))
> +		{
> +		  tree e = eval ? eval : cxx_constant_value (stmt, tf_none);
> +		  if (e == error_mark_node)
> +		    {
> +		      if (maybe_promote_function_to_consteval
> +			  (current_function_decl))
> +			break;
> +		      if (complain & tf_error)
> +			{
> +			  auto_diagnostic_group d;
> +			  location_t loc = cp_expr_loc_or_input_loc (stmt);
> +			  error_at (loc, "call to consteval function %qE is "
> +				    "not a constant expression", stmt);
> +			  /* Explain why it's not a constant expression.  */
> +			  *stmt_p = cxx_constant_value (stmt, complain);
> +			  maybe_explain_promoted_consteval (loc, decl);
> +			  /* Don't return error_mark_node, it would stop our
> +			     tree walk.  */
> +			  break;
> +			}
> +		      return error_mark_node;
> +		    }
> +		  /* We've evaluated the consteval function call.  */
> +		  *stmt_p = e;
> +		}
> +	      /* We've encountered a function call that may turn out to be
> +		 consteval later.  Store its caller so that we can ensure
> +		 that the call is a constant expression.  */
> +	      else if (unchecked_immediate_escalating_function_p (decl))
> +		{
> +		  /* Make sure we're not inserting new elements while walking
> +		     the deferred_escalating_exprs hash table; if we are, it's
> +		     likely that a function wasn't properly marked checked for
> +		     i-e expressions.  */
> +		  gcc_checking_assert (at_eof <= 1);
> +		  maybe_store_cfun_for_late_checking ();
> +		}
>   	    }
> -	  return error_mark_node;
> -	}
>         break;
>   
>       default:
> @@ -1107,11 +1347,35 @@ cp_fold_immediate_r (tree *stmt_p, int *walk_subtrees, void *data_)
>       }
>   
>     return NULL_TREE;
> +
> +escalate:
> +  /* Not consteval yet, but could be.  Have to look deeper.  */
> +  if (unchecked_immediate_escalating_function_p (decl))
> +    {
> +      /* Set before the actual walk to avoid endless recursion.  */
> +      DECL_ESCALATION_CHECKED_P (decl) = true;
> +      cp_fold_data d (ff_escalating, data->caller ? decl : NULL_TREE);

It seems like the "caller" field is in place of setting 
current_function_decl to decl while walking its trees.  Some other 
places instead override current_function_decl and call 
cp_fold_immediate.  Can we unify on the latter pattern, and factor it 
into a separate function?

> +      cp_walk_tree (&DECL_SAVED_TREE (decl), cp_fold_immediate_r, &d, nullptr);
> +    }
> +
> +  /* If it turned out to be consteval, maybe promote the caller.  */
> +  if (DECL_IMMEDIATE_FUNCTION_P (decl)
> +      && (!call || cxx_constant_value (call, tf_none) == error_mark_node))
> +    {
> +      /* We found the escalating expression.  */
> +      if (data->caller)
> +	promote_function_to_consteval (data->caller);
> +      *walk_subtrees = 0;
> +      return stmt;
> +    }

Why different promotion code depending on whether we're recursing?

> +/* We've stashed immediate-escalating functions.  Now see if they indeed
> +   ought to be promoted to consteval.  */
> +
> +void
> +process_pending_immediate_escalating_fns ()
> +{
> +  /* This will be null for -fno-immediate-escalation.  */
> +  if (!deferred_escalating_exprs)
> +    return;
> +
> +  for (auto e : *deferred_escalating_exprs)
> +    if (TREE_CODE (e) == FUNCTION_DECL)
> +      {
> +	if (!DECL_ESCALATION_CHECKED_P (e))
> +	  {
> +	    temp_override<tree> cfd (current_function_decl, e);
> +	    cp_fold_immediate (&DECL_SAVED_TREE (e), mce_false);
> +	  }
> +	if (DECL_IMMEDIATE_FUNCTION_P (e))
> +	  deferred_escalating_exprs->remove (e);
> +      }
> +}
> +
> +/* We've escalated every function that could have been promoted to
> +   consteval.  Check that we are not taking the address of a consteval
> +   function.  */
> +
> +void
> +check_immediate_escalating_refs ()
> +{
> +  /* This will be null for -fno-immediate-escalation.  */
> +  if (!deferred_escalating_exprs)
> +    return;
> +
> +  for (auto ref : *deferred_escalating_exprs)
> +    {
> +      if (TREE_CODE (ref) == FUNCTION_DECL)
> +	/* We saw a function call to an immediate-escalating function in
> +	   the body of REF.  Check that it's a constant if it was promoted
> +	   to consteval.  */
> +	{
> +	  temp_override<tree> cfd (current_function_decl, ref);
> +	  cp_fold_immediate (&DECL_SAVED_TREE (ref), mce_false);

Shouldn't we skip functions here since we just processed them in the 
function above?

Jason