[PATCH/RFC] Improve the "thread_step_needed" logic

[PATCH/RFC] Improve the "thread_step_needed" logic

[Michael Snyder]

This is a fairly significant change, that both simplifies and
improves the logic for deciding when a single thread should be
stepped (to get past a breakpoint), rather than stepping all threads.
In a nutshell, this replaces the state variable "thread_step_needed"
with the following logic in resume():

      resume_ptid = RESUME_ALL;		/* Default */

      if ((step || singlestep_breakpoints_inserted_p) &&
	  !breakpoints_inserted && breakpoint_here_p (read_pc ()))
	{
	  /* Stepping past a breakpoint without inserting breakpoints.
	     Make sure only the current thread gets to step, so that
	     other threads don't sneak past breakpoints while they are
	     not inserted. */

	  resume_ptid = inferior_ptid;
	}

I have run the thread testsuites on Linux, and done rather extensive
hand-testing.  The behavior is clearly improved by this change --
it eliminates instances where threads are allowed to run away because
they are resumed while breakpoints are not inserted.

I'll wait a week or two for people to yell before committing this.


2001-06-15  Michael Snyder  <msnyder@redhat.com>

	* infrun.c: Eliminate the "thread_step_needed" state variable, 
	and replace it with a relatively simple test in resume.
	(resume): Replace thread_step_needed logic with a test for
	stepping, breakpoint_here_p and breakpoints_inserted.
	Move CANNOT_STEP_BREAKPOINT logic to after thread_step logic.
	(proceed): Discard thread_step_needed logic.
	(wait_for_inferior, fetch_inferior_event, handle_inferior_event):
	Discard thread_step_needed logic.

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.37
diff -c -3 -p -r1.37 infrun.c
*** infrun.c	2001/06/15 22:44:20	1.37
--- infrun.c	2001/06/15 23:23:48
*************** static ptid_t previous_inferior_ptid;
*** 110,140 ****
  
  static int may_follow_exec = MAY_FOLLOW_EXEC;
  
- /* resume and wait_for_inferior use this to ensure that when
-    stepping over a hit breakpoint in a threaded application
-    only the thread that hit the breakpoint is stepped and the
-    other threads don't continue.  This prevents having another
-    thread run past the breakpoint while it is temporarily
-    removed.
- 
-    This is not thread-specific, so it isn't saved as part of
-    the infrun state.
- 
-    Versions of gdb which don't use the "step == this thread steps
-    and others continue" model but instead use the "step == this
-    thread steps and others wait" shouldn't do this.  */
- 
- static int thread_step_needed = 0;
- 
- /* This is true if thread_step_needed should actually be used.  At
-    present this is only true for HP-UX native.  */
- 
- #ifndef USE_THREAD_STEP_NEEDED
- #define USE_THREAD_STEP_NEEDED (0)
- #endif
- 
- static int use_thread_step_needed = USE_THREAD_STEP_NEEDED;
- 
  /* GET_LONGJMP_TARGET returns the PC at which longjmp() will resume the
     program.  It needs to examine the jmp_buf argument and extract the PC
     from it.  The return value is non-zero on success, zero otherwise. */
