[rfc] Preferred thread event reporting: Linux native target

[rfc] Preferred thread event reporting: Linux native target

[Ulrich Weigand]

Hello,

this is another patch we've been using internally for a while.

The problem addressed by this patch is related to the way the Linux native
target handled simultaneous events in multi-threaded programs.  After the
OS has signalled an event in some thread, GDB will stop all other threads
(unless in non-stop mode).  During this process, some other thread might
*also* signal an event, so once everything is stopped, there might be
multiple threads with pending events.

However, the Linux native target has to select just one of them to report
up to the infrun logic.  It currently makes that decision randomly.

This may be unexpected to a user who is currently working on one specific
thread (say, single-stepping).  If the single-step event in the thread the
user is working on arrives at the same time as some other event in some 
other thread, it would be nice if that single-step event were reported
first, so as to not interrupt the user's work flow.

And indeed, the Linux native target does contain special code that prefers
reporting an event in a thread that is currently single-stepped, bypassing
the randomization logic that would otherwise be used.

*However*, this unfortunately works only on platforms that use *hardware*
single-stepping, because the native target is not even aware that anything
special is going on when the target uses software single-stepping.

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" -- this is actually simply the currently selected thread
(i.e. the current value of inferior_ptid).  

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.

Tested on powerpc-linux and powerpc64-linux.

Any comments on this approach welcome ...  Am I missing some reason why
this might not be a good idea?

Bye,
Ulrich


ChangeLog:

	* linux-nat.h (struct lwp_info): New member "preferred".
	* linux-nat.c (resume_callback): Reset lp->preferred.
	(linux_nat_resume): Set lp->preferred.
	(select_singlestep_lwp_callback): Rename to ...
	(select_preferred_lwp_callback): ... this.  Check for lp->preferred
	instead of lp->step.
	(select_event_lwp): Update call to select_singlestep_lwp_callback.


diff -urNp gdb-orig/gdb/linux-nat.c gdb-head/gdb/linux-nat.c
--- gdb-orig/gdb/linux-nat.c	2008-08-08 20:14:08.000000000 +0200
+++ gdb-head/gdb/linux-nat.c	2008-08-14 19:28:07.334541534 +0200
@@ -1600,6 +1600,7 @@ resume_callback (struct lwp_info *lp, vo
 			    "RC:  PTRACE_CONT %s, 0, 0 (resume sibling)\n",
 			    target_pid_to_str (lp->ptid));
       lp->stopped = 0;
+      lp->preferred = 0;
       lp->step = 0;
       memset (&lp->siginfo, 0, sizeof (lp->siginfo));
     }
