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

[Simon Marchi <simon.marchi@polymtl.ca>]

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