--- 110,115 ----
*************** resume (int step, enum target_signal sig
*** 821,834 ****
    struct cleanup *old_cleanups = make_cleanup (resume_cleanups, 0);
    QUIT;
  
! #ifdef CANNOT_STEP_BREAKPOINT
!   /* Most targets can step a breakpoint instruction, thus executing it
!      normally.  But if this one cannot, just continue and we will hit
!      it anyway.  */
!   if (step && breakpoints_inserted && breakpoint_here_p (read_pc ()))
!     step = 0;
! #endif
  
    /* Some targets (e.g. Solaris x86) have a kernel bug when stepping
       over an instruction that causes a page fault without triggering
       a hardware watchpoint. The kernel properly notices that it shouldn't
--- 796,804 ----
    struct cleanup *old_cleanups = make_cleanup (resume_cleanups, 0);
    QUIT;
  
!   /* FIXME: calling breakpoint_here_p (read_pc ()) three times! */
  
+ 
    /* Some targets (e.g. Solaris x86) have a kernel bug when stepping
       over an instruction that causes a page fault without triggering
       a hardware watchpoint. The kernel properly notices that it shouldn't
*************** resume (int step, enum target_signal sig
*** 909,950 ****
    if (should_resume)
      {
        ptid_t resume_ptid;
  
!       if (use_thread_step_needed && thread_step_needed)
  	{
! 	  /* We stopped on a BPT instruction;
! 	     don't continue other threads and
! 	     just step this thread. */
! 	  thread_step_needed = 0;
  
! 	  if (!breakpoint_here_p (read_pc ()))
! 	    {
! 	      /* Breakpoint deleted: ok to do regular resume
! 	         where all the threads either step or continue. */
! 	      resume_ptid = RESUME_ALL;
! 	    }
! 	  else
! 	    {
! 	      if (!step)
! 		{
! 		  warning ("Internal error, changing continue to step.");
! 		  remove_breakpoints ();
! 		  breakpoints_inserted = 0;
! 		  trap_expected = 1;
! 		  step = 1;
! 		}
! 	      resume_ptid = inferior_ptid;
! 	    }
  	}
!       else
  	{
! 	  /* Vanilla resume. */
! 	  if ((scheduler_mode == schedlock_on) ||
! 	      (scheduler_mode == schedlock_step && step != 0))
  	    resume_ptid = inferior_ptid;
- 	  else
- 	    resume_ptid = RESUME_ALL;
  	}
        target_resume (resume_ptid, step, sig);
      }
  
--- 879,913 ----
    if (should_resume)
      {
        ptid_t resume_ptid;
+ 
+       resume_ptid = RESUME_ALL;		/* Default */
  
!       if ((step || singlestep_breakpoints_inserted_p) &&
! 	  !breakpoints_inserted && breakpoint_here_p (read_pc ()))
  	{
! 	  /* Stepping past a breakpoint without inserting breakpoints.
! 	     Make sure only the current thread gets to step, so that
! 	     other threads don't sneak past breakpoints while they are
! 	     not inserted. */
  
! 	  resume_ptid = inferior_ptid;
  	}
! 
!       if ((scheduler_mode == schedlock_on) ||
! 	  (scheduler_mode == schedlock_step && 
! 	   (step || singlestep_breakpoints_inserted_p)))
  	{
! 	  /* User-settable 'scheduler' mode requires solo thread resume. */
  	    resume_ptid = inferior_ptid;
  	}
+ 
+ #ifdef CANNOT_STEP_BREAKPOINT
+       /* Most targets can step a breakpoint instruction, thus executing it
+ 	 normally.  But if this one cannot, just continue and we will hit
+ 	 it anyway.  */
+       if (step && breakpoints_inserted && breakpoint_here_p (read_pc ()))
+ 	step = 0;
+ #endif
        target_resume (resume_ptid, step, sig);
      }
  
