Hi Nathan, Bin, bin.cheng <bin.ch...@linux.alibaba.com> wrote:
> On Mon, Jan 20, 2020 at 10:59 PM Iain Sandoe <i...@sandoe.co.uk> wrote:
>> Hi Bin,
>>
>> bin.cheng <bin.ch...@linux.alibaba.com> wrote:
>>
>>> By standard, coroutine body should be encapsulated in try-catch block
>>> as following:
>>> try {
>>> // coroutine body
>>> } catch(...) {
>>> promise.unhandled_exception();
>>> }
>>> Given above try-catch block is implemented in the coroutine actor
>>> function called by coroutine ramp function, so the promise should
>>> be accessed via actor function's coroutine frame pointer argument,
>>> rather than the ramp function's coroutine frame variable.
>>
>> thanks, good catch!
>> it has not triggered for me (including on some more complex test-cases that
>> are not useable
>> in the testsuite).
>>
>>> This patch also refactored code to make the fix easy, unfortunately,
>>
>> see below,
>>
>>> I failed to reduce a test case from cpproro.
>>
>> it would be good if we could try to find a reproducer. I’m happy to try and
>> throw
>> creduce at a preprocessed file, if you have one.
>>
>>> gcc/cp
>>> 2020-01-20 Bin Cheng <bin.li...@linux.alibaba.com>
>>>
>>> * coroutines.cc (act_des_fn, wrap_coroutine_body): New.
>>> (morph_fn_to_coro): Call act_des_fn to build actor/destroy decls, as
>>> well as access promise via actor function's frame pointer argument.
>>> Refactor code into above functions.
>>> (build_actor_fn, build_destroy_fn): Use frame pointer argument
>> + /* We still try to look for the promise method and warn if it's not
>> + present. */
>> + tree ueh_meth
>> + = lookup_promise_method (orig, coro_unhandled_exception_identifier,
>> + loc, /*musthave=*/ false);
>> + if (!ueh_meth || ueh_meth == error_mark_node)
>> + warning_at (loc, 0, "no member named %qE in %qT",
>> + coro_unhandled_exception_identifier,
>> + get_coroutine_promise_type (orig));
>> + }
>> + /* Else we don't check and don't care if the method is missing. */
>> +
>> + return fnbody;
>> +}
>>
>> IMO this ^^^ obfuscates the fix, and I don’t think it should be done at the
>> same time.
> Sure, given we are in this stage, I split the patch and leave refactoring to
> future.
Thank you, this is easier to see.
> IMHO, the function is too long so self-contained operations worth outlined
> function even it's called once.
There are two changes “in the pipeline” that might make an alternate split
better:
1) There is a change to wrap the await_resume() for initial suspend in a
try-catch, this probably means
that it would be better to partition the code that looks up
unhandled_exception (), since that would then
probably be used twice.
2) There is on-going discussion about how to guarantee tail-calling in
symmetric transfers. At present,
ISTM that this will need a second wrapper around the whole resume() function.
I would prefer to leave any refactoring until these two things are clarified,
does that make sense?
----
Nathan, is this OK for trunk as-is?
thanks
Iain
> Patch updated as attached.
>
> Thanks,
> bin
>
> gcc/cp
> 2020-01-20 Bin Cheng <bin.li...@linux.alibaba.com>
> * coroutines.cc (act_des_fn): New.
> (morph_fn_to_coro): Call act_des_fn to build actor/destroy decls.
> Access promise via actor function's frame pointer argument.
> (build_actor_fn, build_destroy_fn): Use frame pointer argument.
0001-Use-promise-in-coroutine-frame-in-actor-function.patch
Description: Binary data
