Re: [PATCH 11/12] Use reinsert_breakpoint for vCont;s

Antoine Tremblay
2016-06-20T18:09:37Z

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.