*************** proceed (CORE_ADDR addr, enum target_sig
*** 1019,1034 ****
    else
      {
        write_pc (addr);
- 
-       /* New address; we don't need to single-step a thread
-          over a breakpoint we just hit, 'cause we aren't
-          continuing from there.
- 
-          It's not worth worrying about the case where a user
-          asks for a "jump" at the current PC--if they get the
-          hiccup of re-hiting a hit breakpoint, what else do
-          they expect? */
-       thread_step_needed = 0;
      }
  
  #ifdef PREPARE_TO_PROCEED
--- 982,987 ----
*************** proceed (CORE_ADDR addr, enum target_sig
*** 1046,1052 ****
    if (PREPARE_TO_PROCEED (1) && breakpoint_here_p (read_pc ()))
      {
        oneproc = 1;
-       thread_step_needed = 1;
      }
  
  #endif /* PREPARE_TO_PROCEED */
--- 999,1004 ----
*************** wait_for_inferior (void)
*** 1287,1294 ****
    /* Fill in with reasonable starting values.  */
    init_execution_control_state (ecs);
  
-   thread_step_needed = 0;
- 
    /* We'll update this if & when we switch to a new thread. */
    previous_inferior_ptid = inferior_ptid;
  
--- 1239,1244 ----
*************** fetch_inferior_event (void *client_data)
*** 1347,1354 ****
        /* Fill in with reasonable starting values.  */
        init_execution_control_state (async_ecs);
  
-       thread_step_needed = 0;
- 
        /* We'll update this if & when we switch to a new thread. */
        previous_inferior_ptid = inferior_ptid;
  
--- 1297,1302 ----
*************** handle_inferior_event (struct execution_
*** 1498,1508 ****
  	/* Fall thru to the normal_state case. */
  
        case infwait_normal_state:
- 	/* Since we've done a wait, we have a new event.  Don't
- 	   carry over any expectations about needing to step over a
- 	   breakpoint. */
- 	thread_step_needed = 0;
- 
  	/* See comments where a TARGET_WAITKIND_SYSCALL_RETURN event
  	   is serviced in this loop, below. */
  	if (ecs->enable_hw_watchpoints_after_wait)
--- 1446,1451 ----
*************** handle_inferior_event (struct execution_
*** 1934,1963 ****
  		      context_switch (ecs);
  		    ecs->waiton_ptid = ecs->ptid;
  		    ecs->wp = &(ecs->ws);
- 		    thread_step_needed = 1;
  		    ecs->another_trap = 1;
  
- 		    /* keep_stepping will call resume, and the
- 		       combination of "thread_step_needed" and
- 		       "ecs->another_trap" will cause resume to
- 		       solo-step the thread.  The subsequent trap
- 		       event will be handled like any other singlestep
- 		       event. */
- 
  		    ecs->infwait_state = infwait_thread_hop_state;
  		    keep_going (ecs);
  		    registers_changed ();
  		    return;
  		  }
  	      }
- 	    else
- 	      {
- 		/* This breakpoint matches--either it is the right
- 		   thread or it's a generic breakpoint for all threads.
- 		   Remember that we'll need to step just _this_ thread
- 		   on any following user continuation! */
- 		thread_step_needed = 1;
- 	      }
  	  }
        }
      else
