[RFA] Clarify infrun variable naming.

[RFA] Clarify infrun variable naming.

[Vladimir Prus]

The infrun.c file has a variable named trap_expected, which
is a bit misleading -- after all, most times when we resume
inferior, we get SIGTRAP. As it turns out, that variable
is set when we're stepping over breakpoints, so a better
name would be stepping_over_breakpoint. Likewise,
ecs->another_trap also indicates that keep_going should
be stepping over breakpoint. The attached patch clarifies
the naming and adds comments, and has no behaviour changes.
(The patch is on top of my previous breakpoints_inserted
removing patch).
OK?

- Volodya

	* infrun.c (trap_expected): Rename
	to stepping_over_breakpoint.  Document.
	(struct execution_control_state): Rename
	the another_trap field to stepping_over_breakpoint.
	(struct inferior_status): Rename the trap_expected
	field to stepping_over_breakpoint.
	(clear_proceed_status, proceed)
	(init_execution_control_state, context_switch)
	(handle_inferior_event, currently_stepping)
	(keep_going, save_inferior_status)
	(restore_inferior_status): Adjust.
	* gdbthread.h (struct thread_info): Rename the
	trap_expected field to stepping_over_breakpoint.
	* thread.c (load_infrun_state, save_infrun_state):
	Adjust.
---
 gdb/gdbthread.h |    2 +-
 gdb/infrun.c    |   91 ++++++++++++++++++++++++++++++++++--------------------
 gdb/thread.c    |    8 ++--
 3 files changed, 62 insertions(+), 39 deletions(-)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index aeb4a40..12e0bcc 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -49,7 +49,7 @@ struct thread_info
   struct symtab *current_symtab;
   int trap_expected;
   int handling_longjmp;
-  int another_trap;
+  int stepping_over_breakpoint;
 
   /* This is set TRUE when a catchpoint of a shared library event
      triggers.  Since we don't wish to leave the inferior in the
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 08b0cf3..05b2743 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -218,9 +218,30 @@ static struct cmd_list_element *stop_command;
 
 static struct symbol *step_start_function;
 
-/* Nonzero if we are expecting a trace trap and should proceed from it.  */
+/* Nonzero if we are presenting stepping over breakpoint.
 
-static int trap_expected;
+   If we hit a breakpoint or watchpoint, and then continue,
+   we need to single step the current thread with breakpoints
+   disabled, so that to avoid hitting the same breakpoint or
+   watchpoint again.  And we should step just a single
+   thread and keep other threads stopped, so that 
+   other threads don't miss breakpoints while they are removed.
+
+   So, this variable simultaneously means that we need to single
+   step current thread, keep other threads stopped, and that
+   breakpoints should be removed while we step.
+
+   This variable is set either:
+   - in proceed, when we resume inferior on explicit user's request
+   - in keep_going, if handle_inferior_event decides we need to
+   step over breakpoint.  
+
+   The variable is cleared in clear_proceed_status, called every
+   time before we call proceed.  The proceed calls wait_for_inferior,
+   which calls handle_inferior_event in a loop, and until 
+   wait_for_inferior exits, this variable is changed only by keep_going.  */
+
+static int stepping_over_breakpoint;
 
 /* Nonzero if we want to give control to the user when we're notified
    of shared library events by the dynamic linker.  */
@@ -646,7 +667,7 @@ a command like `return' or `jump' to continue execution."));
 void
 clear_proceed_status (void)
 {
-  trap_expected = 0;
+  stepping_over_breakpoint = 0;
   step_range_start = 0;
   step_range_end = 0;
   step_frame_id = null_frame_id;
@@ -777,7 +798,7 @@ proceed (CORE_ADDR addr, enum target_signal siggnal, int step)
   if (oneproc)
     /* We will get a trace trap after one instruction.
        Continue it automatically and insert breakpoints then.  */
-    trap_expected = 1;
+    stepping_over_breakpoint = 1;
   else
     insert_breakpoints ();
 
@@ -841,7 +862,7 @@ start_remote (int from_tty)
   init_thread_list ();
   init_wait_for_inferior ();
   stop_soon = STOP_QUIETLY_REMOTE;
-  trap_expected = 0;
+  stepping_over_breakpoint = 0;
 
   /* Always go on waiting for the target, regardless of the mode. */
   /* FIXME: cagney/1999-09-23: At present it isn't possible to
@@ -923,7 +944,9 @@ struct execution_control_state
 {
   struct target_waitstatus ws;
   struct target_waitstatus *wp;
-  int another_trap;
+  /* Should we step over breakpoint next time keep_going 
+     is called?  */
+  int stepping_over_breakpoint;
   int random_signal;
   CORE_ADDR stop_func_start;
   CORE_ADDR stop_func_end;
@@ -1084,7 +1107,7 @@ fetch_inferior_event (void *client_data)
 void
 init_execution_control_state (struct execution_control_state *ecs)
 {
-  ecs->another_trap = 0;
+  ecs->stepping_over_breakpoint = 0;
   ecs->random_signal = 0;
   ecs->step_after_step_resume_breakpoint = 0;
   ecs->handling_longjmp = 0;	/* FIXME */
@@ -1139,20 +1162,20 @@ context_switch (struct execution_control_state *ecs)
     {				/* Perform infrun state context switch: */
       /* Save infrun state for the old thread.  */
       save_infrun_state (inferior_ptid, prev_pc,
-			 trap_expected, step_resume_breakpoint,
+			 stepping_over_breakpoint, step_resume_breakpoint,
 			 step_range_start,
 			 step_range_end, &step_frame_id,
-			 ecs->handling_longjmp, ecs->another_trap,
+			 ecs->handling_longjmp, ecs->stepping_over_breakpoint,
 			 ecs->stepping_through_solib_after_catch,
 			 ecs->stepping_through_solib_catchpoints,
 			 ecs->current_line, ecs->current_symtab);
 
       /* Load infrun state for the new thread.  */
       load_infrun_state (ecs->ptid, &prev_pc,
-			 &trap_expected, &step_resume_breakpoint,
+			 &stepping_over_breakpoint, &step_resume_breakpoint,
 			 &step_range_start,
 			 &step_range_end, &step_frame_id,
-			 &ecs->handling_longjmp, &ecs->another_trap,
+			 &ecs->handling_longjmp, &ecs->stepping_over_breakpoint,
 			 &ecs->stepping_through_solib_after_catch,
 			 &ecs->stepping_through_solib_catchpoints,
 			 &ecs->current_line, &ecs->current_symtab);
@@ -1770,7 +1793,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 		context_switch (ecs);
 	      ecs->waiton_ptid = ecs->ptid;
 	      ecs->wp = &(ecs->ws);
-	      ecs->another_trap = 1;
+	      ecs->stepping_over_breakpoint = 1;
 
 	      ecs->infwait_state = infwait_thread_hop_state;
 	      keep_going (ecs);
@@ -1864,7 +1887,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 			    &ecs->stop_func_start, &ecs->stop_func_end);
   ecs->stop_func_start
     += gdbarch_deprecated_function_start_offset (current_gdbarch);
-  ecs->another_trap = 0;
+  ecs->stepping_over_breakpoint = 0;
   bpstat_clear (&stop_bpstat);
   stop_step = 0;
   stop_stack_dummy = 0;
@@ -1873,7 +1896,7 @@ handle_inferior_event (struct execution_control_state *ecs)
   stopped_by_random_signal = 0;
 
   if (stop_signal == TARGET_SIGNAL_TRAP
-      && trap_expected
+      && stepping_over_breakpoint
       && gdbarch_single_step_through_delay_p (current_gdbarch)
       && currently_stepping (ecs))
     {
@@ -1891,7 +1914,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 	{
 	  /* The user issued a continue when stopped at a breakpoint.
 	     Set up for another trap and get out of here.  */
-         ecs->another_trap = 1;
+         ecs->stepping_over_breakpoint = 1;
          keep_going (ecs);
          return;
 	}
@@ -1900,10 +1923,10 @@ handle_inferior_event (struct execution_control_state *ecs)
 	  /* The user issued a step when stopped at a breakpoint.
 	     Maybe we should stop, maybe we should not - the delay
 	     slot *might* correspond to a line of source.  In any
-	     case, don't decide that here, just set ecs->another_trap,
-	     making sure we single-step again before breakpoints are
-	     re-inserted.  */
-	  ecs->another_trap = 1;
+	     case, don't decide that here, just set 
+	     ecs->stepping_over_breakpoint, making sure we 
+	     single-step again before breakpoints are re-inserted.  */
+	  ecs->stepping_over_breakpoint = 1;
 	}
     }
 
@@ -1911,7 +1934,7 @@ handle_inferior_event (struct execution_control_state *ecs)
      The alternatives are:
      1) break; to really stop and return to the debugger,
      2) drop through to start up again
