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?).
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 :(
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
