Re: [PATCH 02/18] Remote all-stop-on-top-of-non-stop

Pedro Alves
2015-11-23T15:40:40Z

Hi Yao,

Thanks for the review, and sorry for the delay in getting
back to this...

On 10/23/2015 04:30 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>> @@ -2636,8 +2633,37 @@ attach_command_post_wait (char *args, int from_tty, int async_exec)
>> target_post_attach (ptid_get_pid (inferior_ptid));
>>
>> post_create_inferior (&current_target, from_tty);
>> +}
>> +
>> +/* What to do after the first program stops after attaching. */
>> +enum attach_post_wait_mode
>> +{
>> + /* Do nothing. Leaves threads as they are. */
>> + ATTACH_POST_WAIT_NOTHING,
>> +
>> + /* Re-resume threads that are marked running. */
>> + ATTACH_POST_WAIT_RESUME,
>> +
>> + /* Stop all threads. */
>> + ATTACH_POST_WAIT_STOP,
>> +};
>> +
>> +/* Called after we've attached to a process and we've seen it stop for
>> + the first time. If ASYNC_EXEC is true, re-resume threads that
>> + should be running. Else if ATTACH, */
>> +
>
> Comments are not complete.

Whoops, at some point I was going to add an 'attach' parameter, then I
converted it to the new enum. Fixed.

>
>> +
>> + /* Now go over all threads that are stopped, and print their current
>> + frame. If all-stop, then if there's a signalled thread, pick
>> + that as current. */
>
> Is it the code you described in the last paragraph of the commit log?

Yes.

>
>> + ALL_NON_EXITED_THREADS (thread)
>> + {
>> + struct target_waitstatus *ws;
>> +
>> + if (first == NULL)
>> + first = thread;
>> +
>> + if (!non_stop)
>> + set_running (thread->ptid, 0);
>> + else if (thread->state != THREAD_STOPPED)
>> + continue;
>> +
>> + ws = &thread->suspend.waitstatus;
>> +
>> + if (selected == NULL
>> + && thread->suspend.waitstatus_pending_p)
>> + selected = thread;
>> +
>> + if (lowest == NULL || thread->num < lowest->num)
>> + lowest = thread;
>> +
>> + if (non_stop)
>> + print_one_stopped_thread (thread);
>> + }
>> +
>> + /* In all-stop, we only print the status of one thread, and leave
>> + others with their status pending. */
>> + if (!non_stop)
>> + {
>> + thread = selected;
>> + if (thread == NULL)
>> + thread = lowest;
>> + if (thread == NULL)
>> + thread = first;
>
> Looks lowest can't be NULL, so first isn't used.

"lowest" here actually means "lowest stopped". It can thus be NULL if
all threads are running. I've renamed it to 'lowest_stopped' now
to make that clearer.

>
>> +
>> + print_one_stopped_thread (thread);
>> + }
>> +
>> + /* For "info program". */
>> + thread = inferior_thread ();
>> + if (thread->state == THREAD_STOPPED)
>> + set_last_target_status (inferior_ptid, thread->suspend.waitstatus);
>> }
>>
>> static void
>> @@ -3826,7 +3942,7 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
>
>> @@ -12936,11 +13040,15 @@ remote_async (struct target_ops *ops, int enable)
>> event loop to process them. */
>> if (!QUEUE_is_empty (stop_reply_p, stop_reply_queue))
>> mark_async_event_handler (remote_async_inferior_event_token);
>> + if (target_is_non_stop_p ())
>> + mark_async_event_handler (rs->notif_state->get_pending_events_token);
>
> I don't understand why do we need to mark the pending event token.
> Previously, we only need to do so when GDB sees a notification.

Thanks for pointing this out. I should have added some comments here,
as it took me a while to re-understand it. :-)

To explain why I added this, I need to start with the "else" branch ...

