[PATCH] [PR gdb/30630] Fix internal-error in remote_target::wait

[PATCH] [PR gdb/30630] Fix internal-error in remote_target::wait

[Mikhail Terekhov]

From: Mikhail Terekhov <mikhail.terekhov@emc.com>

[PR gdb/30630] Assert inroduced in 32b1f5e8d6b8ddd3be6e471c26dd85a1dac31dda
causes gdb to fail when connecting to remote server in async
nonstop mode. This fix replaces call to target_can_async_p()
by target_is_async_p() in remote_target::queued_stop_reply()
as suggested by Christian Prochaska in comment6 to this PR.
---
 gdb/remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 9bb4f1de598..a3221b6b5d1 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7625,7 +7625,7 @@ remote_target::queued_stop_reply (ptid_t ptid)
   remote_state *rs = get_remote_state ();
   struct stop_reply *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.  */
       mark_async_event_handler (rs->remote_async_inferior_event_token);
-- 
2.35.3

Re: [PATCH] [PR gdb/30630] Fix internal-error in remote_target::wait

[Simon Marchi]

On 10/3/23 13:26, Mikhail Terekhov via Gdb-patches wrote:
> From: Mikhail Terekhov <mikhail.terekhov@emc.com>
> 
> [PR gdb/30630] Assert inroduced in 32b1f5e8d6b8ddd3be6e471c26dd85a1dac31dda
> causes gdb to fail when connecting to remote server in async
> nonstop mode. This fix replaces call to target_can_async_p()
> by target_is_async_p() in remote_target::queued_stop_reply()
> as suggested by Christian Prochaska in comment6 to this PR.

Hi Mikhail,

Can you please add a bit more details about the problem and the fix in
to the commit message to make things more obvious for readers?  My
understanding of it is: the important point in the reproducer is that
when gdbserver attaches the process, the two threads are stopped and
will generate stop reply upon connection.  After consuming the first
stop reply, the remote target will try to mark its async flag because
there is another stop reply in the queue.  But it shouldn't do that
right now, because the target hasn't yet activated its async mode, as
it's in the early stages of connection.

> ---
>  gdb/remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 9bb4f1de598..a3221b6b5d1 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -7625,7 +7625,7 @@ remote_target::queued_stop_reply (ptid_t ptid)
>    remote_state *rs = get_remote_state ();
>    struct stop_reply *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.  */
>        mark_async_event_handler (rs->remote_async_inferior_event_token);
> -- 
> 2.35.3
> 

I suppose we could catch the problem earlier than remote_target::wait.
Right here it should not be allowed to mark the async handler while the
target isn't async.

Your change in itself makes sense to me.  Could you turn the reproducer
from the PR into a test, so we make sure the use case works and doesn't
regress?  You might need to use some less known features of the
testsuite to launch gdbserver directly and connect to it, but you can
probably inspire yourself from test gdb.server/attach-flag.exp (or just
enhance that one).

Actually, when I try to reproduce, I hit this assertion a bit further:

    void
    thread_info::set_pending_waitstatus (const target_waitstatus &ws)
    {
      gdb_assert (!this->has_pending_waitstatus ());

      ...
    }

From my quick debugging it seems like gdbserver is sending two stop
replies for the main thread, there is something fishy going on there.

Thanks,

Simon