Hi Jason, > On 10 Sep 2024, at 20:22, Jason Merrill <ja...@redhat.com> wrote: > > On 9/7/24 6:45 AM, Iain Sandoe wrote: >> As noted in the PR the action of the existing implementation was to >> treat a false value from await_suspend () as equivalent to "do not >> suspend". Actually it needs to be the equivalent of "resume" - and >> we need to restart the dispatcher - since the await_suspend() body >> could have already resumed the coroutine. >> See also https://github.com/cplusplus/CWG/issues/601 (NAD) for more >> discussion. > > I'm having trouble wrapping my head around this stuff, so I'll take your word > for it. :) It would be nice to have more full pseudocode.
It was not 100% clear to any of the implementers it seems - since all of them
got this wrong.
There are two pseudo-codes, I suppose: one that reflects the compile-time
program flow (since we have to react differently to various of the
customisation points) and one that reflects the user’s runtime flow (which also
depends on the customisation points).
Will think about how to expand this some more - maybe in a larger block at the
start of the file.
>> + /* Finish the destory dispatcher. */
>
> Typo.
fixed,
>>
>> + /* Build the dispatcher:
>> + if (resume index is odd)
>> + {
>> + switch (resume index)
>> + case 1:
>> + goto cleanup.
>> + case ... odd suspension point number
>> + .CO_ACTOR (... odd suspension point number)
>> + break;
>> + default:
>> + break;
>> + }
>> + else
>> + {
>> + coro.restart.dispatch:
>> + case 0:
>> + goto start.
>> + case ... even suspension point number
>> + .CO_ACTOR (... even suspension point number)
>> + break;
>> + default:
>> + break;
>> + }
>> + we should not get here unless something is broken badly.
>> + __builtin_trap ();
>> +*/
>
> Maybe mention in this that the odd case is destroy and the even case is
> resume?
done.
> Why is it useful to have two separate switches when they have the same
> default: behavior?
At some (reasonably soon) point I want to try adjusting the call to the actor
so that it takes
a “destroy” parameter (and then to have a small shim for the resume() call -
like the one we
have for the destroy()). The investigation there is to whether we can get
better inlining
when the compiler does not have to look through the indirection. It does not
work well at the
moment (too little inlining) and neither does it work well if we disable
inlining limits (too much).
>
>> + /* For the case of a boolean await_resume () that returns 'true' we should
>> + restart the dispatch, since we cannot know if additional resumes were
>> + executed from within the await_resume function. */
>
> Do you mean await_suspend here?
thanks, fixed.
>
> OK with at least the typo fixed.
Pushed as attached.
- some expanded pseudo-code/description to follow in due course.
thanks
Iain
0001-c-coroutines-Fix-handling-of-bool-await_suspend-PR11.patch
Description: Binary data
