[3/3] gdb: add assertion when marking the remote async flag

  From: Simon Marchi <simon.marchi@efficios.com>

As reported in bug 30630 [1], we hit a case where the remote target's
async flag is marked while the target is not configured (yet) to work
async.  This should not happen.  It is caught thanks to this assert in
remote_target::wait:

    /* Start by clearing the flag that asks for our wait method to be called,
       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 ())
      rs->clear_async_event_handler ();
    else
      gdb_assert (!rs->async_event_handler_marked ());

This is helpful, but I think that we could have caught the problem earlier than
that, at the moment we marked the handler.  Catching problems earlier
makes them easier to debug.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=30630

Change-Id: I7e229c74b04da82bef6a817d5a676be5cf52e833
---
 gdb/remote.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
  

> -----Original Message-----
> From: Gdb-patches <gdb-patches-
> bounces+mikhail.terekhov=dell.com@sourceware.org> On Behalf Of Simon
> Marchi via Gdb-patches
> Sent: Tuesday, October 3, 2023 10:04 PM
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi <simon.marchi@efficios.com>
> Subject: [PATCH 3/3] gdb: add assertion when marking the remote async flag
>
>
> [EXTERNAL EMAIL]
>
> From: Simon Marchi <simon.marchi@efficios.com>
>
> As reported in bug 30630 [1], we hit a case where the remote target's async flag
> is marked while the target is not configured (yet) to work async.  This should not
> happen.  It is caught thanks to this assert in
> remote_target::wait:
>
>     /* Start by clearing the flag that asks for our wait method to be called,
>        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 ())
>       rs->clear_async_event_handler ();
>     else
>       gdb_assert (!rs->async_event_handler_marked ());
>
> This is helpful, but I think that we could have caught the problem earlier than
> that, at the moment we marked the handler.  Catching problems earlier makes
> them easier to debug.
>
> [1]
> https://urldefense.com/v3/__https://sourceware.org/bugzilla/show_bug.cgi?id
> =30630__;!!LpKI!j0fTzubsC8RVdwinGn-r82-
> mX7hLUvNffNCQWwFTCYyR9EWj0CtlY9ERyT_AXgqsNQ0ZjS4Ls53fdBnUrLhWIw
> hIBW9P$ [sourceware[.]org]
>
> Change-Id: I7e229c74b04da82bef6a817d5a676be5cf52e833
> ---
>  gdb/remote.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c index 38d0027dbf9e..7830b5cec33f
> 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -424,7 +424,10 @@ class remote_state
>    }
>
>    void mark_async_event_handler ()
> -  { ::mark_async_event_handler (m_async_event_handler_token); }
> +  {
> +    gdb_assert (this->is_async_p ());
> +    ::mark_async_event_handler (m_async_event_handler_token);  }

This change made the need for fix suggested in [1] more obvious.
The assert in mark_async_event_handler () is stronger than check in
remote_target::queued_stop_reply().
I.e. mark_async_event_handler () should not be called in
remote_target::queued_stop_reply() unless async_handler != NULL i.e.
target_is_async_p() !=0.

I'd suggest to merge in fix from [1] into this series.

>
>    void clear_async_event_handler ()
>    { ::clear_async_event_handler (m_async_event_handler_token); }
> --
> 2.42.0


Internal Use - Confidential
  

On 10/6/23 17:09, Terekhov, Mikhail wrote:
>> -----Original Message-----
>> From: Gdb-patches <gdb-patches-
>> bounces+mikhail.terekhov=dell.com@sourceware.org> On Behalf Of Simon
>> Marchi via Gdb-patches
>> Sent: Tuesday, October 3, 2023 10:04 PM
>> To: gdb-patches@sourceware.org
>> Cc: Simon Marchi <simon.marchi@efficios.com>
>> Subject: [PATCH 3/3] gdb: add assertion when marking the remote async flag
>>
>>
>> [EXTERNAL EMAIL]
>>
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> As reported in bug 30630 [1], we hit a case where the remote target's async flag
>> is marked while the target is not configured (yet) to work async.  This should not
>> happen.  It is caught thanks to this assert in
>> remote_target::wait:
>>
>>     /* Start by clearing the flag that asks for our wait method to be called,
>>        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 ())
>>       rs->clear_async_event_handler ();
>>     else
>>       gdb_assert (!rs->async_event_handler_marked ());
>>
>> This is helpful, but I think that we could have caught the problem earlier than
>> that, at the moment we marked the handler.  Catching problems earlier makes
>> them easier to debug.
>>
>> [1]
>> https://urldefense.com/v3/__https://sourceware.org/bugzilla/show_bug.cgi?id
>> =30630__;!!LpKI!j0fTzubsC8RVdwinGn-r82-
>> mX7hLUvNffNCQWwFTCYyR9EWj0CtlY9ERyT_AXgqsNQ0ZjS4Ls53fdBnUrLhWIw
>> hIBW9P$ [sourceware[.]org]
>>
>> Change-Id: I7e229c74b04da82bef6a817d5a676be5cf52e833
>> ---
>>  gdb/remote.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/remote.c b/gdb/remote.c index 38d0027dbf9e..7830b5cec33f
>> 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -424,7 +424,10 @@ class remote_state
>>    }
>>
>>    void mark_async_event_handler ()
>> -  { ::mark_async_event_handler (m_async_event_handler_token); }
>> +  {
>> +    gdb_assert (this->is_async_p ());
>> +    ::mark_async_event_handler (m_async_event_handler_token);  }
> 
> This change made the need for fix suggested in [1] more obvious.
> The assert in mark_async_event_handler () is stronger than check in
> remote_target::queued_stop_reply().
> I.e. mark_async_event_handler () should not be called in
> remote_target::queued_stop_reply() unless async_handler != NULL i.e.
> target_is_async_p() !=0.
> 
> I'd suggest to merge in fix from [1] into this series.

Yes, your fix needs to be merged independently of my series.  My series
is only to add an assertion closer to the root of the problem (which
could help catch other problems in the future).

Simon
  

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> As reported in bug 30630 [1], we hit a case where the remote target's
> async flag is marked while the target is not configured (yet) to work
> async.  This should not happen.  It is caught thanks to this assert in
> remote_target::wait:
>
>     /* Start by clearing the flag that asks for our wait method to be called,
>        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 ())
>       rs->clear_async_event_handler ();
>     else
>       gdb_assert (!rs->async_event_handler_marked ());
>
> This is helpful, but I think that we could have caught the problem earlier than
> that, at the moment we marked the handler.  Catching problems earlier
> makes them easier to debug.

Agreed.  Looked through this series, all looks good.  I had a few nits
that I reported, but otherwise:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

>
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=30630
>
> Change-Id: I7e229c74b04da82bef6a817d5a676be5cf52e833
> ---
>  gdb/remote.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 38d0027dbf9e..7830b5cec33f 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -424,7 +424,10 @@ class remote_state
>    }
>  
>    void mark_async_event_handler ()
> -  { ::mark_async_event_handler (m_async_event_handler_token); }
> +  {
> +    gdb_assert (this->is_async_p ());
> +    ::mark_async_event_handler (m_async_event_handler_token);
> +  }
>  
>    void clear_async_event_handler ()
>    { ::clear_async_event_handler (m_async_event_handler_token); }
> -- 
> 2.42.0
  

On 10/9/23 05:25, Andrew Burgess wrote:
> Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> As reported in bug 30630 [1], we hit a case where the remote target's
>> async flag is marked while the target is not configured (yet) to work
>> async.  This should not happen.  It is caught thanks to this assert in
>> remote_target::wait:
>>
>>     /* Start by clearing the flag that asks for our wait method to be called,
>>        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 ())
>>       rs->clear_async_event_handler ();
>>     else
>>       gdb_assert (!rs->async_event_handler_marked ());
>>
>> This is helpful, but I think that we could have caught the problem earlier than
>> that, at the moment we marked the handler.  Catching problems earlier
>> makes them easier to debug.
> 
> Agreed.  Looked through this series, all looks good.  I had a few nits
> that I reported, but otherwise:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks, I'll push if my CI run is clean with the changes applied.

Simon