Rewrite non-continuable watchpoints handling

When GDB finds out the target triggered a watchpoint, and the target
has non-continuable watchpoints, GDB sets things up to step past the
instruction that triggered the watchpoint.  This is just like stepping
past a breakpoint, but goes through a different mechanism - it resumes
only the thread that needs to step past the watchpoint, but also
switches a "infwait state" global, that has the effect that the next
target_wait only wait for events only from that thread.

This forcing of a ptid to pass to target_wait obviously becomes a
bottleneck if we ever support stepping past different watchpoints
simultaneously (in separate processes).

It's also unnecessary -- the target should only return events for
threads that have been resumed; if no other thread than the one we're
stepping past the watchpoint has been resumed, then those other
threads should not report events.  If we couldn't assume that, then
stepping past regular breakpoints would be broken for not likewise
forcing a similar infwait_state.

So this patch eliminates infwait_state, and instead teaches keep_going
to mark step_over_info in a way that has the breakpoints module skip
inserting watchpoints (because we're stepping past one), like it skips
breakpoints when we're stepping past one.

Tested on:

 - x86_64 Fedora 20 (continuable watchpoints)
 - PPC64 Fedora 18  (non-steppable watchpoints)

gdb/
2014-10-15  Pedro Alves  <palves@redhat.com>

	* breakpoint.c (should_be_inserted): Don't insert watchpoints if
	trying to step past a non-steppable watchpoint.
	* gdbthread.h (struct thread_info) <stepping_over_watchpoint>: New
	field.
	* infrun.c (struct step_over_info): Add new field
	'nonsteppable_watchpoint_p' and adjust comments.
	(set_step_over_info): New 'nonsteppable_watchpoint_p' parameter.
	Adjust.
	(clear_step_over_info): Clear nonsteppable_watchpoint_p as well.
	(stepping_past_nonsteppable_watchpoint): New function.
	(step_over_info_valid_p): Also return true if stepping past a
	nonsteppable watchpoint.
	(proceed): Adjust call to set_step_over_info.  Remove reference to
	init_infwait_state.
	(init_wait_for_inferior): Remove reference to init_infwait_state.
	(waiton_ptid): Delete global.
	(struct execution_control_state)
	<stepped_after_stopped_by_watchpoint>: Delete field.
	(wait_for_inferior, fetch_inferior_event): Always pass
	minus_one_ptid to target_wait.
	(init_thread_stepping_state): Clear 'stepping_over_watchpoint'
	field.
	(init_infwait_state): Delete function.
	(handle_inferior_event): Remove infwait_state handling.
	(handle_signal_stop) <watchpoints handling>: Adjust after
	stepped_after_stopped_by_watchpoint removal.  Don't remove
	breakpoints here nor set infwait_state.  Set the thread's
	stepping_over_watchpoint flag, and call keep_going instead.
	(keep_going): Handle stepping_over_watchpoint.  Adjust
	set_step_over_info calls.
	* infrun.h (stepping_past_nonsteppable_watchpoint): Declare
	function.

5 files changed, 114 insertions, 90 deletions

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5de5681..0b03948 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,40 @@
 2014-10-15  Pedro Alves  <palves@redhat.com>
 
+	* breakpoint.c (should_be_inserted): Don't insert watchpoints if
+	trying to step past a non-steppable watchpoint.
+	* gdbthread.h (struct thread_info) <stepping_over_watchpoint>: New
+	field.
+	* infrun.c (struct step_over_info): Add new field
+	'nonsteppable_watchpoint_p' and adjust comments.
+	(set_step_over_info): New 'nonsteppable_watchpoint_p' parameter.
+	Adjust.
+	(clear_step_over_info): Clear nonsteppable_watchpoint_p as well.
+	(stepping_past_nonsteppable_watchpoint): New function.
+	(step_over_info_valid_p): Also return true if stepping past a
+	nonsteppable watchpoint.
+	(proceed): Adjust call to set_step_over_info.  Remove reference to
+	init_infwait_state.
+	(init_wait_for_inferior): Remove reference to init_infwait_state.
+	(waiton_ptid): Delete global.
+	(struct execution_control_state)
+	<stepped_after_stopped_by_watchpoint>: Delete field.
+	(wait_for_inferior, fetch_inferior_event): Always pass
+	minus_one_ptid to target_wait.
+	(init_thread_stepping_state): Clear 'stepping_over_watchpoint'
+	field.
+	(init_infwait_state): Delete function.
+	(handle_inferior_event): Remove infwait_state handling.
+	(handle_signal_stop) <watchpoints handling>: Adjust after
+	stepped_after_stopped_by_watchpoint removal.  Don't remove
+	breakpoints here nor set infwait_state.  Set the thread's
+	stepping_over_watchpoint flag, and call keep_going instead.
+	(keep_going): Handle stepping_over_watchpoint.  Adjust
+	set_step_over_info calls.
+	* infrun.h (stepping_past_nonsteppable_watchpoint): Declare
+	function.
+
+2014-10-15  Pedro Alves  <palves@redhat.com>
+
 	* infrun.c (step_over_info_valid_p): New function.
 	(resume): Use step_over_info_valid_p instead of checking the
 	threads's trap_expected flag.
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index dc2e297..2a6e51d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2208,6 +2208,22 @@ should_be_inserted (struct bp_location *bl)
       return 0;
     }
 
