[PATCHv2,5/6] gdb: add assert in remote_target::wait relating to async being off

Message ID 87c0ccf934cb31c7c636dcf0198bf128f38f821f.1637756330.git.aburgess@redhat.com
State New
Headers show
Series
  • Improve 'maint set target-async off' for remote targets
Related show

Commit Message

Mike Frysinger via Gdb-patches Nov. 24, 2021, 12:22 p.m.
While working on another patch I ended up in a situation where I had
async mode disabled (with 'maint set target-async off'), but the async
event token got marked anyway.

In this situation GDB was continually calling into
remote_target::wait, however, the async token would never become
unmarked as the unmarking is guarded by target_is_async_p.

We could just unconditionally unmark the token, but that would feel
like just ignoring a bug, so, instead, lets assert that if
!target_is_async_p, then the async token should not be marked.

This assertion would have caught my earlier mistake.

There should be no user visible changes with this commit.
---
 gdb/remote.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.25.4

Comments

Simon Marchi Nov. 24, 2021, 9:23 p.m. | #1
On 2021-11-24 7:22 a.m., Andrew Burgess via Gdb-patches wrote:
> While working on another patch I ended up in a situation where I had

> async mode disabled (with 'maint set target-async off'), but the async

> event token got marked anyway.

> 

> In this situation GDB was continually calling into

> remote_target::wait, however, the async token would never become

> unmarked as the unmarking is guarded by target_is_async_p.

> 

> We could just unconditionally unmark the token, but that would feel

> like just ignoring a bug, so, instead, lets assert that if

> !target_is_async_p, then the async token should not be marked.

> 

> This assertion would have caught my earlier mistake.

> 

> There should be no user visible changes with this commit.

> ---

>  gdb/remote.c | 6 +++++-

>  1 file changed, 5 insertions(+), 1 deletion(-)

> 

> diff --git a/gdb/remote.c b/gdb/remote.c

> index 25a4d3cab6e..da8ed81ba78 100644

> --- a/gdb/remote.c

> +++ b/gdb/remote.c

> @@ -8309,9 +8309,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,

>    remote_state *rs = get_remote_state ();

>  

>    /* Start by clearing the flag that asks for our wait method to be called,

> -     we'll mark it again at the end if needed.  */

> +     we'll mark it again at the end if needed.  If the target is not in

> +     async mode then the async token should not be marked.  */

>    if (target_is_async_p ())

>      clear_async_event_handler (rs->remote_async_inferior_event_token);

> +  else

> +    gdb_assert (!async_event_handler_marked

> +		(rs->remote_async_inferior_event_token));

>  

>    ptid_t event_ptid;

>  

> -- 

> 2.25.4

> 


LGTM.

I think the series can be merged at least up to here, I think these are
good cleanups.

Simon
Mike Frysinger via Gdb-patches Nov. 25, 2021, 10:04 a.m. | #2
* Simon Marchi <simark@simark.ca> [2021-11-24 16:23:30 -0500]:

> On 2021-11-24 7:22 a.m., Andrew Burgess via Gdb-patches wrote:

> > While working on another patch I ended up in a situation where I had

> > async mode disabled (with 'maint set target-async off'), but the async

> > event token got marked anyway.

> > 

> > In this situation GDB was continually calling into

> > remote_target::wait, however, the async token would never become

> > unmarked as the unmarking is guarded by target_is_async_p.

> > 

> > We could just unconditionally unmark the token, but that would feel

> > like just ignoring a bug, so, instead, lets assert that if

> > !target_is_async_p, then the async token should not be marked.

> > 

> > This assertion would have caught my earlier mistake.

> > 

> > There should be no user visible changes with this commit.

> > ---

> >  gdb/remote.c | 6 +++++-

> >  1 file changed, 5 insertions(+), 1 deletion(-)

> > 

> > diff --git a/gdb/remote.c b/gdb/remote.c

> > index 25a4d3cab6e..da8ed81ba78 100644

> > --- a/gdb/remote.c

> > +++ b/gdb/remote.c

> > @@ -8309,9 +8309,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,

> >    remote_state *rs = get_remote_state ();

> >  

> >    /* Start by clearing the flag that asks for our wait method to be called,

> > -     we'll mark it again at the end if needed.  */

> > +     we'll mark it again at the end if needed.  If the target is not in

> > +     async mode then the async token should not be marked.  */

> >    if (target_is_async_p ())

> >      clear_async_event_handler (rs->remote_async_inferior_event_token);

> > +  else

> > +    gdb_assert (!async_event_handler_marked

> > +		(rs->remote_async_inferior_event_token));

> >  

> >    ptid_t event_ptid;

> >  

> > -- 

> > 2.25.4

> > 

> 

> LGTM.

> 

> I think the series can be merged at least up to here, I think these are

