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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-04-22 09:33:28 -0400]:

> On 2021-04-22 7:03 a.m., Tom de Vries wrote:
> > 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.
> 
> This was proposed in the bug as well:
> 
>   https://sourceware.org/bugzilla/show_bug.cgi?id=27710#c16
> 
> I am not against it, but I think Tom's patch is OK, given it follows
> current practices.

I agree, I'm certainly not blocking the patch.

Thanks,
Andrew