> On 14 Aug 2024, at 05:46, Jason Merrill <ja...@redhat.com> wrote:
>
> On 8/13/24 7:52 PM, Patrick Palka wrote:
>> On Tue, 13 Aug 2024, Jason Merrill wrote:
>>> On 8/12/24 10:01 PM, Patrick Palka wrote:
>>>> Tested on x86_64-pc-linux-gnu, does this look OK for trunk/14?
>>>>
>>>> -- >8 --
>>>>
>>>> In r15-2210 we got rid of the unnecessary cast to lvalue reference when
>>>> passing *this to the promise type ctor, and as a drive-by change we also
>>>> simplified the code to use cp_build_fold_indirect_ref.
>>>>
>>>> But cp_build_fold_indirect_ref apparently does too much here, namely
>>>> it has a shortcut for returning current_class_ref if the operand is
>>>> current_class_ptr. The problem with that shortcut is current_class_ref
>>>> might have gotten clobbered earlier if it appeared in the function body,
>>>> since rewrite_param_uses walks and rewrites in-place all local variable
>>>> uses to their corresponding frame copy.
>>>>
>>>> So later this cp_build_fold_indirect_ref for *__closure will instead return
>>>> the mutated current_class_ref i.e. *frame_ptr->__closure, which doesn't
>>>> make sense here since we're in the ramp function and not the actor function
>>>> where frame_ptr is in scope.
>>>>
>>>> This patch fixes this by building INDIRECT_REF directly instead of using
>>>> cp_build_fold_indirect_ref. (Another approach might be to restore an
>>>> unshare_expr'd current_class_ref after doing coro_rewrite_function_body
>>>> to avoid it remaining clobbered after the rewriting process. Yet
>>>> another more ambitious approach might be to avoid this tree sharing in
>>>> the first place by returning unshared versions of current_class_ref from
>>>> maybe_dummy_object etc.)
>>>
>>> Maybe clear current_class_ptr/ref in coro rewriting so we don't hit the
>>> shortcut?
>> That seems to work, but I'm kind of worried about what other code paths
>> that'd disable, particularly semantic code paths vs just optimizations
>> code paths such as the cp_build_fold_indirect_ref shortcut. IIUC the
>> ramp function has the same signature as the original presumably non-static
>> member function so ideally current class ref should remain set when
>> building the ramp function body and cleared only when building/rewriting
>> the actor function body (which is never a non-static member function and
>> so doesn't have a this pointer, I think?).
Yes, that is what I expect to be needed, and …
>> We do the actor body stuff first however, so even if we clear
>> current_class_ref then, the restored current_class_ref during the
>> later ramp function body stuff (including during the call to
>> cp_build_fold_indirect_ref) will still be clobbered :(
… I have a patch set (soon to be posted) that splits the analysis (needed to
complete the ramp) and the synthesis (of the actor / destroy) so that the latter
can happen after the context of the ramp is exited. (i.e. right at the end of
finish_function and thus the sythesis can be treated as a stand-alone definition
this resolves similar issues with global state that we saw with Arsen’s patch to
resolve label contexts).
Iain
>> So ISTM this more narrow approach might be preferable unless we ever run
>> into another instance of this current_class_ref clobbering issue?
>
> Fair enough.
> Is there a reason not to use build_fold_indirect_ref (without cp_)?
>
> Jason
>
