Re: [rfc] Preferred thread event reporting: Linux native target

[Pedro Alves <pedro at codesourcery dot com>]

Hi Ulrich,

On Thursday 14 August 2008 21:39:59, Ulrich Weigand wrote:

> It seems to me that this implementation detail should not determine
> user-visible behaviour in the way described above.  Therefore, I'd suggest
> to try to *always* prefer reporting events in the thread the user currently
> "cares about" 

Agreed in principle, although that may create contention.  But as you
say, if the user is stepping a thread, your proposal seems
less surprising.

> -- this is actually simply the currently selected thread 
> (i.e. the current value of inferior_ptid).

Disagreed.  inferior_ptid will change if an event happens in
another thread while you're stepping, but the core decides the event
was not a good reason to stop.  E.g., thread hopping.

So, resetting the prefered thread here:

> 	(linux_nat_resume): Set lp->preferred.

.. is fallible.

> The patch below implements this by adding a new member "preferred" to
> "struct lwp_info", setting it according to the value of inferior_ptid
> in linux_nat_resume, and using it (instead of the single-step flag) to
> decide whether to prefer reporting events in this thread.

I'd prefer to check if an lwp is stepping due to user request, by
checking struct thread_info's data directly, intead of your
"prefered" flag.

We already have all the needed data in struct thread_info, although,
because of context-switching, if the lwp matches inferior_ptid,
you have to check the stepping state in the global vars; if it doesn't
match inferior_ptid (which again, is not garanteed to be the
last thread the user had selected), you get the stepping data
directly from the thread_info list.

I happen to be just preparing to submit the series that gets rid of
context-switching, which gets rid of that special case.

Unfortunatelly, currently, GDB doesn't always correctly clear the
stepping state of all threads when proceeding (clear_proceed_status
only clears the current thread), but I'm addressing that too in the
series, see attached.

Maybe the attached could be rebased against pristine head, and
you could use it, although I would prefer to put the whole
series in.

For my series to go in, every target much register at least the main
thread in GDB's thread tables, and as it happens, I think AIX
is the only target I don't have covered, or that I know of
no one covering.

-- 
Pedro Alves

2008-07-31  Pedro Alves  <pedro@codesourcery.com>

	Clear proceed status of all threads that are going to be resumed.

	* infrun.c (clear_proceed_status_thread_callback): New.
	(clear_proceed_status): Use it.
	(proceed): Clear the proceed status off all threads that are going
	to be resumed.

---
 gdb/infrun.c |   49 ++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 11 deletions(-)

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2008-08-14 04:05:15.000000000 +0100
+++ src/gdb/infrun.c	2008-08-14 04:05:17.000000000 +0100
@@ -1081,6 +1081,29 @@ a command like `return' or `jump' to con
 
 /* Proceeding.  */
 
+static int
+clear_proceed_status_thread_callback (struct thread_info *tp, void *data)
+{
+  struct thread_info *ignore = data;
+
+  if (ignore == tp)
+    return 0;
+
+  if (is_exited (tp->ptid))
+    return 0;
+
+  tp->step_range_start = 0;
+  tp->step_range_end = 0;
+  tp->step_frame_id = null_frame_id;
+  tp->step_over_calls = STEP_OVER_UNDEBUGGABLE;
+  tp->proceed_to_finish = 0;
+
+  /* Discard any remaining commands or status from previous stop.  */
+  bpstat_clear (&tp->stop_bpstat);
+
+  return 0;
+}
+
 /* Clear out all variables saying what to do when inferior is continued.
    First do this, then set the ones you want, then call `proceed'.  */
 
@@ -1091,19 +1114,10 @@ clear_proceed_status (void)
     {
       struct thread_info *tp = inferior_thread ();
 
-      tp->trap_expected = 0;
-      tp->step_range_start = 0;
-      tp->step_range_end = 0;
-      tp->step_frame_id = null_frame_id;
-      tp->step_over_calls = STEP_OVER_UNDEBUGGABLE;
+      clear_proceed_status_thread_callback (tp, NULL);
 
+      tp->trap_expected = 0;
       tp->stop_step = 0;
-
-      tp->proceed_to_finish = 0;
-
-      /* Discard any remaining commands or status from previous
-	 stop.  */
-      bpstat_clear (&tp->stop_bpstat);
     }
 
   stop_after_trap = 0;
@@ -1267,6 +1281,19 @@ proceed (CORE_ADDR addr, enum target_sig
      inferior.  */
   gdb_flush (gdb_stdout);
 
+  if (!non_stop
+      && (scheduler_mode == schedlock_off
+	  || (scheduler_mode == schedlock_step && !step)))
+    {
+      /* We're going to let all threads run.  Clear the user stepping
+	 state of all other threads but the selected thread.  The
+	 selected thread is already taken care of and may be starting
+	 a step request, so leave it be.  */
+
+      iterate_over_threads (clear_proceed_status_thread_callback,
+			    inferior_thread ());
+    }
+
   /* Refresh prev_pc value just prior to resuming.  This used to be
      done in stop_stepping, however, setting prev_pc there did not handle
      scenarios such as inferior function calls or returning from