From patchwork Mon Nov 23 15:40:34 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 9794 Received: (qmail 39432 invoked by alias); 23 Nov 2015 15:40:40 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 39422 invoked by uid 89); 23 Nov 2015 15:40:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 23 Nov 2015 15:40:38 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id C8B08E7089; Mon, 23 Nov 2015 15:40:36 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tANFeZEj022021; Mon, 23 Nov 2015 10:40:35 -0500 Message-ID: <56533372.3030805@redhat.com> Date: Mon, 23 Nov 2015 15:40:34 +0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Yao Qi CC: gdb-patches@sourceware.org Subject: Re: [PATCH 02/18] Remote all-stop-on-top-of-non-stop References: <1444836486-25679-1-git-send-email-palves@redhat.com> <1444836486-25679-3-git-send-email-palves@redhat.com> <86vb9x1u2h.fsf@gmail.com> In-Reply-To: <86vb9x1u2h.fsf@gmail.com> 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 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 (¤t_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 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); } }