> good cleanups.


Thanks, I made the change you suggested about one target_can_async_p
function calling the other, and pushed patches 1 to 5.

The final patch still pending.

Thanks,
Andrew
Mike Frysinger via Gdb-patches Nov. 25, 2021, 11:36 a.m. | #3
On 11/25/21 11:04 AM, Andrew Burgess via Gdb-patches wrote:
> * Simon Marchi <simark@simark.ca> [2021-11-24 16:23:30 -0500]:

> 

>> On 2021-11-24 7:22 a.m., Andrew Burgess via Gdb-patches wrote:

>>> While working on another patch I ended up in a situation where I had

>>> async mode disabled (with 'maint set target-async off'), but the async

>>> event token got marked anyway.

>>>

>>> In this situation GDB was continually calling into

>>> remote_target::wait, however, the async token would never become

>>> unmarked as the unmarking is guarded by target_is_async_p.

>>>

>>> We could just unconditionally unmark the token, but that would feel

>>> like just ignoring a bug, so, instead, lets assert that if

>>> !target_is_async_p, then the async token should not be marked.

>>>

>>> This assertion would have caught my earlier mistake.

>>>

>>> There should be no user visible changes with this commit.

>>> ---

>>>  gdb/remote.c | 6 +++++-

>>>  1 file changed, 5 insertions(+), 1 deletion(-)

>>>

>>> diff --git a/gdb/remote.c b/gdb/remote.c

>>> index 25a4d3cab6e..da8ed81ba78 100644

>>> --- a/gdb/remote.c

>>> +++ b/gdb/remote.c

>>> @@ -8309,9 +8309,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,

>>>    remote_state *rs = get_remote_state ();

>>>  

>>>    /* Start by clearing the flag that asks for our wait method to be called,

>>> -     we'll mark it again at the end if needed.  */

>>> +     we'll mark it again at the end if needed.  If the target is not in

>>> +     async mode then the async token should not be marked.  */

>>>    if (target_is_async_p ())

>>>      clear_async_event_handler (rs->remote_async_inferior_event_token);

>>> +  else

>>> +    gdb_assert (!async_event_handler_marked

>>> +		(rs->remote_async_inferior_event_token));

>>>  

>>>    ptid_t event_ptid;

>>>  

>>> -- 

>>> 2.25.4

>>>

>>

>> LGTM.

>>

>> I think the series can be merged at least up to here, I think these are

>> good cleanups.

> 

> Thanks, I made the change you suggested about one target_can_async_p

> function calling the other, and pushed patches 1 to 5.

> 


I'm running into the assert:
https://sourceware.org/bugzilla/show_bug.cgi?id=28627 .

Thanks,
- Tom

> The final patch still pending.

> 

> Thanks,

> Andrew

>
Mike Frysinger via Gdb-patches Nov. 25, 2021, 1:46 p.m. | #4
* Tom de Vries <tdevries@suse.de> [2021-11-25 12:36:07 +0100]:

> On 11/25/21 11:04 AM, Andrew Burgess via Gdb-patches wrote:

> > * Simon Marchi <simark@simark.ca> [2021-11-24 16:23:30 -0500]:

> > 

> >> On 2021-11-24 7:22 a.m., Andrew Burgess via Gdb-patches wrote:

> >>> While working on another patch I ended up in a situation where I had

> >>> async mode disabled (with 'maint set target-async off'), but the async

> >>> event token got marked anyway.

> >>>

> >>> In this situation GDB was continually calling into

> >>> remote_target::wait, however, the async token would never become

> >>> unmarked as the unmarking is guarded by target_is_async_p.

> >>>

> >>> We could just unconditionally unmark the token, but that would feel

> >>> like just ignoring a bug, so, instead, lets assert that if

> >>> !target_is_async_p, then the async token should not be marked.

> >>>

> >>> This assertion would have caught my earlier mistake.

> >>>

> >>> There should be no user visible changes with this commit.

> >>> ---

> >>>  gdb/remote.c | 6 +++++-

> >>>  1 file changed, 5 insertions(+), 1 deletion(-)

> >>>

> >>> diff --git a/gdb/remote.c b/gdb/remote.c

> >>> index 25a4d3cab6e..da8ed81ba78 100644

> >>> --- a/gdb/remote.c

> >>> +++ b/gdb/remote.c

> >>> @@ -8309,9 +8309,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,

> >>>    remote_state *rs = get_remote_state ();

> >>>  

> >>>    /* Start by clearing the flag that asks for our wait method to be called,

> >>> -     we'll mark it again at the end if needed.  */

> >>> +     we'll mark it again at the end if needed.  If the target is not in

> >>> +     async mode then the async token should not be marked.  */

> >>>    if (target_is_async_p ())

