Re: [PATCH] c++: recalculating local specs via build_extra_args [PR114303]

[Jason Merrill <jason@redhat.com>]

On 4/10/24 20:00, Patrick Palka wrote:
> On Wed, 10 Apr 2024, Jason Merrill wrote:
> 
>> On 4/10/24 17:39, Patrick Palka wrote:
>>> On Wed, 10 Apr 2024, Jason Merrill wrote:
>>>
>>>> On 3/12/24 10:51, Patrick Palka wrote:
>>>>> On Tue, 12 Mar 2024, Patrick Palka wrote:
>>>>>> On Tue, 12 Mar 2024, Jason Merrill wrote:
>>>>>>> On 3/11/24 12:53, Patrick Palka wrote:
>>>>>>>>
>>>>>>>> r13-6452-g341e6cd8d603a3 made build_extra_args walk evaluated
>>>>>>>> contexts
>>>>>>>> first so that we prefer processing a local specialization in an
>>>>>>>> evaluated
>>>>>>>> context even if its first use is in an unevaluated context.  But
>>>>>>>> this
>>>>>>>> means we need to avoid walking a tree that already has extra
>>>>>>>> args/specs
>>>>>>>> saved because the list of saved specs appears to be an evaluated
>>>>>>>> context.  It seems then that we should be calculating the saved
>>>>>>>> specs
>>>>>>>> from scratch each time, rather than potentially walking the saved
>>>>>>>> specs
>>>>>>>> list from an earlier partial instantiation when calling
>>>>>>>> build_extra_args
>>>>>>>> a second time around.
>>>>>>>
>>>>>>> Makes sense, but I wonder if we want to approach that by avoiding
>>>>>>> walking into
>>>>>>> *_EXTRA_ARGS in extract_locals_r?  Or do we still want to walk into
>>>>>>> any
>>>>>>> nested
>>>>>>> extra args?  And if so, will we run into this same problem then?
>>>>>>
>>>>>> I'm not sure totally but I'd expect a nested extra-args tree to always
>>>>>> have empty *_EXTRA_ARGS since the outer extra-args tree should
>>>>>> intercept
>>>>>> any substitution before the inner extra-args tree can see it?
>>>>>
>>>>> ... and so in extract_locals_r I think we can assume *_EXTRA_ARGS is
>>>>> empty, and not have to explicitly avoid walking it.
>>>>
>>>> It seems more robust to me to handle _EXTRA_ARGS appropriately in
>>>> build_extra_args rather than expect callers to know that they shouldn't
>>>> pass
>>>> in a tree with _EXTRA_ARGS set.  At least check and abort in that case?
>>>
>>> Sounds good.  That IMHO seems simpler than actually avoiding walking
>>> into *_EXTRA_ARGS from extract_locals_r because we'd have to repeat
>>> the walking logic from cp_walk_subtree modulo the *_EXTRA_ARGS walk.
>>>
>>> How does the following look? Bootstraped and regtested on
>>> x86_64-pc-linux-gnu.
>>>
>>> diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
>>> index c38594cd862..6cc9b95fc06 100644
>>> --- a/gcc/cp/pt.cc
>>> +++ b/gcc/cp/pt.cc
>>> @@ -13310,6 +13310,19 @@ extract_locals_r (tree *tp, int *walk_subtrees,
>>> void *data_)
>>>        /* Remember local typedefs (85214).  */
>>>        tp = &TYPE_NAME (*tp);
>>>    
>>
>> Please add a comment explaining why it needs to be null.
>>
>> Also, how about a generic _EXTRA_ARGS accessor so places like this don't need
>> to check each code themselves?
> 
> Sounds good.
> 
>>
>>> +  if (has_extra_args_mechanism_p (*tp))
>>> +    {
>>> +      if (PACK_EXPANSION_P (*tp))
>>> +	gcc_checking_assert (!PACK_EXPANSION_EXTRA_ARGS (*tp));
>>> +      else if (TREE_CODE (*tp) == REQUIRES_EXPR)
>>> +	gcc_checking_assert (!REQUIRES_EXPR_EXTRA_ARGS (*tp));
>>> +      else if (TREE_CODE (*tp) == IF_STMT
>>> +	       && IF_STMT_CONSTEXPR_P (*tp))
>>> +	gcc_checking_assert (!IF_STMT_EXTRA_ARGS (*tp));
>>> +      else
>>> +	gcc_unreachable ();
>>> +    }
>>> +
>>>      if (TREE_CODE (*tp) == DECL_EXPR)
>>>        {
>>>          tree decl = DECL_EXPR_DECL (*tp);
>>> @@ -18738,7 +18751,8 @@ tsubst_stmt (tree t, tree args, tsubst_flags_t
>>> complain, tree in_decl)
>>>    	  IF_COND (stmt) = IF_COND (t);
>>>    	  THEN_CLAUSE (stmt) = THEN_CLAUSE (t);
>>>    	  ELSE_CLAUSE (stmt) = ELSE_CLAUSE (t);
>>> -	  IF_STMT_EXTRA_ARGS (stmt) = build_extra_args (t, args, complain);
>>> +	  IF_SCOPE (stmt) = NULL_TREE;
>>
>> What does IF_SCOPE have to do with this?
> 
> IF_SCOPE is the same field as IF_STMT_EXTRA_ARGS so we need to clear it
> before calling build_extra_args to avoid tripping over the added assert.

Let's clear it a few lines earlier, then, immediately after the 
poplevel; OK with that change.

finish_if_stmt clears it even before calling poplevel, but that doesn't 
seem necessary.

Jason