>
>> }
>> else
>> {
>> serial_async (rs->remote_desc, NULL, NULL);
>> clear_async_event_handler (remote_async_inferior_event_token);
>> + if (target_is_non_stop_p ())
>> + clear_async_event_handler (rs->notif_state->get_pending_events_token);

... here.

Currently, it's possible that target_async(0) is called while the
pending event token is marked. If we clear the pending event token when
disabling async, we need to mark it again when re-enabling async. To simplify
(and avoid keeping track of whether the token was marked), I just always
marked the token when enabling async. It just results in at most one
harmless spurious event-loop wake up.

That explains why we now need to mark the pending event token
while previously we didn't, but not why do we need to _clear_ the
pending event token while previously we didn't. To understand
that, it helps to identify the target_async(0) call in question. That's
called after hitting a breakpoint and reporting an event to the
user (in inf-loop.c).

case INF_EXEC_COMPLETE:
if (!non_stop)
{
/* Unregister the inferior from the event loop. This is done
so that when the inferior is not running we don't get
distracted by spurious inferior output. */
if (target_has_execution && target_can_async_p ())
target_async (0);
}

Thus if we don't clear the get_pending_events_token when disabling async,
target_wait ends up called even after target_async is disabled. That
would normally be harmless, but annoying as it breaks testing
with "set debug infrun 1", because you end up with a spurious
TARGET_WAITKIND_IGNORE event after gdb has already printed the prompt,
which breaks the testsuite.

But in this case, it's actually worse. It results in the event loop being
_continuously_ woken (#4 - #8):

#1 - the event loop wakes up for get_pending_events_token, which results in
#2 - remote_notif_stop_can_get_pending_events -> mark remote_async_inferior_event_token.
#3 - event loop wakes up for remote_async_inferior_event_token
#4 - fetch_inferior_event -> target_wait -> remote_wait -> TARGET_WAITKIND_IGNORE
#5 - handle_inferior_event -> prepare_to_wait
#6 - because async is off, prepare_to_wait calls mark_infrun_async_event_handler.
#7 - event loop wakes up for infrun_async_inferior_event_token -> fetch_inferior_event.
#8 - goto #4.

E.g., just running to main with logging enabled shows:

...
notif: discard queued event: 'Stop' in Thread 0
infrun: target_wait (-1.0.0, status) =
infrun: -1.0.0 [Thread 0],
infrun: status->kind = ignore
infrun: TARGET_WAITKIND_IGNORE
infrun: prepare_to_wait
notif: discard queued event: 'Stop' in Thread 0
infrun: target_wait (-1.0.0, status) =
infrun: -1.0.0 [Thread 0],
infrun: status->kind = ignore
infrun: TARGET_WAITKIND_IGNORE
infrun: prepare_to_wait
notif: discard queued event: 'Stop' in Thread 0
infrun: target_wait (-1.0.0, status) =
infrun: -1.0.0 [Thread 0],
infrun: status->kind = ignore
infrun: TARGET_WAITKIND_IGNORE
infrun: prepare_to_wait
notif: discard queued event: 'Stop' in Thread 0
infrun: target_wait (-1.0.0, status) =
infrun: -1.0.0 [Thread 0],
infrun: status->kind = ignore
infrun: TARGET_WAITKIND_IGNORE
infrun: prepare_to_wait
notif: discard queued event: 'Stop' in Thread 0
infrun: target_wait (-1.0.0, status) =
infrun: -1.0.0 [Thread 0],
infrun: status->kind = ignore
infrun: TARGET_WAITKIND_IGNORE
infrun: prepare_to_wait
notif: discard queued event: 'Stop' in Thread 0
infrun: target_wait (-1.0.0, status) =
infrun: -1.0.0 [Thread 0],
infrun: status->kind = ignore
infrun: TARGET_WAITKIND_IGNORE
infrun: prepare_to_wait
notif: discard queued event: 'Stop' in Thread 0
...

Forever and ever. The testsuite actually regresses due to this, as some
tests enabling infrun logging (e.g., breakpoint-in-ro-region.exp).

The patch below (applies on top of the series) fixes this by making it so
that the pending event token is already clear when we get to the
target_async(0) call in question. The testsuite does pass cleanly with
this patch and without the patch, but regresses if I apply only the
remote_async hunk (that is, if I revert the changes in the patch #2
under discussion).

The remote_notif_get_pending_events hunk below feels like a good
improvement to me, as it gets rid of unnecessary event loop wake ups.
However, I still think that we need the
clear_async_event_handler / mark_async_event_handler calls in
remote_async like I had them before.

That's because we have other target_async(0) calls that unlike the
inf-loop.c call happen while the target _is_ still running. In particular,
I'm thinking of the one in top.c:gdb_readline_wrapper, when we're about to
show a secondary prompt and nest a secondary event loop that should not
get woken by target events.

So I'm thinking that I should leave the original patch as is, and
add some extra comments to remote_async. WDYT?

From 2a73e1b66417edce50d97e566865f2273a988482 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 19 Nov 2015 18:40:13 +0000
Subject: [PATCH] clear pending events token before acking

---
gdb/remote.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 9e69bfb..04057cc 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -6640,6 +6640,10 @@ remote_notif_get_pending_events (struct notif_client *nc)
"notif: process: '%s' ack pending event\n",
nc->name);

+ /* As we're acking the pending notification, we no longer need
+ to wake the event loop to do it. */
+ clear_async_event_handler (rs->notif_state->get_pending_events_token);
+
/* acknowledge */
nc->ack (nc, rs->buf, rs->notif_state->pending_event[nc->id]);
rs->notif_state->pending_event[nc->id] = NULL;
@@ -13158,15 +13162,11 @@ remote_async (struct target_ops *ops, int enable)
event loop to process them. */
if (!QUEUE_is_empty (stop_reply_p, stop_reply_queue))
mark_async_event_handler (remote_async_inferior_event_token);
- if (target_is_non_stop_p ())
- mark_async_event_handler (rs->notif_state->get_pending_events_token);
}
else
{
serial_async (rs->remote_desc, NULL, NULL);
clear_async_event_handler (remote_async_inferior_event_token);
- if (target_is_non_stop_p ())
- clear_async_event_handler (rs->notif_state->get_pending_events_token);
}
}

--
1.9.3