@@ -1671,6 +1672,9 @@ linux_nat_resume (ptid_t ptid, int step,
   /* Convert to something the lower layer understands.  */
   ptid = pid_to_ptid (GET_LWP (lp->ptid));
 
+  /* Mark this thread as the preferred thread.  */
+  lp->preferred = 1;
+
   /* Remember if we're stepping.  */
   lp->step = step;
 
@@ -2293,12 +2297,12 @@ count_events_callback (struct lwp_info *
   return 0;
 }
 
-/* Select the LWP (if any) that is currently being single-stepped.  */
+/* Select the LWP the user is currently interested in.  */
 
 static int
-select_singlestep_lwp_callback (struct lwp_info *lp, void *data)
+select_preferred_lwp_callback (struct lwp_info *lp, void *data)
 {
-  if (lp->step && lp->status != 0)
+  if (lp->preferred && lp->status != 0)
     return 1;
   else
     return 0;
@@ -2396,8 +2400,8 @@ select_event_lwp (struct lwp_info **orig
   /* Record the wait status for the original LWP.  */
   (*orig_lp)->status = *status;
 
-  /* Give preference to any LWP that is being single-stepped.  */
-  event_lp = iterate_over_lwps (select_singlestep_lwp_callback, NULL);
+  /* Give preference to the LWP the user is currently interested in.  */
+  event_lp = iterate_over_lwps (select_preferred_lwp_callback, NULL);
   if (event_lp != NULL)
     {
       if (debug_linux_nat)
@@ -2407,8 +2411,8 @@ select_event_lwp (struct lwp_info **orig
     }
   else
     {
-      /* No single-stepping LWP.  Select one at random, out of those
-         which have had SIGTRAP events.  */
+      /* No preferred LWP.  Select one at random, out of those which
+	 have had SIGTRAP events.  */
 
       /* First see how many SIGTRAP events we have.  */
       iterate_over_lwps (count_events_callback, &num_events);
diff -urNp gdb-orig/gdb/linux-nat.h gdb-head/gdb/linux-nat.h
--- gdb-orig/gdb/linux-nat.h	2008-08-05 22:16:25.000000000 +0200
+++ gdb-head/gdb/linux-nat.h	2008-08-14 19:20:07.523558223 +0200
@@ -65,6 +65,9 @@ struct lwp_info
   /* Non-zero if we expect a duplicated SIGINT.  */
   int ignore_sigint;
 
+  /* Non-zero if this is the thread preferred for event reporting.  */
+  int preferred;
+
   /* If WAITSTATUS->KIND != TARGET_WAITKIND_SPURIOUS, the waitstatus
      for this LWP's last event.  This may correspond to STATUS above,
      or to a local variable in lin_lwp_wait.  */
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 2348 bytes --]

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

[-- Attachment #2: clear_proceed_other_threads.diff --]
[-- Type: text/x-diff, Size: 2774 bytes --]

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
 \f
 /* 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

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

[Ulrich Weigand]

Pedro Alves wrote:

> > -- 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.

Hmm, but if we "thread hop" inferior_ptid should be prefered
anyway (to get the internal "thread hop" action over with as
quickly as possible), and afterwards we're back to the thread
the user is looking at, right?

> > 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.

On the other hand, if the information we need is visible without
relying on inferior_ptid, that would be even nicer.

> 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.

Great!  This is one more thing I wanted to address; thanks for taking
care of it ...

> 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.

I can test AIX if necessary.  Do you have a patch?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Michael Snyder]

Ulrich Weigand wrote:
> Pedro Alves wrote:
> 
>>> -- 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.
> 
> Hmm, but if we "thread hop" inferior_ptid should be prefered
> anyway (to get the internal "thread hop" action over with as
> quickly as possible), and afterwards we're back to the thread
> the user is looking at, right?

After that, I think, we're back to whichever thread
the OS decides to schedule next.  I don't think there
is any guarantee that that will be the one the user
was previously looking at.

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

[Ulrich Weigand]

Michael Snyder wrote:
> Ulrich Weigand wrote:
> > Pedro Alves wrote:
> > 
> >>> -- 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.
> > 
> > Hmm, but if we "thread hop" inferior_ptid should be prefered
> > anyway (to get the internal "thread hop" action over with as
> > quickly as possible), and afterwards we're back to the thread
> > the user is looking at, right?
> 
> After that, I think, we're back to whichever thread
> the OS decides to schedule next.  I don't think there
> is any guarantee that that will be the one the user
> was previously looking at.

I think the OS scheduler is not really relevant here.
GDB assumes it can decide which of the threads of an
application it wants to *single-step*.

So if an application has threads A, B, and C, and is 
stopped on an event in B, and then decides it wants to
continue single-stepping C, it will tell the target to
continue all thread, while single-stepping C.

Now the OS scheduler may choose to run A, B, or C next
(or several of them in parallel on an SMP system), but
no matter what: we should never get single-step events
from A or B, but we should get a single-step event after
C has executed its first instruction (whenever that may
be).

The above assumption (that GDB can decide which threads
to single-step) is always true when using software single-
stepping.  It is also true for the Linux native target,
and for the remote target (assuming the remote side
implements the protocol accordingly).

There may be odd targets where this assumption is false;
GDB will probably get confused on those.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Pedro Alves]

On Thursday 14 August 2008 22:50:22, Ulrich Weigand wrote:
> Pedro Alves wrote:
> > > -- 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.
>
> Hmm, but if we "thread hop" inferior_ptid should be prefered
> anyway (to get the internal "thread hop" action over with as
> quickly as possible), 

When we thread hop, we tell the target to only allow a single
thread to run.  On linux, there's no way to could already have a
cached event in the thread the user is stepping at
this point, otherwise, we'd not have detected the need for a
thread hop.  So, right after resuming to do the thread hop, if
there's any other cached event to select from, because
they were cached when we reported the stop that originated
the thread hop, it won't be from the thread the user was
stepping.

But, this above is moot currently, because when we
thread hop, we enter infwait_thread_hop_state, which
ends up forcing linux_nat_wait to only return with the ptid of
the thread we're hopping, thus, the thread hop action gets over
as quickly as possible already.

> and afterwards we're back to the thread 
> the user is looking at, right?

Well, in all-stop, there's nothing doing that currently.  when
you switch inferior_ptid (in all-stop mode) to handle a
possible stop, and then you decide you should not stop, there's
nothing reverting back to the previous thread.  inferior_ptid will
stay pointing to the last thread you resumed (the hopping thread),
until you hit another stop event, and context_switch to the thread
that took it.

> > 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.
>
> I can test AIX if necessary.  Do you have a patch?

I stratched my head quite a bit staring at aix-thread.c/sync_threadlists
and its use of ptid_cmp.  I still can't tell if/what I should do there.
Also, I can't tell if rs6000_wait can ever return a ptid
different from pid_to_ptid(main_process_pid), (or -1).

Basically, I need to:

 1) make sure the core never gets a thread related event
    that corresponds to a thread the core doesn't know
    about yet.

 2) #1 implies that every target should register the main
   thread, even when debugging a single-threaded app.
   rs6000-nat.c, being a ptrace based target, already has that
   covered by these:
    http://sourceware.org/ml/gdb-patches/2008-08/msg00170.html
    http://sourceware.org/ml/gdb-patches/2008-08/msg00171.html

 3) #2 implies that a thread_stratum layer should decorate the
    main thread's ptid with thread id info, instead of adding it
    again. That's thread_change_ptid from:    
    http://sourceware.org/ml/gdb-patches/2008-08/msg00170.html

-- 
Pedro Alves

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

[Daniel Jacobowitz]

On Fri, Aug 15, 2008 at 11:54:31PM +0100, Pedro Alves wrote:
> > > 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.
> >
> > I can test AIX if necessary.  Do you have a patch?
> 
> I stratched my head quite a bit staring at aix-thread.c/sync_threadlists
> and its use of ptid_cmp.  I still can't tell if/what I should do there.
> Also, I can't tell if rs6000_wait can ever return a ptid
> different from pid_to_ptid(main_process_pid), (or -1).
> 
> Basically, I need to:
> 
>  1) make sure the core never gets a thread related event
>     that corresponds to a thread the core doesn't know
>     about yet.
> 
>  2) #1 implies that every target should register the main
>    thread, even when debugging a single-threaded app.
>    rs6000-nat.c, being a ptrace based target, already has that
>    covered by these:
>     http://sourceware.org/ml/gdb-patches/2008-08/msg00170.html
>     http://sourceware.org/ml/gdb-patches/2008-08/msg00171.html
> 
>  3) #2 implies that a thread_stratum layer should decorate the
>     main thread's ptid with thread id info, instead of adding it
>     again. That's thread_change_ptid from:    
>     http://sourceware.org/ml/gdb-patches/2008-08/msg00170.html

Ulrich, I don't suppose I could ask you to do this?  Pedro's already
done a heroic job fixing up other targets... Pedro, I know you started
looking at AIX; maybe you have a patch that at least points out the
places that need to change?

-- 
Daniel Jacobowitz
CodeSourcery

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

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Fri, Aug 15, 2008 at 11:54:31PM +0100, Pedro Alves wrote:

> > I stratched my head quite a bit staring at aix-thread.c/sync_threadlists
> > and its use of ptid_cmp.  I still can't tell if/what I should do there.
> > Also, I can't tell if rs6000_wait can ever return a ptid
> > different from pid_to_ptid(main_process_pid), (or -1).

> Ulrich, I don't suppose I could ask you to do this?  Pedro's already
> done a heroic job fixing up other targets... Pedro, I know you started
> looking at AIX; maybe you have a patch that at least points out the
> places that need to change?

I'm not really the AIX expert either, but I'll have a look.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Daniel Jacobowitz]

On Mon, Aug 18, 2008 at 07:00:43PM +0200, Ulrich Weigand wrote:
> I'm not really the AIX expert either, but I'll have a look.

Thank you!  Sadly, AIX is one of those platforms we don't have an
active expert in... we have enough support that we don't want to break
it, but not enough expertise to keep it going, so we'll keep tottering
along :-(

-- 
Daniel Jacobowitz
CodeSourcery

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

[Pedro Alves]

On Monday 18 August 2008 18:00:43, Ulrich Weigand wrote:
> Daniel Jacobowitz wrote:
> > Pedro, I know you started 
> > looking at AIX; maybe you have a patch that at least points out the
> > places that need to change?
>

Yeah.  Let me get it.  At least I'll add some comments to where I
think changes should be made.

> I'm not really the AIX expert either, but I'll have a look.

Thanks a million!

-- 
Pedro Alves

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

[Pedro Alves]

On Friday 15 August 2008 23:54:31, Pedro Alves wrote:

> > and afterwards we're back to the thread
> > the user is looking at, right?
>
> Well, in all-stop, there's nothing doing that currently.  when
> you switch inferior_ptid (in all-stop mode) to handle a
> possible stop, and then you decide you should not stop, there's
> nothing reverting back to the previous thread.  inferior_ptid will
> stay pointing to the last thread you resumed (the hopping thread),
> until you hit another stop event, and context_switch to the thread
> that took it.

Hmmm, I was just taking another look at this, and,
you may be right here.

We'd have also have requested a single-step in the thread we
wanted before detecting a need for a thread hop, so, when we
get to eventually handling that original single-step
finishing, we'd context-switch to it again.  I guess this
would make your change OK.  If not, how does this work then?

-- 
Pedro Alves

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

[Ulrich Weigand]

Pedro Alves wrote:
> On Friday 15 August 2008 23:54:31, Pedro Alves wrote:
> 
> > > and afterwards we're back to the thread
> > > the user is looking at, right?
> >
> > Well, in all-stop, there's nothing doing that currently.  when
> > you switch inferior_ptid (in all-stop mode) to handle a
> > possible stop, and then you decide you should not stop, there's
> > nothing reverting back to the previous thread.  inferior_ptid will
> > stay pointing to the last thread you resumed (the hopping thread),
> > until you hit another stop event, and context_switch to the thread
> > that took it.
> 
> Hmmm, I was just taking another look at this, and,
> you may be right here.
> 
> We'd have also have requested a single-step in the thread we
> wanted before detecting a need for a thread hop, so, when we
> get to eventually handling that original single-step
> finishing, we'd context-switch to it again.  I guess this
> would make your change OK.  If not, how does this work then?

Yes, that was my thought.  We *have* to get inferior_ptid right
on platforms with hardware single-step (because linux-nat.c will
only hardware step that thread), and therefore we will also get
inferior_ptid right on platforms with software single-step 
(because the code is the same).

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com