-     (set ecs->another_trap to 1 to single step once)
+     (set ecs->stepping_over_breakpoint to 1 to single step once)
      3) set ecs->random_signal to 1, and the decision between 1 and 2
      will be made according to the signal handling tables.  */
 
@@ -1967,7 +1990,7 @@ handle_inferior_event (struct execution_control_state *ecs)
 
       /* Don't even think about breakpoints if just proceeded over a
          breakpoint.  */
-      if (stop_signal == TARGET_SIGNAL_TRAP && trap_expected)
+      if (stop_signal == TARGET_SIGNAL_TRAP && stepping_over_breakpoint)
 	{
           if (debug_infrun)
 	    fprintf_unfiltered (gdb_stdlog, "infrun: trap expected\n");
@@ -2006,7 +2029,7 @@ handle_inferior_event (struct execution_control_state *ecs)
       if (stop_signal == TARGET_SIGNAL_TRAP)
 	ecs->random_signal
 	  = !(bpstat_explains_signal (stop_bpstat)
-	      || trap_expected
+	      || stepping_over_breakpoint
 	      || (step_range_end && step_resume_breakpoint == NULL));
       else
 	{
@@ -2167,7 +2190,7 @@ process_event_stop_test:
         if (debug_infrun)
 	  fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SINGLE\n");
 	remove_breakpoints ();
-	ecs->another_trap = 1;
+	ecs->stepping_over_breakpoint = 1;
 	/* Still need to check other stuff, at least the case
 	   where we are stepping and step out of the right range.  */
 	break;
@@ -2228,7 +2251,7 @@ process_event_stop_test:
 	       to doing that.  */
 	    ecs->step_after_step_resume_breakpoint = 0;
 	    remove_breakpoints ();
-	    ecs->another_trap = 1;
+	    ecs->stepping_over_breakpoint = 1;
 	    keep_going (ecs);
 	    return;
 	  }
@@ -2313,13 +2336,13 @@ process_event_stop_test:
 
 	      /* Be sure to lift all breakpoints, so the inferior does
 	         actually step past this point... */
-	      ecs->another_trap = 1;
+	      ecs->stepping_over_breakpoint = 1;
 	      break;
 	    }
 	  else
 	    {
 	      /* We want to step over this breakpoint, then keep going.  */
-	      ecs->another_trap = 1;
+	      ecs->stepping_over_breakpoint = 1;
 	      break;
 	    }
 	}
@@ -2350,7 +2373,7 @@ process_event_stop_test:
 	{
           if (debug_infrun)
 	    fprintf_unfiltered (gdb_stdlog, "infrun: stepping in dynamic linker\n");
-	  ecs->another_trap = 1;
+	  ecs->stepping_over_breakpoint = 1;
 	  keep_going (ecs);
 	  return;
 	}
@@ -2746,7 +2769,7 @@ currently_stepping (struct execution_control_state *ecs)
 {
   return ((!ecs->handling_longjmp
 	   && ((step_range_end && step_resume_breakpoint == NULL)
-	       || trap_expected))
+	       || stepping_over_breakpoint))
 	  || ecs->stepping_through_solib_after_catch
 	  || bpstat_should_step ());
 }
@@ -2925,7 +2948,7 @@ keep_going (struct execution_control_state *ecs)
   /* If we did not do break;, it means we should keep running the
      inferior and not return to debugger.  */
 
-  if (trap_expected && stop_signal != TARGET_SIGNAL_TRAP)
+  if (stepping_over_breakpoint && stop_signal != TARGET_SIGNAL_TRAP)
     {
       /* We took a signal (which we are supposed to pass through to
          the inferior, else we'd have done a break above) and we
@@ -2947,7 +2970,7 @@ keep_going (struct execution_control_state *ecs)
 	 already inserted breakpoints.  Therefore, we don't
 	 care if breakpoints were already inserted, or not.  */
       
-      if (!ecs->another_trap)
+      if (!ecs->stepping_over_breakpoint)
 	{
 	  /* Stop stepping when inserting breakpoints
 	     has failed.  */
@@ -2958,7 +2981,7 @@ keep_going (struct execution_control_state *ecs)
 	    }
 	}
 