> >>>      clear_async_event_handler (rs->remote_async_inferior_event_token);

> >>> +  else

> >>> +    gdb_assert (!async_event_handler_marked

> >>> +		(rs->remote_async_inferior_event_token));

> >>>  

> >>>    ptid_t event_ptid;

> >>>  

> >>> -- 

> >>> 2.25.4

> >>>

> >>

> >> LGTM.

> >>

> >> I think the series can be merged at least up to here, I think these are

> >> good cleanups.

> > 

> > Thanks, I made the change you suggested about one target_can_async_p

> > function calling the other, and pushed patches 1 to 5.

> > 

> 

> I'm running into the assert:

> https://sourceware.org/bugzilla/show_bug.cgi?id=28627 .


Sorry about that.  I've reverted this patch, and closed the bug.

The cause of the assert is fixed by patch #6 in this series, so I
guess I'll remerge this patch after patch #6 lands.

Thanks,
Andrew
Mike Frysinger via Gdb-patches Nov. 25, 2021, 2:17 p.m. | #5
On 11/25/21 2:46 PM, Andrew Burgess wrote:
> * Tom de Vries <tdevries@suse.de> [2021-11-25 12:36:07 +0100]:

> 

>> On 11/25/21 11:04 AM, Andrew Burgess via Gdb-patches wrote:

>>> * Simon Marchi <simark@simark.ca> [2021-11-24 16:23:30 -0500]:

>>>

>>>> On 2021-11-24 7:22 a.m., Andrew Burgess via Gdb-patches wrote:

>>>>> While working on another patch I ended up in a situation where I had

>>>>> async mode disabled (with 'maint set target-async off'), but the async

>>>>> event token got marked anyway.

>>>>>

>>>>> In this situation GDB was continually calling into

>>>>> remote_target::wait, however, the async token would never become

>>>>> unmarked as the unmarking is guarded by target_is_async_p.

>>>>>

>>>>> We could just unconditionally unmark the token, but that would feel

>>>>> like just ignoring a bug, so, instead, lets assert that if

>>>>> !target_is_async_p, then the async token should not be marked.

>>>>>

>>>>> This assertion would have caught my earlier mistake.

>>>>>

>>>>> There should be no user visible changes with this commit.

>>>>> ---

>>>>>  gdb/remote.c | 6 +++++-

>>>>>  1 file changed, 5 insertions(+), 1 deletion(-)

>>>>>

>>>>> diff --git a/gdb/remote.c b/gdb/remote.c

>>>>> index 25a4d3cab6e..da8ed81ba78 100644

>>>>> --- a/gdb/remote.c

>>>>> +++ b/gdb/remote.c

>>>>> @@ -8309,9 +8309,13 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,

>>>>>    remote_state *rs = get_remote_state ();

>>>>>  

>>>>>    /* Start by clearing the flag that asks for our wait method to be called,

>>>>> -     we'll mark it again at the end if needed.  */

>>>>> +     we'll mark it again at the end if needed.  If the target is not in

>>>>> +     async mode then the async token should not be marked.  */

>>>>>    if (target_is_async_p ())

>>>>>      clear_async_event_handler (rs->remote_async_inferior_event_token);

>>>>> +  else

>>>>> +    gdb_assert (!async_event_handler_marked

>>>>> +		(rs->remote_async_inferior_event_token));

>>>>>  

>>>>>    ptid_t event_ptid;

>>>>>  

>>>>> -- 

>>>>> 2.25.4

>>>>>

>>>>

>>>> LGTM.

>>>>

>>>> I think the series can be merged at least up to here, I think these are

>>>> good cleanups.

>>>

>>> Thanks, I made the change you suggested about one target_can_async_p

>>> function calling the other, and pushed patches 1 to 5.

>>>

>>

>> I'm running into the assert:

>> https://sourceware.org/bugzilla/show_bug.cgi?id=28627 .

> 

> Sorry about that.  I've reverted this patch, and closed the bug.

> 


Np :)

Thanks for the quick fix.
- Tom

> The cause of the assert is fixed by patch #6 in this series, so I

> guess I'll remerge this patch after patch #6 lands.

> 

> Thanks,

> Andrew

>

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index 25a4d3cab6e..da8ed81ba78 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -8309,9 +8309,13 @@  remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
   remote_state *rs = get_remote_state ();
 
   /* Start by clearing the flag that asks for our wait method to be called,
-     we'll mark it again at the end if needed.  */
+     we'll mark it again at the end if needed.  If the target is not in
+     async mode then the async token should not be marked.  */
   if (target_is_async_p ())
     clear_async_event_handler (rs->remote_async_inferior_event_token);
+  else
+    gdb_assert (!async_event_handler_marked
+		(rs->remote_async_inferior_event_token));
 
   ptid_t event_ptid;