From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 102471 invoked by alias); 20 Jun 2016 18:09:37 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 102456 invoked by uid 89); 20 Jun 2016 18:09:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_PASS autolearn=ham version=3.3.2 spammy=H*F:D*ericsson.com, suspend, our, among X-HELO: usplmg20.ericsson.net Received: from usplmg20.ericsson.net (HELO usplmg20.ericsson.net) (198.24.6.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Mon, 20 Jun 2016 18:09:35 +0000 Received: from EUSAAHC002.ericsson.se (Unknown_Domain [147.117.188.78]) by usplmg20.ericsson.net (Symantec Mail Security) with SMTP id 16.83.09012.28728675; Mon, 20 Jun 2016 19:27:30 +0200 (CEST) Received: from elxa4wqvvz1 (147.117.188.8) by smtps-am.internal.ericsson.com (147.117.188.78) with Microsoft SMTP Server (TLS) id 14.3.294.0; Mon, 20 Jun 2016 14:09:32 -0400 References: <1464859846-15619-1-git-send-email-yao.qi@linaro.org> <1464859846-15619-12-git-send-email-yao.qi@linaro.org> <61835b69-a4bf-a912-4eb3-b223c2a16614@redhat.com> <86h9cvud2z.fsf@gmail.com> <1cec772e-a659-3f2f-1eae-67d27fdbd9e0@redhat.com> <86a8imtnf7.fsf@gmail.com> User-agent: mu4e 0.9.17; emacs 24.4.1 From: Antoine Tremblay To: Pedro Alves CC: Yao Qi , Subject: Re: [PATCH 11/12] Use reinsert_breakpoint for vCont;s In-Reply-To: Date: Mon, 20 Jun 2016 18:09:00 -0000 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2016-06/txt/msg00336.txt.bz2 Pedro Alves writes: >> @@ -3518,6 +3521,23 @@ linux_wait_1 (ptid_t ptid, >> return ignore_event (ourstatus); >> } >> >> + /* Remove reinsert breakpoints ... */ >> + if (can_software_single_step () >> + && has_reinsert_breakpoints (current_thread) >> + /*... if GDB requests this thread doing resume_step or ...*/ >> + && (current_thread->last_resume_kind == resume_step >> + /* GDBserver has already started the step-over for vCont;s, >> + but it gets some other signal, like SIGSTOP sent by >> + GDBserver for vCont;t or other signal program received. */ >> + || !maybe_internal_trap)) >> + { >> + stop_all_lwps (1, event_child); >> + >> + delete_reinsert_breakpoints (current_thread); >> + >> + unstop_all_lwps (1, event_child); >> + } > > I'm re-looking at this and wondering if this is really the > right place to do this. If the thread hits a breakpoint > that ends up not reported to gdb (e.g., condition evals false), > then we'll remove the reinsert breakpoints here, and then > later reinsert them in proceed_all_lwps. The extra > stopping/unstopping everything is best avoided if possible. > > Thus, couldn't we move this to after: > > /* We found no reason GDB would want us to stop. We either hit one > of our own breakpoints, or finished an internal step GDB > shouldn't know about. */ > if (!report_to_gdb) > { > ... > } > > ? A note about avoiding stop/unstop, could we make it so that we really stop/unstop only when we're actually modifing memory ? If we place multiple reinsert breakpoints at one location over multiple threads like the non-stop-fair-events tests does for example, we end up calling stop/unstop when only the breakpoint refcount gets modified. This could be applied in general to the stop / breakpoint operation / unstop case... I'm testing range-stepping with this patchset and found that with non-stop-fair-events the single stepping threads stop/unstop so often that it actually starves the execution of the thread that could make them stop single stepping. Thread 2-11 of the test step in an infinite loop waiting for a signal from thread 1 but thread 1 even if it's resumed, is stopped/started so quickly that it never gets a chance to really execute. Most likely limiting this stop/unstop for the refcounted case would not solve that issue but may help. I'm not sure what else could be done for that except maybe displaced stepping ? Also if you're testing this out, note that there's a bug in the event selection code I think that can be fixed with : diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index 4e79ec1..9d2f4d9 100644 --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -3664,24 +3670,6 @@ linux_wait_1 (ptid_t ptid, if (!non_stop) stop_all_lwps (0, NULL); - /* If we're not waiting for a specific LWP, choose an event LWP - from among those that have had events. Giving equal priority - to all LWPs that have had events helps prevent - starvation. */ - if (ptid_equal (ptid, minus_one_ptid)) - { - event_child->status_pending_p = 1; - event_child->status_pending = w; - - select_event_lwp (&event_child); - - /* current_thread and event_child must stay in sync. */ - current_thread = get_lwp_thread (event_child); - - event_child->status_pending_p = 0; - w = event_child->status_pending; - } - if (step_over_finished) { if (!non_stop) @@ -3706,6 +3694,25 @@ linux_wait_1 (ptid_t ptid, } } + /* If we're not waiting for a specific LWP, choose an event LWP + from among those that have had events. Giving equal priority + to all LWPs that have had events helps prevent + starvation. */ + if (ptid_equal (ptid, minus_one_ptid)) + { + event_child->status_pending_p = 1; + event_child->status_pending = w; + + select_event_lwp (&event_child); + + /* current_thread and event_child must stay in sync. */ + current_thread = get_lwp_thread (event_child); + + event_child->status_pending_p = 0; + w = event_child->status_pending; + } + + /* Stabilize threads (move out of jump pads). */ if (!non_stop) stabilize_threads (); Otherwise, the suspend refcount gets it wrong since we're changing the event_child before calling unsuspend_all_lwps/unstop. I'm waiting to send this one since it I need the range-stepping to test it but I hope it's usefull to the WIP here.