-      trap_expected = ecs->another_trap;
+      stepping_over_breakpoint = ecs->stepping_over_breakpoint;
 
       /* Do not deliver SIGNAL_TRAP (except when the user explicitly
          specifies that such a signal should be delivered to the
@@ -3676,7 +3699,7 @@ struct inferior_status
   int stop_step;
   int stop_stack_dummy;
   int stopped_by_random_signal;
-  int trap_expected;
+  int stepping_over_breakpoint;
   CORE_ADDR step_range_start;
   CORE_ADDR step_range_end;
   struct frame_id step_frame_id;
@@ -3722,7 +3745,7 @@ save_inferior_status (int restore_stack_info)
   inf_status->stop_step = stop_step;
   inf_status->stop_stack_dummy = stop_stack_dummy;
   inf_status->stopped_by_random_signal = stopped_by_random_signal;
-  inf_status->trap_expected = trap_expected;
+  inf_status->stepping_over_breakpoint = stepping_over_breakpoint;
   inf_status->step_range_start = step_range_start;
   inf_status->step_range_end = step_range_end;
   inf_status->step_frame_id = step_frame_id;
@@ -3774,7 +3797,7 @@ restore_inferior_status (struct inferior_status *inf_status)
   stop_step = inf_status->stop_step;
   stop_stack_dummy = inf_status->stop_stack_dummy;
   stopped_by_random_signal = inf_status->stopped_by_random_signal;
-  trap_expected = inf_status->trap_expected;
+  stepping_over_breakpoint = inf_status->stepping_over_breakpoint;
   step_range_start = inf_status->step_range_start;
   step_range_end = inf_status->step_range_end;
   step_frame_id = inf_status->step_frame_id;
diff --git a/gdb/thread.c b/gdb/thread.c
index b6762e1..3c8644b 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -300,7 +300,7 @@ load_infrun_state (ptid_t ptid,
 		   CORE_ADDR *step_range_end,
 		   struct frame_id *step_frame_id,
 		   int *handling_longjmp,
-		   int *another_trap,
+		   int *stepping_over_breakpoint,
 		   int *stepping_through_solib_after_catch,
 		   bpstat *stepping_through_solib_catchpoints,
 		   int *current_line,
@@ -321,7 +321,7 @@ load_infrun_state (ptid_t ptid,
   *step_range_end = tp->step_range_end;
   *step_frame_id = tp->step_frame_id;
   *handling_longjmp = tp->handling_longjmp;
-  *another_trap = tp->another_trap;
+  *stepping_over_breakpoint = tp->stepping_over_breakpoint;
   *stepping_through_solib_after_catch =
     tp->stepping_through_solib_after_catch;
   *stepping_through_solib_catchpoints =
@@ -341,7 +341,7 @@ save_infrun_state (ptid_t ptid,
 		   CORE_ADDR step_range_end,
 		   const struct frame_id *step_frame_id,
 		   int handling_longjmp,
-		   int another_trap,
+		   int stepping_over_breakpoint,
 		   int stepping_through_solib_after_catch,
 		   bpstat stepping_through_solib_catchpoints,
 		   int current_line,
@@ -362,7 +362,7 @@ save_infrun_state (ptid_t ptid,
   tp->step_range_end = step_range_end;
   tp->step_frame_id = (*step_frame_id);
   tp->handling_longjmp = handling_longjmp;
-  tp->another_trap = another_trap;
+  tp->stepping_over_breakpoint = stepping_over_breakpoint;
   tp->stepping_through_solib_after_catch = stepping_through_solib_after_catch;
   tp->stepping_through_solib_catchpoints = stepping_through_solib_catchpoints;
   tp->current_line = current_line;
-- 
1.5.3.5

RE: [RFA] Clarify infrun variable naming.

[Pierre Muller]

> +   If we hit a breakpoint or watchpoint, and then continue,
> +   we need to single step the current thread with breakpoints
> +   disabled, so that to avoid hitting the same breakpoint or
> +   watchpoint again.  And we should step just a single
> +   thread and keep other threads stopped, so that
> +   other threads don't miss breakpoints while they are removed.
> +
> +   So, this variable simultaneously means that we need to single
> +   step current thread, keep other threads stopped, and that
> +   breakpoints should be removed while we step.
  But this is the reason of the failure to catch watchpoints
that happen at the point where we are just stepping over a breakpoint, 
because we step with the watchpoints disabled.
  Why don't we enable all break- and watchpoints but the
ones that do have the same PC we are currently?

  Enabling at least all watchpoints would fix gdb/38 failure as
seen in gdb.base/watchpoint.exp where it is noted as a KFAIL.

  I tried to check this by adding a insert_watchpoint function
a few days ago, but if you are working on it anyhow,
and probably master this much better than I do, it would be
great to solve that issue at the same time.


Pierre

RE: [RFA] Clarify infrun variable naming.

[Vladimir Prus]

Pierre Muller wrote:

> 
> 
>> +   If we hit a breakpoint or watchpoint, and then continue,
>> +   we need to single step the current thread with breakpoints
>> +   disabled, so that to avoid hitting the same breakpoint or
>> +   watchpoint again.  And we should step just a single
>> +   thread and keep other threads stopped, so that
>> +   other threads don't miss breakpoints while they are removed.
>> +
>> +   So, this variable simultaneously means that we need to single
>> +   step current thread, keep other threads stopped, and that
>> +   breakpoints should be removed while we step.
>   But this is the reason of the failure to catch watchpoints
> that happen at the point where we are just stepping over a breakpoint,
> because we step with the watchpoints disabled.
>   Why don't we enable all break- and watchpoints but the
> ones that do have the same PC we are currently?

Because that's extra work, and I haven't got around to that yet ;-)
In case of watchpoints, you probably meant enabling all watchpoint
at different data address, not PC?
 
>   Enabling at least all watchpoints would fix gdb/38 failure as
> seen in gdb.base/watchpoint.exp where it is noted as a KFAIL.
> 
>   I tried to check this by adding a insert_watchpoint function
> a few days ago, but if you are working on it anyhow,
> and probably master this much better than I do, it would be
> great to solve that issue at the same time.

I plan to address this soon (but as a separate patch).

- Volodya

RE: [RFA] Clarify infrun variable naming.

[Pierre Muller]

> >   But this is the reason of the failure to catch watchpoints
> > that happen at the point where we are just stepping over a
> breakpoint,
> > because we step with the watchpoints disabled.
> >   Why don't we enable all break- and watchpoints but the
> > ones that do have the same PC we are currently?
> 
> Because that's extra work, and I haven't got around to that yet ;-)
> In case of watchpoints, you probably meant enabling all watchpoint
> at different data address, not PC?
  Stepping over watchpoint is architechture dependent, 
for i386 this is not needed as the watchpoint is generated with PC 
at the instruction after the one that triggered the watchpoint... 
> 
> >   Enabling at least all watchpoints would fix gdb/38 failure as
> > seen in gdb.base/watchpoint.exp where it is noted as a KFAIL.
> >
> >   I tried to check this by adding a insert_watchpoint function
> > a few days ago, but if you are working on it anyhow,
> > and probably master this much better than I do, it would be
> > great to solve that issue at the same time.
> 
> I plan to address this soon (but as a separate patch).
Great news,
thanks Volodya!

Pierre

Re: [RFA] Clarify infrun variable naming.

[Vladimir Prus]

On Friday 23 November 2007 18:41:55 Pierre Muller wrote:
> > >   But this is the reason of the failure to catch watchpoints
> > > that happen at the point where we are just stepping over a
> > breakpoint,
> > > because we step with the watchpoints disabled.
> > >   Why don't we enable all break- and watchpoints but the
> > > ones that do have the same PC we are currently?
> > 
> > Because that's extra work, and I haven't got around to that yet ;-)
> > In case of watchpoints, you probably meant enabling all watchpoint
> > at different data address, not PC?
>   Stepping over watchpoint is architechture dependent, 
> for i386 this is not needed as the watchpoint is generated with PC 
> at the instruction after the one that triggered the watchpoint... 

Yes. What I mean is that there are two situations now:

1. When we step over breakpoint, we disable everything, including watchpoints.
2. When we hit watchpoint, and the PC is at the instruction itself, we disable
all breakpoints and watchpoints when stepping.

(2) might not be a problem now, but if we wish to interact with one thread,
while others are running, it might become a problem -- other threads
might miss unrelated breakpoints and watchpoints. So, we need to:

   - Remove breakpoints at current PC
   - Remove watchpoint a the address being accessed
   - Single step

I suspect you was more interested in (1), but that's basically two sides
of the coin.

- Volodya

Re: [RFA] Clarify infrun variable naming.

[Daniel Jacobowitz]

On Fri, Nov 23, 2007 at 06:51:47PM +0300, Vladimir Prus wrote:
>    - Remove breakpoints at current PC
>    - Remove watchpoint a the address being accessed
>    - Single step

Alternatively, remove all watchpoints but only for the current thread.

-- 
Daniel Jacobowitz
CodeSourcery

RE: [RFA] Clarify infrun variable naming.

[Pierre Muller]

> Yes. What I mean is that there are two situations now:
> 
> 1. When we step over breakpoint, we disable everything, including
> watchpoints.
> 2. When we hit watchpoint, and the PC is at the instruction itself, we
> disable
> all breakpoints and watchpoints when stepping.
> 
> (2) might not be a problem now, but if we wish to interact with one
> thread,
> while others are running, it might become a problem -- other threads
> might miss unrelated breakpoints and watchpoints. So, we need to:
> 
>    - Remove breakpoints at current PC
>    - Remove watchpoint a the address being accessed
>    - Single step
> 
> I suspect you was more interested in (1), but that's basically two
> sides
> of the coin.

  Yes, but this scheme should indeed also work to resolve the
watchpoint miss of gdb/38.
  About Daniel's answer, suggesting to use thread specific
watchpoints, I don't know if thread specific
watchpoints are available on all archs... Which would mean
that both schemes need to be implemented.

Pierre

Re: [RFA] Clarify infrun variable naming.

[Vladimir Prus]

On Friday 23 November 2007 19:05:57 Daniel Jacobowitz wrote:
> On Fri, Nov 23, 2007 at 06:51:47PM +0300, Vladimir Prus wrote:
> >    - Remove breakpoints at current PC
> >    - Remove watchpoint a the address being accessed
> >    - Single step
> 
> Alternatively, remove all watchpoints but only for the current thread.

How? target_remove_watchpoint does not appear to provide for that,
and in fact, I'm not sure what architectures provide for watchpoint that
fires in all threads except for thread N.

- Volodya

Re: [RFA] Clarify infrun variable naming.

[Daniel Jacobowitz]

On Fri, Nov 23, 2007 at 08:02:57PM +0300, Vladimir Prus wrote:
> How? target_remove_watchpoint does not appear to provide for that,

Correct, it doesn't (yet).

> and in fact, I'm not sure what architectures provide for watchpoint that
> fires in all threads except for thread N.

I don't know which do and which don't.  I know that GNU/Linux does;
every thread has its own watchpoint registers.  SMP embedded systems
generally do too, with a set of watchpoints on each processor.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Clarify infrun variable naming.

[Jim Blandy]

Vladimir Prus <vladimir at codesourcery.com> writes:
> The infrun.c file has a variable named trap_expected, which
> is a bit misleading -- after all, most times when we resume
> inferior, we get SIGTRAP. As it turns out, that variable
> is set when we're stepping over breakpoints, so a better
> name would be stepping_over_breakpoint. Likewise,
> ecs->another_trap also indicates that keep_going should
> be stepping over breakpoint. The attached patch clarifies
> the naming and adds comments, and has no behaviour changes.
> (The patch is on top of my previous breakpoints_inserted
> removing patch).
> OK?

Vlad and I had a conversation about this on the CodeSourcery IRC
channel, let me summarize here:

stepping_over_breakpoint would be a better name for trap_expected,
except that we already have a variable named stepping_past_breakpoint.
What is the relationship between trap_expected and
stepping_past_breakpoint?

As Vlad explains in the comments in his patch, we set trap_expected
when we do a single-thread, breakpoints-uninserted step to get a
thread past a breakpoint it has hit.

We set stepping_past_breakpoint when GDB reports a breakpoint hit, and
then the user switches to a different thread and steps that thread.
In this case, GDB needs to step the original thread across the
breakpoint it hit (a single-thread, breakpoints-uninserted step), and
then step of the thread the user selected.  In effect, we set aside
the user's command until we've dealt with the thread that hit the
breakpoint.

Note that stepping_past_breakpoint is only ever set when trap_expected
is set.  Thinking in terms of desired behavior, this makes sense
because there's no need to put off stepping the user's selected thread
if we can simply continue the thread the hit the breakpoint ---
trap_expected is our indication that we can't.  And looking at the
code, we see this is true because prepare_to_proceed only sets
stepping_past_breakpoint if it will return non-zero, and that proceed
always sets trap_expected if prepare_to_proceed returns non-zero.
Check.

I think stepping_past_breakpoint is not a good name for this variable:
it doesn't do anything to suggest the two-thread delayed-action
situation at hand.  It sounds like it'd be involved with an ordinary
step past a breakpoint.

I don't think there's any need for both stepping_past_breakpoint and
stepping_past_breakpoint_ptid.  We could have a single ptid_t
variable, equal to null_ptid when no special handling is necessary.

So, my suggestions were:

- We should replace stepping_past_breakpoint and
  stepping_past_breakpoint_ptid with a single ptid_t variable,
  deferred_step_ptid.

- We should rename trap_expected to stepped_over_breakpoint.  The past
  tense 'stepped' suggests that we're talking about a step which has
  already happened (which is true by the time we reach
  handle_inferior_event).

(And if I meditate carefully enough on the best possible names, it'll
be quite some time before I have to look at Vlad's harder patches.  :))

Re: [RFA] Clarify infrun variable naming.

[Daniel Jacobowitz]

On Tue, Dec 04, 2007 at 05:17:52PM -0800, Jim Blandy wrote:
> - We should rename trap_expected to stepped_over_breakpoint.  The past
>   tense 'stepped' suggests that we're talking about a step which has
>   already happened (which is true by the time we reach
>   handle_inferior_event).

Don't we set it before we've stepped over the breakpoint?  I find this
a little confusing...

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Clarify infrun variable naming.

[Daniel Jacobowitz]

On Tue, Dec 04, 2007 at 05:17:52PM -0800, Jim Blandy wrote:
> - We should rename trap_expected to stepped_over_breakpoint.  The past
>   tense 'stepped' suggests that we're talking about a step which has
>   already happened (which is true by the time we reach
>   handle_inferior_event).

Don't we set it before we've stepped over the breakpoint?  I find this
a little confusing...

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Clarify infrun variable naming.

[Vladimir Prus]

On Wednesday 05 December 2007 04:17:52 Jim Blandy wrote:

> - We should rename trap_expected to stepped_over_breakpoint.  The past
>   tense 'stepped' suggests that we're talking about a step which has
>   already happened (which is true by the time we reach
>   handle_inferior_event).

Just like Dan, I find 'stepped' confusing -- we set it before we
actually stepped. I think 'stepping' is fine -- it's set before
we're about to step, and is set until we're done with stepping.

Otherwise, your proposal looks good to me!

- Volodya

Re: [RFA] Clarify infrun variable naming.

[Vladimir Prus]

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

On Wednesday 05 December 2007 04:17:52 Jim Blandy wrote:
> 
> Vladimir Prus <vladimir at codesourcery.com> writes:
> > The infrun.c file has a variable named trap_expected, which
> > is a bit misleading -- after all, most times when we resume
> > inferior, we get SIGTRAP. As it turns out, that variable
> > is set when we're stepping over breakpoints, so a better
> > name would be stepping_over_breakpoint. Likewise,
> > ecs->another_trap also indicates that keep_going should
> > be stepping over breakpoint. The attached patch clarifies
> > the naming and adds comments, and has no behaviour changes.
> > (The patch is on top of my previous breakpoints_inserted
> > removing patch).
> > OK?
...
> So, my suggestions were:
> 
> - We should replace stepping_past_breakpoint and
>   stepping_past_breakpoint_ptid with a single ptid_t variable,
>   deferred_step_ptid.
> 
> - We should rename trap_expected to stepped_over_breakpoint.  The past
>   tense 'stepped' suggests that we're talking about a step which has
>   already happened (which is true by the time we reach
>   handle_inferior_event).
> 
> (And if I meditate carefully enough on the best possible names, it'll
> be quite some time before I have to look at Vlad's harder patches.  :))

Here's the patch that renamed stepping_past_breakpoint_ptid. I attach both
patch and the delta relative to the previous revision.

The 'stepped' vs. 'stepping' change was not done -- for reason I've posted
already.

OK?

- Volodya

       * infrun.c (trap_expected): Rename
        to stepping_over_breakpoint.  Document.
        (stepping_past_breakpoint): Remove.
        (stepping_past_breakpoint_ptdi): Renamed
        to deferred_step_ptid.
        (struct execution_control_state): Rename
        the another_trap field to stepping_over_breakpoint.
        (struct inferior_status): Rename the trap_expected
        field to stepping_over_breakpoint.
        (clear_proceed_status, proceed)
        (init_execution_control_state, context_switch)
        (handle_inferior_event, currently_stepping)
        (keep_going, save_inferior_status)
        (restore_inferior_status, prepare_to_proceed): Adjust.
        * gdbthread.h (struct thread_info): Rename the
        trap_expected field to stepping_over_breakpoint.
        * thread.c (load_infrun_state, save_infrun_state):
        Adjust.
---
 gdb/gdbthread.h |    2 +-
 gdb/infrun.c    |  117 ++++++++++++++++++++++++++++++++++---------------------
 gdb/thread.c    |    8 ++--
 3 files changed, 77 insertions(+), 50 deletions(-)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index aeb4a40..12e0bcc 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -49,7 +49,7 @@ struct thread_info
   struct symtab *current_symtab;
   int trap_expected;
   int handling_longjmp;
-  int another_trap;
+  int stepping_over_breakpoint;
 
   /* This is set TRUE when a catchpoint of a shared library event
      triggers.  Since we don't wish to leave the inferior in the
diff --git a/gdb/infrun.c b/gdb/infrun.c
index c608f72..77623b6 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -218,9 +218,30 @@ static struct cmd_list_element *stop_command;
 
 static struct symbol *step_start_function;
 
-/* Nonzero if we are expecting a trace trap and should proceed from it.  */
+/* Nonzero if we are presenting stepping over breakpoint.
 
-static int trap_expected;
+   If we hit a breakpoint or watchpoint, and then continue,
+   we need to single step the current thread with breakpoints
+   disabled, so that to avoid hitting the same breakpoint or
+   watchpoint again.  And we should step just a single
+   thread and keep other threads stopped, so that 
+   other threads don't miss breakpoints while they are removed.
+
+   So, this variable simultaneously means that we need to single
+   step current thread, keep other threads stopped, and that
+   breakpoints should be removed while we step.
+
+   This variable is set either:
+   - in proceed, when we resume inferior on explicit user's request
+   - in keep_going, if handle_inferior_event decides we need to
+   step over breakpoint.  
+
+   The variable is cleared in clear_proceed_status, called every
+   time before we call proceed.  The proceed calls wait_for_inferior,
+   which calls handle_inferior_event in a loop, and until 
+   wait_for_inferior exits, this variable is changed only by keep_going.  */
+
+static int stepping_over_breakpoint;
 
 /* Nonzero if we want to give control to the user when we're notified
    of shared library events by the dynamic linker.  */
