[PATCH] gdb/remote: fix assertion failure during startup

[PATCH] gdb/remote: fix assertion failure during startup

[Trung Nguyen]

Replace the check for `target_can_async_p` in
`remote_target::queued_stop_reply` with `target_is_async_p`.

During early startup, `process_initial_stop_replies` was called before
asynchronous mode was enabled. The function then called
`remote_target::queued_stop_reply`, which then tried to mark the event
handler on all targets claiming async support, regardless of whether
async mode had been enabled or not. This triggered an assertion fail if
there had been queued stop replies.
---
 gdb/remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 4948a54e322..0bc2fea14ed 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7908,7 +7908,7 @@ remote_target::queued_stop_reply (ptid_t ptid)
   remote_state *rs = get_remote_state ();
   stop_reply_up r = remote_notif_remove_queued_reply (ptid);
 
-  if (!rs->stop_reply_queue.empty () && target_can_async_p ())
+  if (!rs->stop_reply_queue.empty () && target_is_async_p ())
     {
       /* There's still at least an event left.  */
       rs->mark_async_event_handler ();
-- 
2.39.1.windows.1

Re: [PATCH] gdb/remote: fix assertion failure during startup

[Simon Marchi]

On 7/29/24 9:11 AM, Trung Nguyen wrote:
> Replace the check for `target_can_async_p` in
> `remote_target::queued_stop_reply` with `target_is_async_p`.
> 
> During early startup, `process_initial_stop_replies` was called before
> asynchronous mode was enabled. The function then called
> `remote_target::queued_stop_reply`, which then tried to mark the event
> handler on all targets claiming async support, regardless of whether
> async mode had been enabled or not. This triggered an assertion fail if
> there had been queued stop replies.

It is useful to include the procedure to reproduce the bug in the commit
message.  Also, when an assert is hit, paste the assertion message.  A
backtrace at that point is also nice.  Basically, as much info as
possible for a reviewer of future reader of git log to understand this
change.

> ---
>  gdb/remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 4948a54e322..0bc2fea14ed 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -7908,7 +7908,7 @@ remote_target::queued_stop_reply (ptid_t ptid)
>    remote_state *rs = get_remote_state ();
>    stop_reply_up r = remote_notif_remove_queued_reply (ptid);
>  
> -  if (!rs->stop_reply_queue.empty () && target_can_async_p ())
> +  if (!rs->stop_reply_queue.empty () && target_is_async_p ())
>      {
>        /* There's still at least an event left.  */
>        rs->mark_async_event_handler ();
> -- 
> 2.39.1.windows.1
> 

I had a feeling of déjà vu with this patch, because there was a similar
patch a few months ago:

https://inbox.sourceware.org/gdb-patches/20231003172627.23077-1-Mikhail.Terekhov@dell.com/

I gave some comments but we never heard back from the contributor.  It
is not fresh in my memory, but I suppose my comments are still relevant,
so please give that a look.

Simon

Re: [PATCH] gdb/remote: fix assertion failure during startup

[Simon Marchi]

Re-adding the gdb-patches list.  Please keep discussions public, unless
truly necessary.

On 7/29/24 10:06 AM, Trung Nguyen wrote:
> On Mon, Jul 29, 2024 at 11:30 PM Simon Marchi <simark@simark.ca> wrote:
>>
>> On 7/29/24 9:11 AM, Trung Nguyen wrote:
>>> Replace the check for `target_can_async_p` in
>>> `remote_target::queued_stop_reply` with `target_is_async_p`.
>>>
>>> During early startup, `process_initial_stop_replies` was called before
>>> asynchronous mode was enabled. The function then called
>>> `remote_target::queued_stop_reply`, which then tried to mark the event
>>> handler on all targets claiming async support, regardless of whether
>>> async mode had been enabled or not. This triggered an assertion fail if
>>> there had been queued stop replies.
>>
>> It is useful to include the procedure to reproduce the bug in the commit
>> message.  Also, when an assert is hit, paste the assertion message.  A
>> backtrace at that point is also nice.  Basically, as much info as
>> possible for a reviewer of future reader of git log to understand this
>> change.
>>
> 
> The bug only reproduces on an official GDB build connected to an unofficial
> GDB remote with support for a new target. How should I mention that?

You could mention it just like that, "this failed assert happens when
connecting to XYZ".  But most importantly, you should describe what in
particular, in the GDB <-> remote conversation, causes GDB to reach this
invalid state.  In the analysis in bug 30630, I think it was determined
that it was because there were two initial stop replies (instead of one,
typically).  Is it what happens in your case?  You could also paste some
relevant parts of the remote protocol chit-chat ("set debug remote 1").
So even if we don't have access to your stub, we can at least understand
what happens GDB-side.

But if your case is really the same as bug 30630, you should just re-use
the reproducer in that bug for your commit message.  It uses gdbserver,
so anyone can run it.

> Otherwise, the bug can be reproduced by invoking GDB with
> `gdb -ex "set non-stop on" -ex "target remote $HOST:$PORT"`.

This line alone is clearly not sufficient, it also depends on what the
remote side replies.

> Furthermore, the backtrace is very long, should I include all of that in the
> commit message?

Yes, don't worry about the length.  Although I usually try to trim the
irrelevant beginning and end of the backtrace, so it's easier to focus
on the important frames.  In your case, anything above
"internal_error_loc" and anything below "execute_command" can be
removed.

Simon

Re: [PATCH] gdb/remote: fix assertion failure during startup

[Trung Nguyen]

On Tue, Jul 30, 2024 at 12:50 AM Simon Marchi <simark@simark.ca> wrote:
>
> Re-adding the gdb-patches list.  Please keep discussions public, unless
> truly necessary.

Sorry, wasn't familiar with all of this and pressed "Reply" instead of
"Reply All".
I didn't mean to make this personal or private.

> You could mention it just like that, "this failed assert happens when
> connecting to XYZ".  But most importantly, you should describe what in
> particular, in the GDB <-> remote conversation, causes GDB to reach this
> invalid state.  In the analysis in bug 30630, I think it was determined
> that it was because there were two initial stop replies (instead of one,
> typically).  Is it what happens in your case?  You could also paste some
> relevant parts of the remote protocol chit-chat ("set debug remote 1").
> So even if we don't have access to your stub, we can at least understand
> what happens GDB-side.

I've pushed a V2 of the patch with a more detailed commit message and
another place that turned out to require a `target_is_async_p` because of
a stop event during early inferior startup.

Thanks,
Trung