--- 1877,1890 ----
*************** handle_inferior_event (struct execution_
*** 2413,2419 ****
  	case BPSTAT_WHAT_SINGLE:
  	  if (breakpoints_inserted)
  	    {
- 	      thread_step_needed = 1;
  	      remove_breakpoints ();
  	    }
  	  breakpoints_inserted = 0;
--- 2340,2345 ----

Re: [PATCH/RFC] Improve the "thread_step_needed" logic

[Daniel Jacobowitz]

On Fri, Jun 15, 2001 at 04:35:09PM -0700, Michael Snyder wrote:
> This is a fairly significant change, that both simplifies and
> improves the logic for deciding when a single thread should be
> stepped (to get past a breakpoint), rather than stepping all threads.
> In a nutshell, this replaces the state variable "thread_step_needed"
> with the following logic in resume():

<sigh>

While you're fixing this, could you please address my issue from last
week with this code?  Checking "step" at this point in resume() is not
correct, since it will have been cleared if SOFTWARE_SINGLE_STEP_P ().

> --- 879,913 ----
>     if (should_resume)
>       {
>         ptid_t resume_ptid;
> + 
> +       resume_ptid = RESUME_ALL;		/* Default */
>   
> !       if ((step || singlestep_breakpoints_inserted_p) &&
> ! 	  !breakpoints_inserted && breakpoint_here_p (read_pc ()))

> + 
> + #ifdef CANNOT_STEP_BREAKPOINT
> +       /* Most targets can step a breakpoint instruction, thus executing it
> + 	 normally.  But if this one cannot, just continue and we will hit
> + 	 it anyway.  */
> +       if (step && breakpoints_inserted && breakpoint_here_p (read_pc ()))
> + 	step = 0;
> + #endif


Specifically, those bits.

-- 
Daniel Jacobowitz                           Debian GNU/Linux Developer
Monta Vista Software                              Debian Security Team

Re: [PATCH/RFC] Improve the "thread_step_needed" logic

[Michael Snyder]

Daniel Jacobowitz wrote:
> 
> On Fri, Jun 15, 2001 at 04:35:09PM -0700, Michael Snyder wrote:
> > This is a fairly significant change, that both simplifies and
> > improves the logic for deciding when a single thread should be
> > stepped (to get past a breakpoint), rather than stepping all threads.
> > In a nutshell, this replaces the state variable "thread_step_needed"
> > with the following logic in resume():
> 
> <sigh>
> 
> While you're fixing this, could you please address my issue from last
> week with this code? 

I'm pretty sure I did!

 Checking "step" at this point in resume() is not
> correct, since it will have been cleared if SOFTWARE_SINGLE_STEP_P ().

But I don't just check "step", I check
"(step || singlestep_breakpoints_inserted_p)".

That was specifically to address your issue.
Do you think it fails to do so?

> 
> > --- 879,913 ----
> >     if (should_resume)
> >       {
> >         ptid_t resume_ptid;
> > +
> > +       resume_ptid = RESUME_ALL;             /* Default */
> >
> > !       if ((step || singlestep_breakpoints_inserted_p) &&
> > !       !breakpoints_inserted && breakpoint_here_p (read_pc ()))
> 
> > +
> > + #ifdef CANNOT_STEP_BREAKPOINT
> > +       /* Most targets can step a breakpoint instruction, thus executing it
> > +      normally.  But if this one cannot, just continue and we will hit
> > +      it anyway.  */
> > +       if (step && breakpoints_inserted && breakpoint_here_p (read_pc ()))
> > +     step = 0;
> > + #endif
> 
> Specifically, those bits.
> 
> --
> Daniel Jacobowitz                           Debian GNU/Linux Developer
> Monta Vista Software                              Debian Security Team

Re: [PATCH/RFC] Improve the "thread_step_needed" logic

[Michael Snyder]

Daniel Jacobowitz wrote:
> 
> On Fri, Jun 15, 2001 at 04:35:09PM -0700, Michael Snyder wrote:
> > This is a fairly significant change, that both simplifies and
> > improves the logic for deciding when a single thread should be
> > stepped (to get past a breakpoint), rather than stepping all threads.
> > In a nutshell, this replaces the state variable "thread_step_needed"
> > with the following logic in resume():
> 
> <sigh>
> 
> While you're fixing this, could you please address my issue from last
> week with this code?  Checking "step" at this point in resume() is not
> correct, since it will have been cleared if SOFTWARE_SINGLE_STEP_P ().
> 
> > --- 879,913 ----
> >     if (should_resume)
> >       {
> >         ptid_t resume_ptid;
> > +
> > +       resume_ptid = RESUME_ALL;             /* Default */
> >
> > !       if ((step || singlestep_breakpoints_inserted_p) &&
> > !       !breakpoints_inserted && breakpoint_here_p (read_pc ()))
> 
> > +
> > + #ifdef CANNOT_STEP_BREAKPOINT
> > +       /* Most targets can step a breakpoint instruction, thus executing it
> > +      normally.  But if this one cannot, just continue and we will hit
> > +      it anyway.  */
> > +       if (step && breakpoints_inserted && breakpoint_here_p (read_pc ()))
> > +     step = 0;
> > + #endif
> 
> Specifically, those bits.

Oh, sorry, I missed this part of your msg.  
Well, this isn't related to SOFTWARE_SINGLESTEP_P, 
it's a separate issue.  But if you'll notice, I believe this is also 
handled correctly, because my "thread_step_needed" test includes
(NOT breakpoints_inserted), while this CANNOT_STEP_BREAKPOINT clause
will only be taken if breakpoints_inserted is true.  So they are
mutually exclusive, if you will.  I tested this on Alpha Linux, which
does NOT use software single step, but DOES use CANNOT_STEP_BREAKPOINT,
and it works.

Michael

Re: [PATCH/RFC] Improve the "thread_step_needed" logic

[Daniel Jacobowitz]

On Fri, Jun 15, 2001 at 06:13:56PM -0700, Michael Snyder wrote:
> > <sigh>
> > 
> > While you're fixing this, could you please address my issue from last
> > week with this code?  Checking "step" at this point in resume() is not
> > correct, since it will have been cleared if SOFTWARE_SINGLE_STEP_P ().
> > 
> > > --- 879,913 ----
> > >     if (should_resume)
> > >       {
> > >         ptid_t resume_ptid;
> > > +
> > > +       resume_ptid = RESUME_ALL;             /* Default */
> > >
> > > !       if ((step || singlestep_breakpoints_inserted_p) &&
> > > !       !breakpoints_inserted && breakpoint_here_p (read_pc ()))
> > 
> > > +
> > > + #ifdef CANNOT_STEP_BREAKPOINT
> > > +       /* Most targets can step a breakpoint instruction, thus executing it
> > > +      normally.  But if this one cannot, just continue and we will hit
> > > +      it anyway.  */
> > > +       if (step && breakpoints_inserted && breakpoint_here_p (read_pc ()))
> > > +     step = 0;
> > > + #endif
> > 
> > Specifically, those bits.
> 
> Oh, sorry, I missed this part of your msg.  
> Well, this isn't related to SOFTWARE_SINGLESTEP_P, 
> it's a separate issue.  But if you'll notice, I believe this is also 
> handled correctly, because my "thread_step_needed" test includes
> (NOT breakpoints_inserted), while this CANNOT_STEP_BREAKPOINT clause
> will only be taken if breakpoints_inserted is true.  So they are
> mutually exclusive, if you will.  I tested this on Alpha Linux, which
> does NOT use software single step, but DOES use CANNOT_STEP_BREAKPOINT,
> and it works.

My apologies.  I need to read closer before replying :)

I'm assuming that singlestep_breakpoints_inserted_p is zero normally
upon entering resume - that only makes sense.  In that case, your
change preserves the exact behavior I was asking for.  I'm going to run
it through the testsuite on MIPS/Linux, which I've finally gotten into
(almost) functional shape, and let you know if something breaks - I
expect that it will not break, though.

-- 
Daniel Jacobowitz                           Debian GNU/Linux Developer
Monta Vista Software                              Debian Security Team

Re: [PATCH/RFC] Improve the "thread_step_needed" logic

[Michael Snyder]

Daniel Jacobowitz wrote:
> 
> On Fri, Jun 15, 2001 at 06:13:56PM -0700, Michael Snyder wrote:
> > > <sigh>
> > >
> > > While you're fixing this, could you please address my issue from last
> > > week with this code?  Checking "step" at this point in resume() is not
> > > correct, since it will have been cleared if SOFTWARE_SINGLE_STEP_P ().
> > >
> > > > --- 879,913 ----
> > > >     if (should_resume)
> > > >       {
> > > >         ptid_t resume_ptid;
> > > > +
> > > > +       resume_ptid = RESUME_ALL;             /* Default */
> > > >
> > > > !       if ((step || singlestep_breakpoints_inserted_p) &&
> > > > !       !breakpoints_inserted && breakpoint_here_p (read_pc ()))
> > >
> > > > +
> > > > + #ifdef CANNOT_STEP_BREAKPOINT
> > > > +       /* Most targets can step a breakpoint instruction, thus executing it
> > > > +      normally.  But if this one cannot, just continue and we will hit
> > > > +      it anyway.  */
> > > > +       if (step && breakpoints_inserted && breakpoint_here_p (read_pc ()))
> > > > +     step = 0;
> > > > + #endif
> > >
> > > Specifically, those bits.
> >
> > Oh, sorry, I missed this part of your msg.
> > Well, this isn't related to SOFTWARE_SINGLESTEP_P,
> > it's a separate issue.  But if you'll notice, I believe this is also
> > handled correctly, because my "thread_step_needed" test includes
> > (NOT breakpoints_inserted), while this CANNOT_STEP_BREAKPOINT clause
> > will only be taken if breakpoints_inserted is true.  So they are
> > mutually exclusive, if you will.  I tested this on Alpha Linux, which
> > does NOT use software single step, but DOES use CANNOT_STEP_BREAKPOINT,
> > and it works.
> 
> My apologies.  I need to read closer before replying :)
> 
> I'm assuming that singlestep_breakpoints_inserted_p is zero normally
> upon entering resume - that only makes sense.  In that case, your
> change preserves the exact behavior I was asking for.  I'm going to run
> it through the testsuite on MIPS/Linux, which I've finally gotten into
> (almost) functional shape, and let you know if something breaks - I
> expect that it will not break, though.

Since I do not have a Mips Linux system, I will be
very interested in your results.

Thanks,
Michael

Re: [PATCH/RFC] Improve the "thread_step_needed" logic

[Daniel Jacobowitz]

On Mon, Jun 18, 2001 at 09:11:37AM -0700, Michael Snyder wrote:
> Daniel Jacobowitz wrote:
> > My apologies.  I need to read closer before replying :)
> > 
> > I'm assuming that singlestep_breakpoints_inserted_p is zero normally
> > upon entering resume - that only makes sense.  In that case, your
> > change preserves the exact behavior I was asking for.  I'm going to run
> > it through the testsuite on MIPS/Linux, which I've finally gotten into
> > (almost) functional shape, and let you know if something breaks - I
> > expect that it will not break, though.
> 
> Since I do not have a Mips Linux system, I will be
> very interested in your results.

Nothing blew up - in fact, between that and another patch I have my
closest to a successful test run yet.  Not great, but:

                === gdb Summary ===

# of expected passes            6603
# of unexpected failures        141
# of unexpected successes       32
# of expected failures          161
# of unresolved testcases       26
# of untested testcases         24

Thanks for your help!

-- 
Daniel Jacobowitz                           Debian GNU/Linux Developer
Monta Vista Software                              Debian Security Team

Re: [PATCH/RFC] Improve the "thread_step_needed" logic

[Michael Snyder]

Committed.


Michael Snyder wrote:
> 
> This is a fairly significant change, that both simplifies and
> improves the logic for deciding when a single thread should be
> stepped (to get past a breakpoint), rather than stepping all threads.
> In a nutshell, this replaces the state variable "thread_step_needed"
> with the following logic in resume():
> 
>       resume_ptid = RESUME_ALL;         /* Default */
> 
>       if ((step || singlestep_breakpoints_inserted_p) &&
>           !breakpoints_inserted && breakpoint_here_p (read_pc ()))
>         {
>           /* Stepping past a breakpoint without inserting breakpoints.
>              Make sure only the current thread gets to step, so that
>              other threads don't sneak past breakpoints while they are
>              not inserted. */
> 
>           resume_ptid = inferior_ptid;
>         }
> 
> I have run the thread testsuites on Linux, and done rather extensive
> hand-testing.  The behavior is clearly improved by this change --
> it eliminates instances where threads are allowed to run away because
> they are resumed while breakpoints are not inserted.
> 
> I'll wait a week or two for people to yell before committing this.
> 
> 2001-06-15  Michael Snyder  <msnyder@redhat.com>
> 
>         * infrun.c: Eliminate the "thread_step_needed" state variable,
>         and replace it with a relatively simple test in resume.
>         (resume): Replace thread_step_needed logic with a test for
>         stepping, breakpoint_here_p and breakpoints_inserted.
>         Move CANNOT_STEP_BREAKPOINT logic to after thread_step logic.
>         (proceed): Discard thread_step_needed logic.
>         (wait_for_inferior, fetch_inferior_event, handle_inferior_event):
>         Discard thread_step_needed logic.
> 
> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.37
> diff -c -3 -p -r1.37 infrun.c
> *** infrun.c    2001/06/15 22:44:20     1.37
> --- infrun.c    2001/06/15 23:23:48
> *************** static ptid_t previous_inferior_ptid;
> *** 110,140 ****
> 
>   static int may_follow_exec = MAY_FOLLOW_EXEC;
> 
> - /* resume and wait_for_inferior use this to ensure that when
> -    stepping over a hit breakpoint in a threaded application
> -    only the thread that hit the breakpoint is stepped and the
> -    other threads don't continue.  This prevents having another
> -    thread run past the breakpoint while it is temporarily
> -    removed.
> -
> -    This is not thread-specific, so it isn't saved as part of
> -    the infrun state.
> -
> -    Versions of gdb which don't use the "step == this thread steps
> -    and others continue" model but instead use the "step == this
> -    thread steps and others wait" shouldn't do this.  */
> -
> - static int thread_step_needed = 0;
> -
> - /* This is true if thread_step_needed should actually be used.  At
> -    present this is only true for HP-UX native.  */
> -
> - #ifndef USE_THREAD_STEP_NEEDED
> - #define USE_THREAD_STEP_NEEDED (0)
> - #endif
> -
> - static int use_thread_step_needed = USE_THREAD_STEP_NEEDED;
> -
>   /* GET_LONGJMP_TARGET returns the PC at which longjmp() will resume the
>      program.  It needs to examine the jmp_buf argument and extract the PC
>      from it.  The return value is non-zero on success, zero otherwise. */
> --- 110,115 ----
> *************** resume (int step, enum target_signal sig
> *** 821,834 ****
>     struct cleanup *old_cleanups = make_cleanup (resume_cleanups, 0);
>     QUIT;
> 
> ! #ifdef CANNOT_STEP_BREAKPOINT
> !   /* Most targets can step a breakpoint instruction, thus executing it
> !      normally.  But if this one cannot, just continue and we will hit
> !      it anyway.  */
> !   if (step && breakpoints_inserted && breakpoint_here_p (read_pc ()))
> !     step = 0;
> ! #endif
> 
>     /* Some targets (e.g. Solaris x86) have a kernel bug when stepping
>        over an instruction that causes a page fault without triggering
>        a hardware watchpoint. The kernel properly notices that it shouldn't
> --- 796,804 ----
>     struct cleanup *old_cleanups = make_cleanup (resume_cleanups, 0);
>     QUIT;
> 
> !   /* FIXME: calling breakpoint_here_p (read_pc ()) three times! */
> 
> +
>     /* Some targets (e.g. Solaris x86) have a kernel bug when stepping
>        over an instruction that causes a page fault without triggering
>        a hardware watchpoint. The kernel properly notices that it shouldn't
> *************** resume (int step, enum target_signal sig
> *** 909,950 ****
>     if (should_resume)
>       {
>         ptid_t resume_ptid;
> 
> !       if (use_thread_step_needed && thread_step_needed)
>         {
> !         /* We stopped on a BPT instruction;
> !            don't continue other threads and
> !            just step this thread. */
> !         thread_step_needed = 0;
> 
> !         if (!breakpoint_here_p (read_pc ()))
> !           {
> !             /* Breakpoint deleted: ok to do regular resume
> !                where all the threads either step or continue. */
> !             resume_ptid = RESUME_ALL;
> !           }
> !         else
> !           {
> !             if (!step)
> !               {
> !                 warning ("Internal error, changing continue to step.");
> !                 remove_breakpoints ();
> !                 breakpoints_inserted = 0;
> !                 trap_expected = 1;
> !                 step = 1;
> !               }
> !             resume_ptid = inferior_ptid;
> !           }
>         }
> !       else
>         {
> !         /* Vanilla resume. */
> !         if ((scheduler_mode == schedlock_on) ||
> !             (scheduler_mode == schedlock_step && step != 0))
>             resume_ptid = inferior_ptid;
> -         else
> -           resume_ptid = RESUME_ALL;
>         }
>         target_resume (resume_ptid, step, sig);
>       }
> 
> --- 879,913 ----
>     if (should_resume)
>       {
>         ptid_t resume_ptid;
> +
> +       resume_ptid = RESUME_ALL;               /* Default */
> 
> !       if ((step || singlestep_breakpoints_inserted_p) &&
> !         !breakpoints_inserted && breakpoint_here_p (read_pc ()))
>         {
> !         /* Stepping past a breakpoint without inserting breakpoints.
> !            Make sure only the current thread gets to step, so that
> !            other threads don't sneak past breakpoints while they are
> !            not inserted. */
> 
> !         resume_ptid = inferior_ptid;
>         }
> !
> !       if ((scheduler_mode == schedlock_on) ||
> !         (scheduler_mode == schedlock_step &&
> !          (step || singlestep_breakpoints_inserted_p)))
>         {
> !         /* User-settable 'scheduler' mode requires solo thread resume. */
>             resume_ptid = inferior_ptid;
>         }
> +
> + #ifdef CANNOT_STEP_BREAKPOINT
> +       /* Most targets can step a breakpoint instruction, thus executing it
> +        normally.  But if this one cannot, just continue and we will hit
> +        it anyway.  */
> +       if (step && breakpoints_inserted && breakpoint_here_p (read_pc ()))
> +       step = 0;
> + #endif
>         target_resume (resume_ptid, step, sig);
>       }
> 
> *************** proceed (CORE_ADDR addr, enum target_sig
> *** 1019,1034 ****
>     else
>       {
>         write_pc (addr);
> -
> -       /* New address; we don't need to single-step a thread
> -          over a breakpoint we just hit, 'cause we aren't
> -          continuing from there.
> -
> -          It's not worth worrying about the case where a user
> -          asks for a "jump" at the current PC--if they get the
> -          hiccup of re-hiting a hit breakpoint, what else do
> -          they expect? */
> -       thread_step_needed = 0;
>       }
> 
>   #ifdef PREPARE_TO_PROCEED
> --- 982,987 ----
> *************** proceed (CORE_ADDR addr, enum target_sig
> *** 1046,1052 ****
>     if (PREPARE_TO_PROCEED (1) && breakpoint_here_p (read_pc ()))
>       {
>         oneproc = 1;
> -       thread_step_needed = 1;
>       }
> 
>   #endif /* PREPARE_TO_PROCEED */
> --- 999,1004 ----
> *************** wait_for_inferior (void)
> *** 1287,1294 ****
>     /* Fill in with reasonable starting values.  */
>     init_execution_control_state (ecs);
> 
> -   thread_step_needed = 0;
> -
>     /* We'll update this if & when we switch to a new thread. */
>     previous_inferior_ptid = inferior_ptid;
> 
> --- 1239,1244 ----
> *************** fetch_inferior_event (void *client_data)
> *** 1347,1354 ****
>         /* Fill in with reasonable starting values.  */
>         init_execution_control_state (async_ecs);
> 
> -       thread_step_needed = 0;
> -
>         /* We'll update this if & when we switch to a new thread. */
>         previous_inferior_ptid = inferior_ptid;
> 
> --- 1297,1302 ----
> *************** handle_inferior_event (struct execution_
> *** 1498,1508 ****
>         /* Fall thru to the normal_state case. */
> 
>         case infwait_normal_state:
> -       /* Since we've done a wait, we have a new event.  Don't
> -          carry over any expectations about needing to step over a
> -          breakpoint. */
> -       thread_step_needed = 0;
> -
>         /* See comments where a TARGET_WAITKIND_SYSCALL_RETURN event
>            is serviced in this loop, below. */
>         if (ecs->enable_hw_watchpoints_after_wait)
> --- 1446,1451 ----
> *************** handle_inferior_event (struct execution_
> *** 1934,1963 ****
>                       context_switch (ecs);
>                     ecs->waiton_ptid = ecs->ptid;
>                     ecs->wp = &(ecs->ws);
> -                   thread_step_needed = 1;
>                     ecs->another_trap = 1;
> 
> -                   /* keep_stepping will call resume, and the
> -                      combination of "thread_step_needed" and
> -                      "ecs->another_trap" will cause resume to
> -                      solo-step the thread.  The subsequent trap
> -                      event will be handled like any other singlestep
> -                      event. */
> -
>                     ecs->infwait_state = infwait_thread_hop_state;
>                     keep_going (ecs);
>                     registers_changed ();
>                     return;
>                   }
>               }
> -           else
> -             {
> -               /* This breakpoint matches--either it is the right
> -                  thread or it's a generic breakpoint for all threads.
> -                  Remember that we'll need to step just _this_ thread
> -                  on any following user continuation! */
> -               thread_step_needed = 1;
> -             }
>           }
>         }
>       else
> --- 1877,1890 ----
> *************** handle_inferior_event (struct execution_
> *** 2413,2419 ****
>         case BPSTAT_WHAT_SINGLE:
>           if (breakpoints_inserted)
>             {
> -             thread_step_needed = 1;
>               remove_breakpoints ();
>             }
>           breakpoints_inserted = 0;
> --- 2340,2345 ----