@@ -442,10 +463,14 @@ static CORE_ADDR singlestep_pc;
 static ptid_t saved_singlestep_ptid;
 static int stepping_past_singlestep_breakpoint;
 
-/* Similarly, if we are stepping another thread past a breakpoint,
-   save the original thread here so that we can resume stepping it later.  */
-static ptid_t stepping_past_breakpoint_ptid;
-static int stepping_past_breakpoint;
+/* If not equal to null_ptid, means that after stepping over breakpoint
+   is finished, we need to switch to deferred_step_ptid, and step it.
+
+   The use case is when a breakpoint in one thread, and then the user 
+   has switched to another thread and issued 'step'. We need to step over
+   breakpoint in the thread which hit breakpoint, but then continue
+   stepping the thread user has selected.  */
+static ptid_t deferred_step_ptid;
  
 
 /* Things to clean up if we QUIT out of resume ().  */
@@ -647,7 +672,7 @@ a command like `return' or `jump' to continue execution."));
 void
 clear_proceed_status (void)
 {
-  trap_expected = 0;
+  stepping_over_breakpoint = 0;
   step_range_start = 0;
   step_range_end = 0;
   step_frame_id = null_frame_id;
@@ -693,8 +718,7 @@ prepare_to_proceed (int step)
       /* If stepping, remember current thread to switch back to.  */
       if (step)
        {
-         stepping_past_breakpoint = 1;
-         stepping_past_breakpoint_ptid = inferior_ptid;
+         deferred_step_ptid = inferior_ptid;
        }
 
       /* Switch back to WAIT_PID thread.  */
@@ -778,7 +802,7 @@ proceed (CORE_ADDR addr, enum target_signal siggnal, int step)
   if (oneproc)
     /* We will get a trace trap after one instruction.
        Continue it automatically and insert breakpoints then.  */
-    trap_expected = 1;
+    stepping_over_breakpoint = 1;
   else
     insert_breakpoints ();
 
@@ -842,7 +866,7 @@ start_remote (int from_tty)
   init_thread_list ();
   init_wait_for_inferior ();
   stop_soon = STOP_QUIETLY_REMOTE;
-  trap_expected = 0;
+  stepping_over_breakpoint = 0;
 
   /* Always go on waiting for the target, regardless of the mode. */
   /* FIXME: cagney/1999-09-23: At present it isn't possible to
@@ -887,7 +911,7 @@ init_wait_for_inferior (void)
   clear_proceed_status ();
 
   stepping_past_singlestep_breakpoint = 0;
-  stepping_past_breakpoint = 0;
+  deferred_step_ptid = null_ptid;
 }
  
 /* This enum encodes possible reasons for doing a target_wait, so that
@@ -924,7 +948,9 @@ struct execution_control_state
 {
   struct target_waitstatus ws;
   struct target_waitstatus *wp;
-  int another_trap;
+  /* Should we step over breakpoint next time keep_going 
+     is called?  */
+  int stepping_over_breakpoint;
   int random_signal;
   CORE_ADDR stop_func_start;
   CORE_ADDR stop_func_end;
@@ -1085,7 +1111,7 @@ fetch_inferior_event (void *client_data)
 void
 init_execution_control_state (struct execution_control_state *ecs)
 {
-  ecs->another_trap = 0;
+  ecs->stepping_over_breakpoint = 0;
   ecs->random_signal = 0;
   ecs->step_after_step_resume_breakpoint = 0;
   ecs->handling_longjmp = 0;   /* FIXME */
@@ -1140,20 +1166,20 @@ context_switch (struct execution_control_state *ecs)
     {                          /* Perform infrun state context switch: */
       /* Save infrun state for the old thread.  */
       save_infrun_state (inferior_ptid, prev_pc,
-                        trap_expected, step_resume_breakpoint,
+                        stepping_over_breakpoint, step_resume_breakpoint,
                         step_range_start,
                         step_range_end, &step_frame_id,
-                        ecs->handling_longjmp, ecs->another_trap,
+                        ecs->handling_longjmp, ecs->stepping_over_breakpoint,
                         ecs->stepping_through_solib_after_catch,
                         ecs->stepping_through_solib_catchpoints,
                         ecs->current_line, ecs->current_symtab);
 
       /* Load infrun state for the new thread.  */
       load_infrun_state (ecs->ptid, &prev_pc,
-                        &trap_expected, &step_resume_breakpoint,
+                        &stepping_over_breakpoint, &step_resume_breakpoint,
                         &step_range_start,
                         &step_range_end, &step_frame_id,
-                        &ecs->handling_longjmp, &ecs->another_trap,
+                        &ecs->handling_longjmp, &ecs->stepping_over_breakpoint,
                         &ecs->stepping_through_solib_after_catch,
                         &ecs->stepping_through_solib_catchpoints,
                         &ecs->current_line, &ecs->current_symtab);
@@ -1620,17 +1646,15 @@ handle_inferior_event (struct execution_control_state *ecs)
 
   stepping_past_singlestep_breakpoint = 0;
 
-  if (stepping_past_breakpoint)
+  if (!ptid_equal (deferred_step_ptid, null_ptid))
     {
-      stepping_past_breakpoint = 0;
-
       /* If we stopped for some other reason than single-stepping, ignore
         the fact that we were supposed to switch back.  */
       if (stop_signal == TARGET_SIGNAL_TRAP)
        {
          if (debug_infrun)
            fprintf_unfiltered (gdb_stdlog,
-                               "infrun: stepping_past_breakpoint\n");
+                               "infrun: handling deferred step\n");
 
          /* Pull the single step breakpoints out of the target.  */
          if (singlestep_breakpoints_inserted_p)
@@ -1641,7 +1665,8 @@ handle_inferior_event (struct execution_control_state *ecs)
 
          /* Note: We do not call context_switch at this point, as the
             context is already set up for stepping the original thread.  */
-         switch_to_thread (stepping_past_breakpoint_ptid);
+         switch_to_thread (deferred_step_ptid);
+         deferred_step_ptid = null_ptid;
          /* Suppress spurious "Switching to ..." message.  */
          previous_inferior_ptid = inferior_ptid;
 
@@ -1649,6 +1674,8 @@ handle_inferior_event (struct execution_control_state *ecs)
          prepare_to_wait (ecs);
          return;
        }
+
+      deferred_step_ptid = null_ptid;
     }
 
   /* See if a thread hit a thread-specific breakpoint that was meant for
@@ -1779,7 +1806,7 @@ handle_inferior_event (struct execution_control_state *ecs)
                context_switch (ecs);
              ecs->waiton_ptid = ecs->ptid;
              ecs->wp = &(ecs->ws);
-             ecs->another_trap = 1;
+             ecs->stepping_over_breakpoint = 1;
 
              ecs->infwait_state = infwait_thread_hop_state;
              keep_going (ecs);
@@ -1870,7 +1897,7 @@ handle_inferior_event (struct execution_control_state *ecs)
                            &ecs->stop_func_start, &ecs->stop_func_end);
   ecs->stop_func_start
     += gdbarch_deprecated_function_start_offset (current_gdbarch);
-  ecs->another_trap = 0;
+  ecs->stepping_over_breakpoint = 0;
   bpstat_clear (&stop_bpstat);
   stop_step = 0;
   stop_stack_dummy = 0;
@@ -1879,7 +1906,7 @@ handle_inferior_event (struct execution_control_state *ecs)
   stopped_by_random_signal = 0;
 
   if (stop_signal == TARGET_SIGNAL_TRAP
-      && trap_expected
+      && stepping_over_breakpoint
       && gdbarch_single_step_through_delay_p (current_gdbarch)
       && currently_stepping (ecs))
     {
@@ -1897,7 +1924,7 @@ handle_inferior_event (struct execution_control_state *ecs)
        {
          /* The user issued a continue when stopped at a breakpoint.
             Set up for another trap and get out of here.  */
-         ecs->another_trap = 1;
+         ecs->stepping_over_breakpoint = 1;
          keep_going (ecs);
          return;
        }
@@ -1906,10 +1933,10 @@ handle_inferior_event (struct execution_control_state *ecs)
          /* The user issued a step when stopped at a breakpoint.
             Maybe we should stop, maybe we should not - the delay
             slot *might* correspond to a line of source.  In any
-            case, don't decide that here, just set ecs->another_trap,
-            making sure we single-step again before breakpoints are
-            re-inserted.  */
-         ecs->another_trap = 1;
+            case, don't decide that here, just set 
+            ecs->stepping_over_breakpoint, making sure we 
+            single-step again before breakpoints are re-inserted.  */
+         ecs->stepping_over_breakpoint = 1;
        }
     }
 