+  /* Don't insert watchpoints if we're trying to step past the
+     instruction that triggered one.  */
+  if ((bl->loc_type == bp_loc_hardware_watchpoint)
+      && stepping_past_nonsteppable_watchpoint ())
+    {
+      if (debug_infrun)
+	{
+	  fprintf_unfiltered (gdb_stdlog,
+			      "infrun: stepping past non-steppable watchpoint. "
+			      "skipping watchpoint at %s:%d\n",
+			      paddress (bl->gdbarch, bl->address),
+			      bl->length);
+	}
+      return 0;
+    }
+
   return 1;
 }
 
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 26ca925..36ad5a7 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -197,6 +197,11 @@ struct thread_info
   /* Should we step over breakpoint next time keep_going is called?  */
   int stepping_over_breakpoint;
 
+  /* Should we step over a watchpoint next time keep_going is called?
+     This is needed on targets with non-continuable, non-steppable
+     watchpoints.  */
+  int stepping_over_watchpoint;
+
   /* Set to TRUE if we should finish single-stepping over a breakpoint
      after hitting the current step-resume breakpoint.  The context here
      is that GDB is to do `next' or `step' while signal arrives.
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 650d711..d0431df 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -377,8 +377,6 @@ static void context_switch (ptid_t ptid);
 
 void init_thread_stepping_state (struct thread_info *tss);
 
-static void init_infwait_state (void);
-
 static const char follow_fork_mode_child[] = "child";
 static const char follow_fork_mode_parent[] = "parent";
 
@@ -1208,16 +1206,20 @@ static ptid_t singlestep_ptid;
 /* PC when we started this single-step.  */
 static CORE_ADDR singlestep_pc;
 
-/* Info about an instruction that is being stepped over.  Invalid if
-   ASPACE is NULL.  */
+/* Info about an instruction that is being stepped over.  */
 
 struct step_over_info
 {
-  /* The instruction's address space.  */
+  /* If we're stepping past a breakpoint, this is the address space
+     and address of the instruction the breakpoint is set at.  We'll
+     skip inserting all breakpoints here.  Valid iff ASPACE is
+     non-NULL.  */
   struct address_space *aspace;
-
-  /* The instruction's address.  */
   CORE_ADDR address;
+
+  /* The instruction being stepped over triggers a nonsteppable
+     watchpoint.  If true, we'll skip inserting watchpoints.  */
+  int nonsteppable_watchpoint_p;
 };
 
 /* The step-over info of the location that is being stepped over.
@@ -1250,10 +1252,12 @@ static struct step_over_info step_over_info;
    stepping over.  */
 
 static void
-set_step_over_info (struct address_space *aspace, CORE_ADDR address)
+set_step_over_info (struct address_space *aspace, CORE_ADDR address,
+		    int nonsteppable_watchpoint_p)
 {
   step_over_info.aspace = aspace;
   step_over_info.address = address;
+  step_over_info.nonsteppable_watchpoint_p = nonsteppable_watchpoint_p;
 }
 
 /* Called when we're not longer stepping over a breakpoint / an
@@ -1264,6 +1268,7 @@ clear_step_over_info (void)
 {
   step_over_info.aspace = NULL;
   step_over_info.address = 0;
+  step_over_info.nonsteppable_watchpoint_p = 0;
 }
 
 /* See infrun.h.  */
@@ -1278,12 +1283,21 @@ stepping_past_instruction_at (struct address_space *aspace,
 				       step_over_info.address));
 }
 
+/* See infrun.h.  */
+
+int
+stepping_past_nonsteppable_watchpoint (void)
+{
+  return step_over_info.nonsteppable_watchpoint_p;
+}
+
 /* Returns true if step-over info is valid.  */
 
 static int
 step_over_info_valid_p (void)
 {
-  return (step_over_info.aspace != NULL);
+  return (step_over_info.aspace != NULL
+	  || stepping_past_nonsteppable_watchpoint ());
 }
 
 
