Re: [PATCH Coroutines]Access promise via actor function's frame pointer argument

[bin.cheng]

On Mon, Jan 20, 2020 at 10:59 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
>
> Hi Bin,
>
> bin.cheng <bin.cheng@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.linux@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.  IMHO, the function is too long so self-contained operations worth outlined function even it's called once.

Patch updated as attached.

Thanks,
bin

gcc/cp
2020-01-20  Bin Cheng  <bin.linux@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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Use-promise-in-coroutine-frame-in-actor-function.patch
Type: application/octet-stream
Size: 5687 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20200121/1f5feb5a/attachment.obj>