Re: [PATCH][gdb] Fix assert in remote_async_get_pending_events_handler

[Tom de Vries <tdevries@suse.de>]

On 4/22/21 12:19 PM, Andrew Burgess wrote:
> * Tom de Vries <tdevries@suse.de> [2021-04-22 10:51:29 +0200]:
> 
>> Hi,
>>
>> Occassionally I run into the following assert:
>> ...
>> (gdb) PASS: gdb.multi/multi-target-continue.exp: inferior 5
>> Remote debugging from host ::1, port 49990^M
>> Process multi-target-continue created; pid = 31241^M
>> src/gdb/remote-notif.c:113: internal-error: \
>>   void remote_async_get_pending_events_handler(gdb_client_data): \
>>   Assertion `target_is_non_stop_p ()' failed.^M
>> ...
>>
>> The assert checks target_is_non_stop_p, which is related to the current
>> target.
>>
>> Fix this by changing the assert such that it checks non-stopness related to
>> the event it's handling.
>>
>> Tested on x86_64-linux.
>>
>> Any comments?
> 
> This seems fine to me.  I wonder though if you considered converting
> target_is_non_stop_p into a member function on target_ops?
> If we did
> then we would avoid having to switch targets just to ask this
> question.  All of the helper functions that target_is_non_stop_p calls
> are already available as member functions so there would be no
> additional changes needed I think.
> 

Um, I'm the one who ran into the problem, Simon is the one who came up
with the fix, so I guess this is a question for him.  I'm afraid I'm not
familiar with this code at all.

Thanks,
- Tom

> Just a thought.
> 
> Thanks,
> Andrew
> 
>>
>> Thanks,
>> - Tom
>>
>> [gdb] Fix assert in remote_async_get_pending_events_handler
>>
>> 2021-04-16  Simon Marchi  <simon.marchi@polymtl.ca>
>> 	    Tom de Vries  <tdevries@suse.de>
>>
>> 	PR remote/27710
>> 	* remote.c (remote_target_is_non_stop_p): New function.
>> 	* remote.h (remote_target_is_non_stop_p): Declare.
>> 	* remote-notif.c (remote_async_get_pending_events_handler): Fix assert
>> 	to check non-stopness using notif_state->remote rather current target.
>>
>> ---
>>  gdb/remote-notif.c |  2 +-
>>  gdb/remote.c       | 11 +++++++++++
>>  gdb/remote.h       |  1 +
>>  3 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
>> index 5a3e1395b76..4245015ae37 100644
>> --- a/gdb/remote-notif.c
>> +++ b/gdb/remote-notif.c
>> @@ -110,7 +110,7 @@ remote_async_get_pending_events_handler (gdb_client_data data)
>>  {
>>    remote_notif_state *notif_state = (remote_notif_state *) data;
>>    clear_async_event_handler (notif_state->get_pending_events_token);
>> -  gdb_assert (target_is_non_stop_p ());
>> +  gdb_assert (remote_target_is_non_stop_p (notif_state->remote));
>>    remote_notif_process (notif_state, NULL);
>>  }
>>  
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 7429e1a86b3..2e365df9bba 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -14730,6 +14730,17 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
>>    return packet_check_result (rs->buf.data ()) == PACKET_OK;
>>  }
>>  
>> +/* Return true if remote target T is non-stop.  */
>> +
>> +bool
>> +remote_target_is_non_stop_p (remote_target *t)
>> +{
>> +  scoped_restore_current_thread restore_thread;
>> +  switch_to_target_no_thread (t);
>> +
>> +  return target_is_non_stop_p ();
>> +}
>> +
>>  #if GDB_SELF_TEST
>>  
>>  namespace selftests {
>> diff --git a/gdb/remote.h b/gdb/remote.h
>> index 18352ddb866..46bfa01fc79 100644
>> --- a/gdb/remote.h
>> +++ b/gdb/remote.h
>> @@ -77,4 +77,5 @@ extern int remote_register_number_and_offset (struct gdbarch *gdbarch,
>>  
>>  extern void remote_notif_get_pending_events (remote_target *remote,
>>  					     struct notif_client *np);
>> +extern bool remote_target_is_non_stop_p (remote_target *t);
>>  #endif