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

[Antoine Tremblay <antoine.tremblay@ericsson.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.