@@ -1917,7 +1944,7 @@ handle_inferior_event (struct execution_control_state *ecs)
      The alternatives are:
      1) break; to really stop and return to the debugger,
      2) drop through to start up again
-     (set ecs->another_trap to 1 to single step once)
+     (set ecs->stepping_over_breakpoint to 1 to single step once)
      3) set ecs->random_signal to 1, and the decision between 1 and 2
      will be made according to the signal handling tables.  */
 
@@ -2001,7 +2028,7 @@ handle_inferior_event (struct execution_control_state *ecs)
       if (stop_signal == TARGET_SIGNAL_TRAP)
        ecs->random_signal
          = !(bpstat_explains_signal (stop_bpstat)
-             || trap_expected
+             || stepping_over_breakpoint
              || (step_range_end && step_resume_breakpoint == NULL));
       else
        {
@@ -2162,7 +2189,7 @@ process_event_stop_test:
         if (debug_infrun)
          fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_SINGLE\n");
        remove_breakpoints ();
-       ecs->another_trap = 1;
+       ecs->stepping_over_breakpoint = 1;
        /* Still need to check other stuff, at least the case
           where we are stepping and step out of the right range.  */
        break;
@@ -2223,7 +2250,7 @@ process_event_stop_test:
               to doing that.  */
            ecs->step_after_step_resume_breakpoint = 0;
            remove_breakpoints ();
-           ecs->another_trap = 1;
+           ecs->stepping_over_breakpoint = 1;
            keep_going (ecs);
            return;
          }
