[patch/rfc] Fix step/next across signals

[patch/rfc] Fix step/next across signals

[Andrew Cagney]

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

Hello,

The attached fixes a bug with step/next across signals.  Given:

	Breakpoint at 10 hit
	10	while (!done);
	(gdb) next

when there was a signal pending (that set DONE), GDB would:

Attempt to step of the breakpoint at "10":
- pull all breakpoints
- PT_STEP (to get off the BP instruction)
- get back the signal instead

Attempt to skip the signal handler:
- add a step_resume_breakpoint at "10" the signal return addr
- PT_STEP delivering the signal
- insert all breakpoints (including step_resume)
- PT_CONTINUE the inferior
- get back SIGTRAP from the step_resume breakpoint
- delete the step_resume bp

Go back to doing the next:
- PT_STEP the inferior (breakpoints including "10" still inserted)
- re-hit "10"

the problem is that GDB forgot that it was, at the time of the signal, 
trying to step off a breakpoint.

The attached patch fixes this, it notes if/when it was stepping off a 
breakpoint, so that it can return to that task once the 
step-resume-breakpoint is hit.

I've tested it on a my patched PPC/NetBSD kernel and it KPASSes 1757; 
and a vanila rhel3u2 system with no test changes.

Since the 1757 KFAILs pass, I've removed them as `obvious'.

comments?
Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 5420 bytes --]

Index: testsuite/ChangeLog
2004-08-26  Andrew Cagney  <cagney@gnu.org>

	* gdb.base/sigstep.exp (breakpoint_over_handler): Remove kfail
	gdb/1757.

Index: ChangeLog
2004-08-26  Andrew Cagney  <cagney@gnu.org>

	Fix PR breakpoints/1757.
	* infrun.c (struct execution_control_state): Replace
	remove_breakpoints_on_following_step with
	step_after_step_resume_breakpoint.
	(init_execution_control_state): Update.
	(handle_inferior_event): For signals, when stepping off a
	breakpoint, set step_after_step_resume_breakpoint.  When
	BPSTAT_WHAT_STEP_RESUME, do a single-step off the breakpoint.
	(keep_going): Delete code handling
	remove_breakpoints_on_following_step.

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.175
diff -p -u -r1.175 infrun.c
--- infrun.c	25 Aug 2004 15:18:05 -0000	1.175
+++ infrun.c	27 Aug 2004 15:19:03 -0000
@@ -914,12 +914,12 @@ struct execution_control_state
   CORE_ADDR stop_func_end;
   char *stop_func_name;
   struct symtab_and_line sal;
-  int remove_breakpoints_on_following_step;
   int current_line;
   struct symtab *current_symtab;
   int handling_longjmp;		/* FIXME */
   ptid_t ptid;
   ptid_t saved_inferior_ptid;
+  int step_after_step_resume_breakpoint;
   int stepping_through_solib_after_catch;
   bpstat stepping_through_solib_catchpoints;
   int enable_hw_watchpoints_after_wait;
