| Message ID | 20241129134721.52008-1-iain@sandoe.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | c++, coroutines: Make suspend index consistent for debug. | expand |
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? > 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 <iain@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
> On 9 Dec 2024, at 18:56, Jason Merrill <jason@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(). Iain > >> 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 <iain@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 >
On 12/9/24 2:00 PM, Iain Sandoe wrote: > > >> On 9 Dec 2024, at 18:56, Jason Merrill <jason@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? >>> 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 <iain@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 >> >
> On 9 Dec 2024, at 19:34, Jason Merrill <jason@redhat.com> wrote: > > On 12/9/24 2:00 PM, Iain Sandoe wrote: >>> On 9 Dec 2024, at 18:56, Jason Merrill <jason@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 <iain@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
On 12/9/24 2:39 PM, Iain Sandoe wrote: > > >> On 9 Dec 2024, at 19:34, Jason Merrill <jason@redhat.com> wrote: >> >> On 12/9/24 2:00 PM, Iain Sandoe wrote: >>>> On 9 Dec 2024, at 18:56, Jason Merrill <jason@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. So can we change the comment and variable name to refer to resume index instead of suspend? OK with that adjustment. >> >>>>> 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 <iain@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 >
> On 10 Dec 2024, at 01:02, Jason Merrill <jason@redhat.com> wrote: > > On 12/9/24 2:39 PM, Iain Sandoe wrote: >>> On 9 Dec 2024, at 19:34, Jason Merrill <jason@redhat.com> wrote: >>> >>> On 12/9/24 2:00 PM, Iain Sandoe wrote: >>>>> On 9 Dec 2024, at 18:56, Jason Merrill <jason@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. > > So can we change the comment and variable name to refer to resume index instead of suspend? OK with that adjustment. Yes, that makes more sense, pushed as attached. thanks Iain
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
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. 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 <iain@sandoe.co.uk> --- gcc/cp/coroutines.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)