> On 9 Dec 2024, at 19:34, Jason Merrill <ja...@redhat.com> wrote:
>
> On 12/9/24 2:00 PM, Iain Sandoe wrote:
>>> On 9 Dec 2024, at 18:56, Jason Merrill <ja...@redhat.com> wrote:
>>>
>>> On 11/29/24 8:47 AM, Iain Sandoe wrote:
>>>> Tested on x86_64-darwin, x86_64-linux,
>>>> OK for trunk?
>>>> thanks
>>>> Iain
>>>> --- 8< ---
>>>> At present, we only update the suspend index when we actually are
>>>> at the stage that the coroutine is considered suspended. This is
>>>> on the basis that it is UB to resume or destroy a coroutines that
>>>> is not suspended (and therefore we never need to access this value
>>>> otherwise). However, it is possible that someone could set a debug
>>>> breakpoint on the resume which can be reached without suspending
>>>> if await_ready() returns true.
>>>
>>> Hmm, https://eel.is/c++draft/expr#await-5 reads to me like there's only a
>>> suspend point if await_ready() returns false?
>> yes, exactly.
>> which would mean that we only updated the suspend index in that case (which
>> is fine if the only observer of the index is the coroutines code).
>> This change is entirely to cater for the fact that a debugger can observe
>> the coroutine in states other than those that can be reached from resume()
>> or destroy().
>
> I guess I'm getting confused by the comment referring to a suspend point,
> where actually we're dealing with the resume index, which is independent of
> the suspend point?
Yes - although the coroutines code only ever inspects it after a suspension - a
debugger could inspect it at any time and, logically, it should icrement as we
pass each “if (awaiter.await_ready())” independent of whether that results in
a suspension.
>
>>>> In that case, the debugger would
>>>> read an incorrect suspend index. Fixed by moving the update to
>>>> just before the test for ready.
>>>> gcc/cp/ChangeLog:
>>>> * coroutines.cc (expand_one_await_expression): Update the
>>>> suspend point index before checking if the coroutine is
>>>> ready.
>>>> Signed-off-by: Iain Sandoe <i...@sandoe.co.uk>
>>>> ---
>>>> gcc/cp/coroutines.cc | 9 +++++----
>>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc
>>>> index 3148559d208..b36730d793c 100644
>>>> --- a/gcc/cp/coroutines.cc
>>>> +++ b/gcc/cp/coroutines.cc
>>>> @@ -1964,6 +1964,11 @@ expand_one_await_expression (tree *expr, tree
>>>> *await_expr, void *d)
>>>> /* Use the await_ready() call to test if we need to suspend. */
>>>> tree ready_cond = TREE_VEC_ELT (awaiter_calls, 0); /* await_ready(). */
>>>> + /* We are about to pass this suspend point. */
>>>> + tree susp_idx = build_int_cst (short_unsigned_type_node, data->index);
>>>> + tree r = cp_build_init_expr (data->resume_idx, susp_idx);
>>>> + finish_expr_stmt (r);
>>>> +
>>>> /* Convert to bool, if necessary. */
>>>> if (TREE_CODE (TREE_TYPE (ready_cond)) != BOOLEAN_TYPE)
>>>> ready_cond = cp_convert (boolean_type_node, ready_cond,
>>>> @@ -1974,10 +1979,6 @@ expand_one_await_expression (tree *expr, tree
>>>> *await_expr, void *d)
>>>> ready_cond = invert_truthvalue_loc (loc, ready_cond);
>>>> finish_if_stmt_cond (ready_cond, susp_if);
>>>> - tree susp_idx = build_int_cst (short_unsigned_type_node, data->index);
>>>> - tree r = cp_build_init_expr (data->resume_idx, susp_idx);
>>>> - finish_expr_stmt (r);
>>>> -
>>>> /* Find out what we have to do with the awaiter's suspend method.
>>>> [expr.await]
>>>> (5.1) If the result of await-ready is false, the coroutine is
>>>> considered