@@ -2308,13 +2335,13 @@ process_event_stop_test:
 
              /* Be sure to lift all breakpoints, so the inferior does
                 actually step past this point... */
-             ecs->another_trap = 1;
+             ecs->stepping_over_breakpoint = 1;
              break;
            }
          else
            {
              /* We want to step over this breakpoint, then keep going.  */
-             ecs->another_trap = 1;
+             ecs->stepping_over_breakpoint = 1;
              break;
            }
        }
@@ -2345,7 +2372,7 @@ process_event_stop_test:
        {
           if (debug_infrun)
            fprintf_unfiltered (gdb_stdlog, "infrun: stepping in dynamic linker\n");
-         ecs->another_trap = 1;
+         ecs->stepping_over_breakpoint = 1;
          keep_going (ecs);
          return;
        }
@@ -2741,7 +2768,7 @@ currently_stepping (struct execution_control_state *ecs)
 {
   return ((!ecs->handling_longjmp
           && ((step_range_end && step_resume_breakpoint == NULL)
-              || trap_expected))
+              || stepping_over_breakpoint))
          || ecs->stepping_through_solib_after_catch
          || bpstat_should_step ());
 }
@@ -2920,7 +2947,7 @@ keep_going (struct execution_control_state *ecs)
   /* If we did not do break;, it means we should keep running the
      inferior and not return to debugger.  */
 
