Re: [PATCH, v2] c++: Diagnose taking address of an immediate member function [PR102753]

[Jason Merrill <jason@redhat.com>]

On 10/29/21 11:24, Jakub Jelinek wrote:
> On Tue, Oct 26, 2021 at 04:58:11PM -0400, Jason Merrill wrote:
>>> I'm afraid I don't have a good idea where to move that diagnostic to though,
>>> it would need to be done somewhere where we are certain we aren't in a
>>> subexpression of immediate invocation.  Given statement expressions, even
>>> diagnostics after parsing whole statements might not be good enough, e.g.
>>> void
>>> qux ()
>>> {
>>>     static_assert (bar (({ constexpr auto a = 1; foo; })) == 42);
>>> }
>>
>> I suppose (a wrapper for) fold_build_cleanup_point_expr would be a possible
>> place to check, since that's called for full-expressions.
> 
> I've played a little bit with this (tried to do it at cp_fold time), but
> there are problems with that.
> cp_fold of course isn't a good spot for this because it can be called from
> fold_for_warn and at that point we don't know if we are inside of immediate
> invocation's argument or not, or it can be called even inside of consteval
> fn bodies etc.

How about checking in cp_fold_r instead of cp_fold?

> So, let's suppose we do a separate cp_walk_tree just for
> this if cxx_dialect >= cxx20 e.g. from cp_fold_function and
> cp_fully_fold_init or some other useful spot, like in the patch below
> we avoid walking into THEN_CLAUSE of IF_STMT_CONSTEVAL_P IF_STMTs.
> And if this would be done before cp_fold_function's cp_fold_r walk,
> we'd also need calls to source_location_current_p as an exception.
> The major problem is the location used for the error_at,
> e.g. the ADDR_EXPRs pretty much never EXPR_HAS_LOCATION and PTRMEM_CST
> doesn't even have location, so while we would report diagnostics, it would
> be always
> cc1plus: error: taking address of an immediate function ‘consteval int S::foo() const’
> etc.

I've checked in a patch to give PTRMEM_CST a location wrapper; perhaps 
that will be helpful.

> I guess one option is to report it even later, during gimplification where
> gimplify_expr etc. track input_location, but what to do with static
> initializers?
> Another option would be to have a walk_tree_1 variant that would be updating
> input_location similarly to how gimplify_expr does that, i.e.
>    saved_location = input_location;
>    if (save_expr != error_mark_node
>        && EXPR_HAS_LOCATION (*expr_p))
>      input_location = EXPR_LOCATION (*expr_p);
> ...
>    input_location = saved_location;
> but probably using RAII because walk_tree_1 has a lot of returns in it.

iloc_sentinel seems relevant.

> And turn walk_tree_1 into a template instantiated twice, once as walk_tree_1
> without the input_location handling in it and once with it under some
> different name?

Maybe just add the handling to walk_tree_1?

> Or do we have some other expression walker that does update input_location
> as it goes?
> 
> --- gcc/cp/typeck.c.jj	2021-10-27 09:03:07.555043491 +0200
> +++ gcc/cp/typeck.c	2021-10-29 15:59:57.871449304 +0200
> @@ -6773,16 +6773,6 @@ cp_build_addr_expr_1 (tree arg, bool str
>   	    return error_mark_node;
>   	  }
>   
> -	if (TREE_CODE (t) == FUNCTION_DECL
> -	    && DECL_IMMEDIATE_FUNCTION_P (t)
> -	    && !in_immediate_context ())
> -	  {
> -	    if (complain & tf_error)
> -	      error_at (loc, "taking address of an immediate function %qD",
> -			t);
> -	    return error_mark_node;
> -	  }
> -
>   	type = build_ptrmem_type (context_for_name_lookup (t),
>   				  TREE_TYPE (t));
>   	t = make_ptrmem_cst (type, t);
> @@ -6809,15 +6799,6 @@ cp_build_addr_expr_1 (tree arg, bool str
>       {
>         tree stripped_arg = tree_strip_any_location_wrapper (arg);
>         if (TREE_CODE (stripped_arg) == FUNCTION_DECL
> -	  && DECL_IMMEDIATE_FUNCTION_P (stripped_arg)
> -	  && !in_immediate_context ())
> -	{
> -	  if (complain & tf_error)
> -	    error_at (loc, "taking address of an immediate function %qD",
> -		      stripped_arg);
> -	  return error_mark_node;
> -	}
> -      if (TREE_CODE (stripped_arg) == FUNCTION_DECL
>   	  && !mark_used (stripped_arg, complain) && !(complain & tf_error))
>   	return error_mark_node;
>         val = build_address (arg);
> --- gcc/cp/cp-gimplify.c.jj	2021-09-18 09:47:08.409573816 +0200
> +++ gcc/cp/cp-gimplify.c	2021-10-29 16:48:42.308261319 +0200
> @@ -902,6 +902,17 @@ cp_fold_r (tree *stmt_p, int *walk_subtr
>   	}
>         cp_walk_tree (&OMP_FOR_PRE_BODY (stmt), cp_fold_r, data, NULL);
>         *walk_subtrees = 0;
> +      return NULL;
> +    }
> +
> +  if (code == IF_STMT && IF_STMT_CONSTEVAL_P (stmt))
> +    {
> +      /* Don't walk THEN_CLAUSE (stmt) for consteval if.  IF_COND is always
> +	 boolean_false_node.  */
> +      cp_walk_tree (&ELSE_CLAUSE (stmt), cp_fold_r, data, NULL);
> +      cp_walk_tree (&IF_SCOPE (stmt), cp_fold_r, data, NULL);
> +      *walk_subtrees = 0;
> +      return NULL;
>       }
>   
>     return NULL;
> @@ -1418,9 +1429,9 @@ cp_genericize_r (tree *stmt_p, int *walk
>   	}
>   
>         if (tree fndecl = cp_get_callee_fndecl_nofold (stmt))
> -	if (DECL_IMMEDIATE_FUNCTION_P (fndecl))
> +	if (DECL_IMMEDIATE_FUNCTION_P (fndecl)
> +	    && source_location_current_p (fndecl))
>   	  {
> -	    gcc_assert (source_location_current_p (fndecl));
>   	    *stmt_p = cxx_constant_value (stmt);
>   	    break;
>   	  }
> @@ -2319,8 +2330,28 @@ cp_fold (tree x)
>   	}
>         goto unary;
>   
> +    case PTRMEM_CST:
> +      if (TREE_CODE (PTRMEM_CST_MEMBER (x)) == FUNCTION_DECL
> +	  && DECL_IMMEDIATE_FUNCTION_P (PTRMEM_CST_MEMBER (x)))
> +	{
> +	  error_at (input_location,
> +		    "taking address of an immediate function %qD",
> +		    PTRMEM_CST_MEMBER (x));
> +	  x = error_mark_node;
> +	  break;
> +	}
> +      break;
> +
>       case ADDR_EXPR:
>         loc = EXPR_LOCATION (x);
> +      if (TREE_CODE (TREE_OPERAND (x, 0)) == FUNCTION_DECL
> +	  && DECL_IMMEDIATE_FUNCTION_P (TREE_OPERAND (x, 0)))
> +	{
> +	  error_at (loc, "taking address of an immediate function %qD",
> +		    TREE_OPERAND (x, 0));
> +	  x = error_mark_node;
> +	  break;
> +	}
>         op0 = cp_fold_maybe_rvalue (TREE_OPERAND (x, 0), false);
>   
>         /* Cope with user tricks that amount to offsetof.  */
> 
> 
> 	Jakub
>