[PATCH/RFC] Improve the "thread_step_needed" logic

[Michael Snyder <msnyder at mvstp600e dot cygnus dot com>]

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