-  if (trap_expected && stop_signal != TARGET_SIGNAL_TRAP)
+  if (stepping_over_breakpoint && stop_signal != TARGET_SIGNAL_TRAP)
     {
       /* We took a signal (which we are supposed to pass through to
          the inferior, else we'd have done a break above) and we
@@ -2942,7 +2969,7 @@ keep_going (struct execution_control_state *ecs)
         already inserted breakpoints.  Therefore, we don't
         care if breakpoints were already inserted, or not.  */
       
-      if (!ecs->another_trap)
+      if (!ecs->stepping_over_breakpoint)
        {
          struct gdb_exception e;
          /* Stop stepping when inserting breakpoints
@@ -2958,7 +2985,7 @@ keep_going (struct execution_control_state *ecs)
            }
        }
 
-      trap_expected = ecs->another_trap;
+      stepping_over_breakpoint = ecs->stepping_over_breakpoint;
 
       /* Do not deliver SIGNAL_TRAP (except when the user explicitly
          specifies that such a signal should be delivered to the
@@ -3676,7 +3703,7 @@ struct inferior_status
   int stop_step;
   int stop_stack_dummy;
   int stopped_by_random_signal;
-  int trap_expected;
+  int stepping_over_breakpoint;
   CORE_ADDR step_range_start;
   CORE_ADDR step_range_end;
   struct frame_id step_frame_id;
@@ -3722,7 +3749,7 @@ save_inferior_status (int restore_stack_info)
   inf_status->stop_step = stop_step;
   inf_status->stop_stack_dummy = stop_stack_dummy;
   inf_status->stopped_by_random_signal = stopped_by_random_signal;
-  inf_status->trap_expected = trap_expected;
+  inf_status->stepping_over_breakpoint = stepping_over_breakpoint;
   inf_status->step_range_start = step_range_start;
   inf_status->step_range_end = step_range_end;
   inf_status->step_frame_id = step_frame_id;
@@ -3774,7 +3801,7 @@ restore_inferior_status (struct inferior_status *inf_status)
   stop_step = inf_status->stop_step;
   stop_stack_dummy = inf_status->stop_stack_dummy;
   stopped_by_random_signal = inf_status->stopped_by_random_signal;
-  trap_expected = inf_status->trap_expected;
+  stepping_over_breakpoint = inf_status->stepping_over_breakpoint;
   step_range_start = inf_status->step_range_start;
   step_range_end = inf_status->step_range_end;
   step_frame_id = inf_status->step_frame_id;
diff --git a/gdb/thread.c b/gdb/thread.c
index b6762e1..3c8644b 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -300,7 +300,7 @@ load_infrun_state (ptid_t ptid,
                   CORE_ADDR *step_range_end,
                   struct frame_id *step_frame_id,
                   int *handling_longjmp,
-                  int *another_trap,
+                  int *stepping_over_breakpoint,
                   int *stepping_through_solib_after_catch,
                   bpstat *stepping_through_solib_catchpoints,
                   int *current_line,
@@ -321,7 +321,7 @@ load_infrun_state (ptid_t ptid,
   *step_range_end = tp->step_range_end;
   *step_frame_id = tp->step_frame_id;
   *handling_longjmp = tp->handling_longjmp;
-  *another_trap = tp->another_trap;
+  *stepping_over_breakpoint = tp->stepping_over_breakpoint;
   *stepping_through_solib_after_catch =
     tp->stepping_through_solib_after_catch;
   *stepping_through_solib_catchpoints =
@@ -341,7 +341,7 @@ save_infrun_state (ptid_t ptid,
                   CORE_ADDR step_range_end,
                   const struct frame_id *step_frame_id,
                   int handling_longjmp,
-                  int another_trap,
+                  int stepping_over_breakpoint,
                   int stepping_through_solib_after_catch,
                   bpstat stepping_through_solib_catchpoints,
                   int current_line,
@@ -362,7 +362,7 @@ save_infrun_state (ptid_t ptid,
   tp->step_range_end = step_range_end;
   tp->step_frame_id = (*step_frame_id);
   tp->handling_longjmp = handling_longjmp;
-  tp->another_trap = another_trap;
+  tp->stepping_over_breakpoint = stepping_over_breakpoint;
   tp->stepping_through_solib_after_catch = stepping_through_solib_after_catch;
   tp->stepping_through_solib_catchpoints = stepping_through_solib_catchpoints;
   tp->current_line = current_line;
-- 
1.5.3.5


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

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 043d8f1..77623b6 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -463,10 +463,14 @@ static CORE_ADDR singlestep_pc;
 static ptid_t saved_singlestep_ptid;
 static int stepping_past_singlestep_breakpoint;
 
-/* Similarly, if we are stepping another thread past a breakpoint,
-   save the original thread here so that we can resume stepping it later.  */
-static ptid_t stepping_past_breakpoint_ptid;
-static int stepping_past_breakpoint;
+/* If not equal to null_ptid, means that after stepping over breakpoint
+   is finished, we need to switch to deferred_step_ptid, and step it.
+
+   The use case is when a breakpoint in one thread, and then the user 
+   has switched to another thread and issued 'step'. We need to step over
+   breakpoint in the thread which hit breakpoint, but then continue
+   stepping the thread user has selected.  */
+static ptid_t deferred_step_ptid;
 \f
 
 /* Things to clean up if we QUIT out of resume ().  */
@@ -714,8 +718,7 @@ prepare_to_proceed (int step)
       /* If stepping, remember current thread to switch back to.  */
       if (step)
 	{
-	  stepping_past_breakpoint = 1;
-	  stepping_past_breakpoint_ptid = inferior_ptid;
+	  deferred_step_ptid = inferior_ptid;
 	}
 
       /* Switch back to WAIT_PID thread.  */
@@ -908,7 +911,7 @@ init_wait_for_inferior (void)
   clear_proceed_status ();
 
   stepping_past_singlestep_breakpoint = 0;
-  stepping_past_breakpoint = 0;
+  deferred_step_ptid = null_ptid;
 }
 \f
 /* This enum encodes possible reasons for doing a target_wait, so that
@@ -1643,17 +1646,15 @@ handle_inferior_event (struct execution_control_state *ecs)
 
   stepping_past_singlestep_breakpoint = 0;
 
-  if (stepping_past_breakpoint)
+  if (!ptid_equal (deferred_step_ptid, null_ptid))
     {
-      stepping_past_breakpoint = 0;
-
       /* If we stopped for some other reason than single-stepping, ignore
 	 the fact that we were supposed to switch back.  */
       if (stop_signal == TARGET_SIGNAL_TRAP)
 	{
 	  if (debug_infrun)
 	    fprintf_unfiltered (gdb_stdlog,
-				"infrun: stepping_past_breakpoint\n");
+				"infrun: handling deferred step\n");
 
 	  /* Pull the single step breakpoints out of the target.  */
 	  if (singlestep_breakpoints_inserted_p)
@@ -1664,7 +1665,8 @@ handle_inferior_event (struct execution_control_state *ecs)
 
 	  /* Note: We do not call context_switch at this point, as the
 	     context is already set up for stepping the original thread.  */
-	  switch_to_thread (stepping_past_breakpoint_ptid);
+	  switch_to_thread (deferred_step_ptid);
+	  deferred_step_ptid = null_ptid;
 	  /* Suppress spurious "Switching to ..." message.  */
 	  previous_inferior_ptid = inferior_ptid;
 
@@ -1672,6 +1674,8 @@ handle_inferior_event (struct execution_control_state *ecs)
 	  prepare_to_wait (ecs);
 	  return;
 	}
