From patchwork Mon Apr 13 15:28:35 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 6184 Received: (qmail 127345 invoked by alias); 13 Apr 2015 15:28:41 -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 126919 invoked by uid 89); 13 Apr 2015 15:28:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD 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, 13 Apr 2015 15:28:40 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t3DFSbMv006140 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 13 Apr 2015 11:28:37 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t3DFSaHK030802; Mon, 13 Apr 2015 11:28:36 -0400 Message-ID: <552BE0A3.5040001@redhat.com> Date: Mon, 13 Apr 2015 16:28:35 +0100 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Yao Qi CC: gdb-patches@sourceware.org Subject: Re: [PATCH v2 00/23] All-stop on top of non-stop References: <1428410990-28560-1-git-send-email-palves@redhat.com> <86lhi0v1el.fsf@gmail.com> <55278B2B.1070809@redhat.com> <868ue0uyfa.fsf@gmail.com> In-Reply-To: <868ue0uyfa.fsf@gmail.com> On 04/10/2015 10:26 AM, Yao Qi wrote: > Pedro Alves writes: > >> Fun. TBC, that was only with gdbserver, right? >> > > Right, I just run non-stop-fair-events.exp on native aarch64-linux gdb, > they all pass. > >> I suspect the test was only passing by change before though. > > They pass before this change. > >> AFAICS, aarch64 doesn't have a displaced stepping implementation. > > No, aarch64 doesn't have. > >> I'd suspect current master fails other non-stop tests? (and hopefully >> this series fixes them). > > Current master doesn't fail other non-stop tests on aarch64-linux. This > series doesn't change the test result except non-stop-fair-events.exp. > >> >> So GDB should now be falling back to stopping all threads to >> step past the breakpoint on aarch64, while before threads were >> just missing breakpoints. Likely something wrong with that >> with remote targets still. > > I'll take a look, and see if I can find anything wrong. With displaced stepping disabled on x86, I can reproduce it sometimes here too. The issue seems to be that we're constantly bouncing between the events of the same two threads over and over, and thus the signal event is never processed. Exactly the sort of issue the test is meant to catch. Hurray, I guess? :-) I've been testing with the patch below, and it seems to fix it. Testing against gdbserver + software single-step + displaced stepping disabled caught a couple other issues too. I'm testing this further and cleaning it all up now. Thanks, Pedro Alves commit 9112338313db3a23b9abb4d2176ce6f62498dc08 Author: Pedro Alves AuthorDate: Mon Apr 13 14:16:27 2015 +0100 better randomization diff --git a/gdb/infrun.c b/gdb/infrun.c index 4dd25d6..334d153 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -4154,6 +4154,82 @@ switch_to_thread_cleanup (void *ptid_p) switch_to_thread (ptid); } +/* Save the thread's event and stop reason to process it later. */ + +static void +save_waitstatus (struct thread_info *tp, struct target_waitstatus *ws) +{ + struct regcache *regcache; + struct address_space *aspace; + + if (debug_infrun) + { + char *statstr; + + statstr = target_waitstatus_to_string (ws); + fprintf_unfiltered (gdb_stdlog, + "infrun: saving status %s for %d.%ld.%ld\n", + statstr, + ptid_get_pid (tp->ptid), + ptid_get_lwp (tp->ptid), + ptid_get_tid (tp->ptid)); + xfree (statstr); + } + + /* Record for later. */ + tp->suspend.waitstatus = *ws; + tp->suspend.waitstatus_pending_p = 1; + + regcache = get_thread_regcache (tp->ptid); + aspace = get_regcache_aspace (regcache); + + if (ws->kind == TARGET_WAITKIND_STOPPED + && ws->value.sig == GDB_SIGNAL_TRAP) + { + CORE_ADDR pc = regcache_read_pc (regcache); + + adjust_pc_after_break (tp, &tp->suspend.waitstatus); + + if (thread_stopped_by_watchpoint (tp->ptid)) + { + tp->suspend.stop_reason + = TARGET_STOPPED_BY_WATCHPOINT; + } + else if (target_supports_stopped_by_sw_breakpoint () + && thread_stopped_by_sw_breakpoint (tp->ptid)) + { + tp->suspend.stop_reason + = TARGET_STOPPED_BY_SW_BREAKPOINT; + } + else if (target_supports_stopped_by_hw_breakpoint () + && thread_stopped_by_hw_breakpoint (tp->ptid)) + { + tp->suspend.stop_reason + = TARGET_STOPPED_BY_HW_BREAKPOINT; + } + else if (!target_supports_stopped_by_hw_breakpoint () + && hardware_breakpoint_inserted_here_p (aspace, + pc)) + { + tp->suspend.stop_reason + = TARGET_STOPPED_BY_HW_BREAKPOINT; + } + else if (!target_supports_stopped_by_sw_breakpoint () + && software_breakpoint_inserted_here_p (aspace, + pc)) + { + tp->suspend.stop_reason + = TARGET_STOPPED_BY_SW_BREAKPOINT; + } + else if (!thread_has_single_step_breakpoints_set (tp) + && currently_stepping (tp)) + { + tp->suspend.stop_reason + = TARGET_STOPPED_BY_SINGLE_STEP; + } + } +} + /* Stop all threads. */ static void @@ -4317,61 +4393,11 @@ stop_all_threads (void) } /* Record for later. */ - t->suspend.waitstatus = ws; - t->suspend.waitstatus_pending_p = 1; + save_waitstatus (t, &ws); sig = (ws.kind == TARGET_WAITKIND_STOPPED ? ws.value.sig : GDB_SIGNAL_0); - regcache = get_thread_regcache (t->ptid); - aspace = get_regcache_aspace (regcache); - - if (ws.kind == TARGET_WAITKIND_STOPPED - && ws.value.sig == GDB_SIGNAL_TRAP) - { - CORE_ADDR pc = regcache_read_pc (regcache); - - adjust_pc_after_break (t, &t->suspend.waitstatus); - - if (thread_stopped_by_watchpoint (t->ptid)) - { - t->suspend.stop_reason - = TARGET_STOPPED_BY_WATCHPOINT; - } - else if (target_supports_stopped_by_sw_breakpoint () - && thread_stopped_by_sw_breakpoint (t->ptid)) - { - t->suspend.stop_reason - = TARGET_STOPPED_BY_SW_BREAKPOINT; - } - else if (target_supports_stopped_by_hw_breakpoint () - && thread_stopped_by_hw_breakpoint (t->ptid)) - { - t->suspend.stop_reason - = TARGET_STOPPED_BY_HW_BREAKPOINT; - } - else if (!target_supports_stopped_by_hw_breakpoint () - && hardware_breakpoint_inserted_here_p (aspace, - pc)) - { - t->suspend.stop_reason - = TARGET_STOPPED_BY_HW_BREAKPOINT; - } - else if (!target_supports_stopped_by_sw_breakpoint () - && software_breakpoint_inserted_here_p (aspace, - pc)) - { - t->suspend.stop_reason - = TARGET_STOPPED_BY_SW_BREAKPOINT; - } - else if (!thread_has_single_step_breakpoints_set (t) - && currently_stepping (t)) - { - t->suspend.stop_reason - = TARGET_STOPPED_BY_SINGLE_STEP; - } - } - if (displaced_step_fixup (t->ptid, sig) < 0) { /* Add it back to the step-over queue. */ @@ -4379,6 +4405,7 @@ stop_all_threads (void) thread_step_over_chain_enqueue (t); } + regcache = get_thread_regcache (t->ptid); t->suspend.stop_pc = regcache_read_pc (regcache); if (debug_infrun) @@ -5033,7 +5060,13 @@ restart_threads (struct thread_info *event_thread) /* If some thread needs to start a step-over at this point, it should still be in the step-over queue, and thus skipped above. */ - gdb_assert (!thread_still_needs_step_over (tp)); + if (thread_still_needs_step_over (tp)) + { + internal_error (__FILE__, __LINE__, + "thread [%s] needs a step-over, but not in " + "step-over queue\n", + target_pid_to_str (tp->ptid)); + } if (currently_stepping (tp)) { @@ -5056,10 +5089,24 @@ restart_threads (struct thread_info *event_thread) } } +/* Callback for iterate_over_threads. Find a resumed thread that has + a pending waitstatus. */ + +static int +resumed_thread_with_pending_status (struct thread_info *tp, + void *arg) +{ + return (tp->resumed + && tp->suspend.waitstatus_pending_p); +} + /* Called when we get an event that may finish an in-line or - out-of-line (displaced stepping) step-over started previously. */ + out-of-line (displaced stepping) step-over started previously. + Return true if the event is processed and we should go back to the + event loop; false if the caller should continue processing the + event. */ -static void +static int finish_step_over (struct execution_control_state *ecs) { int had_step_over_info; @@ -5081,7 +5128,7 @@ finish_step_over (struct execution_control_state *ecs) } if (!target_is_non_stop_p ()) - return; + return 0; /* Start a new step-over in another thread if there's one that needs it. */ @@ -5094,10 +5141,67 @@ finish_step_over (struct execution_control_state *ecs) these other threads stop. */ if (had_step_over_info && !step_over_info_valid_p ()) { + struct thread_info *pending; + restart_threads (ecs->event_thread); - gdb_assert (!ecs->event_thread->resumed); + /* If we have events pending, go through handle_inferior_event + again, picking up a pending event at random. This avoids + thread starvation. */ + pending = iterate_over_threads (resumed_thread_with_pending_status, + NULL); + if (pending != NULL) + { + struct thread_info *tp = ecs->event_thread; + struct regcache *regcache; + + if (debug_infrun) + { + fprintf_unfiltered (gdb_stdlog, + "infrun: found resumed threads with " + "pending events, saving status\n"); + } + + gdb_assert (pending != tp); + + /* Record the event thread's event for later. */ + save_waitstatus (tp, &ecs->ws); + /* This was cleared early, by handle_inferior_event. Set it + so this pending event is considered by + do_target_wait. */ + tp->resumed = 1; + + gdb_assert (!tp->executing); + + regcache = get_thread_regcache (tp->ptid); + tp->suspend.stop_pc = regcache_read_pc (regcache); + + if (debug_infrun) + { + fprintf_unfiltered (gdb_stdlog, + "infrun: saved stop_pc=%s for %s " + "(currently_stepping=%d)\n", + paddress (target_gdbarch (), + tp->suspend.stop_pc), + target_pid_to_str (tp->ptid), + currently_stepping (tp)); + } + + /* This in-line step-over finished; clear this so we won't + start a new one. This is what handle_signal_stop would + do, if we returned false. */ + tp->stepping_over_breakpoint = 0; + tp->stepping_over_watchpoint = 0; + + /* Wake up the event loop again. */ + mark_async_event_handler (infrun_async_inferior_event_token); + + prepare_to_wait (ecs); + return 1; + } } + + return 0; } /* Come here when the program has stopped with a signal. */ @@ -5116,7 +5220,8 @@ handle_signal_stop (struct execution_control_state *ecs) /* Do we need to clean up the state of a thread that has completed a displaced single-step? (Doing so usually affects the PC, so do it here, before we set stop_pc.) */ - finish_step_over (ecs); + if (finish_step_over (ecs)) + return; /* If we either finished a single-step or hit a breakpoint, but the user wanted this thread to be stopped, pretend we got a