@@ -2555,7 +2569,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
       struct regcache *regcache = get_current_regcache ();
 
       set_step_over_info (get_regcache_aspace (regcache),
-			  regcache_read_pc (regcache));
+			  regcache_read_pc (regcache), 0);
     }
   else
     clear_step_over_info ();
@@ -2595,9 +2609,6 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
      correctly when the inferior is stopped.  */
   tp->prev_pc = regcache_read_pc (get_current_regcache ());
 
-  /* Reset to normal state.  */
-  init_infwait_state ();
-
   /* Resume inferior.  */
   resume (tp->control.trap_expected || step || bpstat_should_step (),
 	  tp->suspend.stop_signal);
@@ -2662,7 +2673,6 @@ init_wait_for_inferior (void)
   target_last_wait_ptid = minus_one_ptid;
 
   previous_inferior_ptid = inferior_ptid;
-  init_infwait_state ();
 
   /* Discard any skipped inlined frames.  */
   clear_inline_frame_state (minus_one_ptid);
@@ -2683,9 +2693,6 @@ enum infwait_states
   infwait_nonstep_watch_state
 };
 
-/* The PTID we'll do a target_wait on.*/
-ptid_t waiton_ptid;
-
 /* Current inferior wait state.  */
 static enum infwait_states infwait_state;
 
@@ -2705,11 +2712,6 @@ struct execution_control_state
   const char *stop_func_name;
   int wait_some_more;
 
-  /* We were in infwait_step_watch_state or
-     infwait_nonstep_watch_state state, and the thread reported an
-     event.  */
-  int stepped_after_stopped_by_watchpoint;
-
   /* True if the event thread hit the single-step breakpoint of
      another thread.  Thus the event doesn't cause a stop, the thread
      needs to be single-stepped past the single-step breakpoint before
@@ -3034,6 +3036,7 @@ wait_for_inferior (void)
       struct execution_control_state ecss;
       struct execution_control_state *ecs = &ecss;
       struct cleanup *old_chain;
+      ptid_t waiton_ptid = minus_one_ptid;
 
       memset (ecs, 0, sizeof (*ecs));
 
@@ -3089,6 +3092,7 @@ fetch_inferior_event (void *client_data)
   struct cleanup *ts_old_chain;
   int was_sync = sync_execution;
   int cmd_done = 0;
+  ptid_t waiton_ptid = minus_one_ptid;
 
   memset (ecs, 0, sizeof (*ecs));
 
@@ -3206,6 +3210,7 @@ void
 init_thread_stepping_state (struct thread_info *tss)
 {
   tss->stepping_over_breakpoint = 0;
+  tss->stepping_over_watchpoint = 0;
   tss->step_after_step_resume_breakpoint = 0;
 }
 
@@ -3375,13 +3380,6 @@ adjust_pc_after_break (struct execution_control_state *ecs)
     }
 }
 
-static void
-init_infwait_state (void)
-{
-  waiton_ptid = pid_to_ptid (-1);
-  infwait_state = infwait_normal_state;
-}
-
 static int
 stepped_in_from (struct frame_info *frame, struct frame_id step_frame_id)
 {
@@ -3602,40 +3600,6 @@ handle_inferior_event (struct execution_control_state *ecs)
 	   && ecs->ws.kind != TARGET_WAITKIND_EXITED)
     set_executing (ecs->ptid, 0);
 
-  switch (infwait_state)
-    {
-    case infwait_normal_state:
-      if (debug_infrun)
-        fprintf_unfiltered (gdb_stdlog, "infrun: infwait_normal_state\n");
-      break;
-
-    case infwait_step_watch_state:
-      if (debug_infrun)
-        fprintf_unfiltered (gdb_stdlog,
-			    "infrun: infwait_step_watch_state\n");
-
-      ecs->stepped_after_stopped_by_watchpoint = 1;
-      break;
-
-    case infwait_nonstep_watch_state:
-      if (debug_infrun)
-        fprintf_unfiltered (gdb_stdlog,
-			    "infrun: infwait_nonstep_watch_state\n");
-      insert_breakpoints ();
-
-      /* FIXME-maybe: is this cleaner than setting a flag?  Does it
-         handle things like signals arriving and other things happening
-         in combination correctly?  */
-      ecs->stepped_after_stopped_by_watchpoint = 1;
-      break;
-
-    default:
-      internal_error (__FILE__, __LINE__, _("bad switch"));
-    }
-
-  infwait_state = infwait_normal_state;
-  waiton_ptid = pid_to_ptid (-1);
-
   switch (ecs->ws.kind)
     {
     case TARGET_WAITKIND_LOADED:
@@ -4225,7 +4189,9 @@ handle_signal_stop (struct execution_control_state *ecs)
       singlestep_breakpoints_inserted_p = 0;
     }
 