+
+      deferred_step_ptid = null_ptid;
     }
 
   /* See if a thread hit a thread-specific breakpoint that was meant for

Re: [RFA] Clarify infrun variable naming.

[Jim Blandy]

Vladimir Prus <vladimir at codesourcery.com> writes:
> On Wednesday 05 December 2007 04:17:52 Jim Blandy wrote:
>> 
>> Vladimir Prus <vladimir at codesourcery.com> writes:
>> > The infrun.c file has a variable named trap_expected, which
>> > is a bit misleading -- after all, most times when we resume
>> > inferior, we get SIGTRAP. As it turns out, that variable
>> > is set when we're stepping over breakpoints, so a better
>> > name would be stepping_over_breakpoint. Likewise,
>> > ecs->another_trap also indicates that keep_going should
>> > be stepping over breakpoint. The attached patch clarifies
>> > the naming and adds comments, and has no behaviour changes.
>> > (The patch is on top of my previous breakpoints_inserted
>> > removing patch).
>> > OK?
> ...
>> So, my suggestions were:
>> 
>> - We should replace stepping_past_breakpoint and
>>   stepping_past_breakpoint_ptid with a single ptid_t variable,
>>   deferred_step_ptid.
>> 
>> - We should rename trap_expected to stepped_over_breakpoint.  The past
>>   tense 'stepped' suggests that we're talking about a step which has
>>   already happened (which is true by the time we reach
>>   handle_inferior_event).
>> 
>> (And if I meditate carefully enough on the best possible names, it'll
>> be quite some time before I have to look at Vlad's harder patches.  :))
>
> Here's the patch that renamed stepping_past_breakpoint_ptid. I attach both
> patch and the delta relative to the previous revision.
>
> The 'stepped' vs. 'stepping' change was not done -- for reason I've posted
> already.

Sure, that's fine.  I agree with your and Dan's arguments.

I noticed some text issues; with those fixed, this can go in.

> +/* Nonzero if we are presenting stepping over breakpoint.

This should be "over a breakpoint".

> -static int trap_expected;
> +   If we hit a breakpoint or watchpoint, and then continue,
> +   we need to single step the current thread with breakpoints
> +   disabled, so that to avoid hitting the same breakpoint or

"so that" needs to be deleted, or perhaps changed to "so as"?

> +   watchpoint again.  And we should step just a single
> +   thread and keep other threads stopped, so that 
> +   other threads don't miss breakpoints while they are removed.
> +
> +   So, this variable simultaneously means that we need to single
> +   step current thread, keep other threads stopped, and that
> +   breakpoints should be removed while we step.

"step the current thread".

> +   This variable is set either:
> +   - in proceed, when we resume inferior on explicit user's request

"resume the inferior at the user's explicit request"

> +/* If not equal to null_ptid, means that after stepping over breakpoint

'this means'

> +   is finished, we need to switch to deferred_step_ptid, and step it.
> +
> +   The use case is when a breakpoint in one thread, and then the user 

'when one thread has hit a breakpoint'?

> +   has switched to another thread and issued 'step'. We need to step over
> +   breakpoint in the thread which hit breakpoint, but then continue

'the breakpoint'

> +   stepping the thread user has selected.  */
> +static ptid_t deferred_step_ptid;