@@ -1068,7 +1068,7 @@ init_execution_control_state (struct exe
 {
   /* ecs->another_trap? */
   ecs->random_signal = 0;
-  ecs->remove_breakpoints_on_following_step = 0;
+  ecs->step_after_step_resume_breakpoint = 0;
   ecs->handling_longjmp = 0;	/* FIXME */
   ecs->stepping_through_solib_after_catch = 0;
   ecs->stepping_through_solib_catchpoints = NULL;
@@ -1932,10 +1932,29 @@ process_event_stop_test:
       if (signal_program[stop_signal] == 0)
 	stop_signal = TARGET_SIGNAL_0;
 
-      if (step_range_end != 0
-	  && stop_signal != TARGET_SIGNAL_0
-	  && stop_pc >= step_range_start && stop_pc < step_range_end
-	  && frame_id_eq (get_frame_id (get_current_frame ()), step_frame_id))
+      if (prev_pc == read_pc ()
+	  && !breakpoints_inserted
+	  && breakpoint_here_p (read_pc ())
+	  && step_resume_breakpoint == NULL)
+	{
+	  /* We were just starting a new sequence, attempting to
+	     single-step off of a breakpoint and expecting a SIGTRAP.
+	     Intead this signal arrives.  This signal will take us out
+	     of the stepping range so GDB needs to remember to, when
+	     the signal handler returns, resume stepping off that
+	     breakpoint.  */
+	  /* To simplify things, "continue" is forced to use the same
+	     code paths as single-step - set a breakpoint at the
+	     signal return address and then, once hit, step off that
+	     breakpoint.  */
+	  insert_step_resume_breakpoint (get_current_frame (), ecs);
+	  ecs->step_after_step_resume_breakpoint = 1;
+	}
+      else if (step_range_end != 0
+	       && stop_signal != TARGET_SIGNAL_0
+	       && stop_pc >= step_range_start && stop_pc < step_range_end
+	       && frame_id_eq (get_frame_id (get_current_frame ()),
+			       step_frame_id))
 	{
 	  /* The inferior is about to take a signal that will take it
 	     out of the single step range.  Set a breakpoint at the
@@ -2054,6 +2073,18 @@ process_event_stop_test:
 	      bpstat_find_step_resume_breakpoint (stop_bpstat);
 	  }
 	delete_step_resume_breakpoint (&step_resume_breakpoint);
+	if (ecs->step_after_step_resume_breakpoint)
+	  {
+	    /* Back when the step-resume breakpoint was inserted, we
+	       were trying to single-step off a breakpoint.  Go back
+	       to doing that.  */
+	    ecs->step_after_step_resume_breakpoint = 0;
+	    remove_breakpoints ();
+	    breakpoints_inserted = 0;
+	    ecs->another_trap = 1;
+	    keep_going (ecs);
+	    return;
+	  }
 	break;
 
       case BPSTAT_WHAT_THROUGH_SIGTRAMP:
@@ -2700,20 +2731,9 @@ keep_going (struct execution_control_sta
          The signal was SIGTRAP, e.g. it was our signal, but we
          decided we should resume from it.
 
-         We're going to run this baby now!
+         We're going to run this baby now!  */
 
-         Insert breakpoints now, unless we are trying to one-proceed
-         past a breakpoint.  */
-      /* If we've just finished a special step resume and we don't
-         want to hit a breakpoint, pull em out.  */
-      if (step_resume_breakpoint == NULL
-	  && ecs->remove_breakpoints_on_following_step)
-	{
-	  ecs->remove_breakpoints_on_following_step = 0;
-	  remove_breakpoints ();
-	  breakpoints_inserted = 0;
-	}
-      else if (!breakpoints_inserted && !ecs->another_trap)
+      if (!breakpoints_inserted && !ecs->another_trap)
 	{
 	  breakpoints_failed = insert_breakpoints ();
 	  if (breakpoints_failed)
Index: testsuite/gdb.base/sigstep.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/sigstep.exp,v
retrieving revision 1.5
diff -p -u -r1.5 sigstep.exp
--- testsuite/gdb.base/sigstep.exp	25 Aug 2004 15:26:19 -0000	1.5
+++ testsuite/gdb.base/sigstep.exp	27 Aug 2004 15:19:18 -0000
@@ -328,7 +328,6 @@ proc breakpoint_over_handler { i } {
     # Make the signal pending
     sleep 1
     
-    setup_kfail "powerpc*-*-*" gdb/1757
     gdb_test "$i" "done = 0.*" "$prefix; performing $i"
     gdb_test "clear $infinite_loop" "" "$prefix; clear infinite loop"
 }

Re: [patch/rfc] Fix step/next across signals

[Daniel Jacobowitz]

On Fri, Aug 27, 2004 at 11:37:59AM -0400, Andrew Cagney wrote:
> comments?

Looks great to me!

-- 
Daniel Jacobowitz

Re: [patch/rfc] Fix step/next across signals

[Michael Chastain]

Hah, a signals test patch that I can understand.  :)

  -    setup_kfail "powerpc*-*-*" gdb/1757

This part is approved.  Commit it whenever you think it's right.

Michael

Re: [patch/rfc] Fix step/next across signals

[Andrew Cagney]

I've checked this in.

Andrew

> Hello,
> 
> The attached fixes a bug with step/next across signals.  Given:
> 
>     Breakpoint at 10 hit
>     10    while (!done);
>     (gdb) next
> 
> when there was a signal pending (that set DONE), GDB would:
> 
> Attempt to step of the breakpoint at "10":
> - pull all breakpoints
> - PT_STEP (to get off the BP instruction)
> - get back the signal instead
> 
> Attempt to skip the signal handler:
> - add a step_resume_breakpoint at "10" the signal return addr
> - PT_STEP delivering the signal
> - insert all breakpoints (including step_resume)
> - PT_CONTINUE the inferior
> - get back SIGTRAP from the step_resume breakpoint
> - delete the step_resume bp
> 
> Go back to doing the next:
> - PT_STEP the inferior (breakpoints including "10" still inserted)
> - re-hit "10"
> 
> the problem is that GDB forgot that it was, at the time of the signal, trying to step off a breakpoint.
> 
> The attached patch fixes this, it notes if/when it was stepping off a breakpoint, so that it can return to that task once the step-resume-breakpoint is hit.
> 
> I've tested it on a my patched PPC/NetBSD kernel and it KPASSes 1757; and a vanila rhel3u2 system with no test changes.
> 
> Since the 1757 KFAILs pass, I've removed them as `obvious'.
> 
> comments?
> Andrew
> 
> 
> 
> Index: testsuite/ChangeLog
> 2004-08-26  Andrew Cagney  <cagney@gnu.org>
> 
> 	* gdb.base/sigstep.exp (breakpoint_over_handler): Remove kfail
> 	gdb/1757.
> 
> Index: ChangeLog
> 2004-08-26  Andrew Cagney  <cagney@gnu.org>
> 
> 	Fix PR breakpoints/1757.
> 	* infrun.c (struct execution_control_state): Replace
> 	remove_breakpoints_on_following_step with
> 	step_after_step_resume_breakpoint.
> 	(init_execution_control_state): Update.
> 	(handle_inferior_event): For signals, when stepping off a
> 	breakpoint, set step_after_step_resume_breakpoint.  When
> 	BPSTAT_WHAT_STEP_RESUME, do a single-step off the breakpoint.
> 	(keep_going): Delete code handling
> 	remove_breakpoints_on_following_step.
>