-  if (ecs->stepped_after_stopped_by_watchpoint)
+  if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP
+      && ecs->event_thread->control.trap_expected
+      && ecs->event_thread->stepping_over_watchpoint)
     stopped_by_watchpoint = 0;
   else
     stopped_by_watchpoint = watchpoints_triggered (&ecs->ws);
@@ -4255,29 +4221,21 @@ handle_signal_stop (struct execution_control_state *ecs)
 	 It is far more common to need to disable a watchpoint to step
 	 the inferior over it.  If we have non-steppable watchpoints,
 	 we must disable the current watchpoint; it's simplest to
-	 disable all watchpoints and breakpoints.  */
-      int hw_step = 1;
-
-      if (!target_have_steppable_watchpoint)
-	{
-	  remove_breakpoints ();
-	  /* See comment in resume why we need to stop bypassing signals
-	     while breakpoints have been removed.  */
-	  target_pass_signals (0, NULL);
-	}
-	/* Single step */
-      hw_step = maybe_software_singlestep (gdbarch, stop_pc);
-      target_resume (ecs->ptid, hw_step, GDB_SIGNAL_0);
-      waiton_ptid = ecs->ptid;
-      if (target_have_steppable_watchpoint)
-	infwait_state = infwait_step_watch_state;
-      else
-	infwait_state = infwait_nonstep_watch_state;
-      prepare_to_wait (ecs);
+	 disable all watchpoints.
+
+	 Any breakpoint at PC must also be stepped over -- if there's
+	 one, it will have already triggered before the watchpoint
+	 triggered, and we either already reported it to the user, or
+	 it didn't cause a stop and we called keep_going.  In either
+	 case, if there was a breakpoint at PC, we must be trying to
+	 step past it.  */
+      ecs->event_thread->stepping_over_watchpoint = 1;
+      keep_going (ecs);
       return;
     }
 
   ecs->event_thread->stepping_over_breakpoint = 0;
+  ecs->event_thread->stepping_over_watchpoint = 0;
   bpstat_clear (&ecs->event_thread->control.stop_bpstat);
   ecs->event_thread->control.stop_step = 0;
   stop_print_frame = 1;
@@ -6015,6 +5973,8 @@ keep_going (struct execution_control_state *ecs)
     {
       volatile struct gdb_exception e;
       struct regcache *regcache = get_current_regcache ();
+      int remove_bp;
+      int remove_wps;
 
       /* Either the trap was not expected, but we are continuing
 	 anyway (if we got a signal, the user asked it be passed to
@@ -6034,13 +5994,19 @@ keep_going (struct execution_control_state *ecs)
 	 (watchpoints, etc.) but the one we're stepping over, step one
 	 instruction, and then re-insert the breakpoint when that step
 	 is finished.  */
-      if ((ecs->hit_singlestep_breakpoint
-	   || thread_still_needs_step_over (ecs->event_thread))
-	  && !use_displaced_stepping (get_regcache_arch (regcache)))
+
+      remove_bp = (ecs->hit_singlestep_breakpoint
+		   || thread_still_needs_step_over (ecs->event_thread));
+      remove_wps = (ecs->event_thread->stepping_over_watchpoint
+		    && !target_have_steppable_watchpoint);
+
+      if (remove_bp && !use_displaced_stepping (get_regcache_arch (regcache)))
 	{
 	  set_step_over_info (get_regcache_aspace (regcache),
-			      regcache_read_pc (regcache));
+			      regcache_read_pc (regcache), remove_wps);
 	}
+      else if (remove_wps)
+	set_step_over_info (NULL, 0, remove_wps);
       else
 	clear_step_over_info ();
 
@@ -6056,9 +6022,7 @@ keep_going (struct execution_control_state *ecs)
 	  return;
 	}
 
-      ecs->event_thread->control.trap_expected
-	= (ecs->event_thread->stepping_over_breakpoint
-	   || ecs->hit_singlestep_breakpoint);
+      ecs->event_thread->control.trap_expected = (remove_bp || remove_wps);
 
       /* Do not deliver GDB_SIGNAL_TRAP (except when the user
 	 explicitly specifies that such a signal should be delivered
diff --git a/gdb/infrun.h b/gdb/infrun.h
index fb6276b..4d5159e 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -120,6 +120,10 @@ extern void insert_step_resume_breakpoint_at_sal (struct gdbarch *,
 extern int stepping_past_instruction_at (struct address_space *aspace,
 					 CORE_ADDR address);
 
+/* Returns true if we're trying to step past an instruction that
+   triggers a non-steppable watchpoint.  */
+extern int stepping_past_nonsteppable_watchpoint (void);
+
 extern void set_step_info (struct frame_info *frame,
 			   struct symtab_and_line sal);