This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 11/12] Use reinsert_breakpoint for vCont;s
- From: Antoine Tremblay <antoine dot tremblay at ericsson dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: Yao Qi <qiyaoltc at gmail dot com>, <gdb-patches at sourceware dot org>
- Date: Mon, 20 Jun 2016 14:09:31 -0400
- Subject: Re: [PATCH 11/12] Use reinsert_breakpoint for vCont;s
- Authentication-results: sourceware.org; auth=none
- References: <1464859846-15619-1-git-send-email-yao dot qi at linaro dot org> <1464859846-15619-12-git-send-email-yao dot qi at linaro dot org> <61835b69-a4bf-a912-4eb3-b223c2a16614 at redhat dot com> <86h9cvud2z dot fsf at gmail dot com> <1cec772e-a659-3f2f-1eae-67d27fdbd9e0 at redhat dot com> <86a8imtnf7 dot fsf at gmail dot com> <ed0b4fbf-5d6c-dd5d-19ba-c38a87cc7d4e at redhat dot com>
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.