[v3,2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 18244d2..7f99a5a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,30 @@ 
 2014-03-20  Pedro Alves  <palves@redhat.com>
 
+	PR breakpoints/7143
+	* breakpoint.c (should_be_inserted): Don't insert breakpoints that
+	are being stepped over.
+	(breakpoint_address_match): Make extern.
+	* breakpoint.h (breakpoint_address_match): New declaration.
+	* inferior.h (stepping_past_instruction_at): New declaration.
+	* infrun.c (struct step_over_info): New type.
+	(step_over_info): New global.
+	(set_step_over_info, clear_step_over_info)
+	(stepping_past_instruction_at): New functions.
+	(handle_inferior_event): Clear the step-over info when
+	trap_expected is cleared.
+	(resume): Remove now stale comment.
+	(clear_proceed_status): Clear step-over info.
+	(proceed): Adjust step-over handling to set or clear the step-over
+	info instead of removing all breakpoints.
+	(handle_signal_stop): When setting up a thread-hop, don't remove
+	breakpoints here.
+	(stop_stepping): Clear step-over info.
+	(keep_going): Adjust step-over handling to set or clear step-over
+	info and then always inserting breakpoints, instead of removing
+	all breakpoints when stepping over one.
+
+2014-03-20  Pedro Alves  <palves@redhat.com>
+
 	* infrun.c (previous_inferior_ptid): Adjust comment.
 	(deferred_step_ptid): Delete.
 	(infrun_thread_ptid_changed, prepare_to_proceed)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1551b99..03b21cb 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -165,11 +165,6 @@  static void describe_other_breakpoints (struct gdbarch *,
 					struct program_space *, CORE_ADDR,
 					struct obj_section *, int);
 
-static int breakpoint_address_match (struct address_space *aspace1,
-				     CORE_ADDR addr1,
-				     struct address_space *aspace2,
-				     CORE_ADDR addr2);
-
 static int watchpoint_locations_match (struct bp_location *loc1,
 				       struct bp_location *loc2);
 
@@ -2034,6 +2029,14 @@  should_be_inserted (struct bp_location *bl)
   if (bl->pspace->breakpoints_not_allowed)
     return 0;
 
+  /* Don't insert a breakpoint if we're trying to step past its
+     location.  */
+  if ((bl->loc_type == bp_loc_software_breakpoint
+       || bl->loc_type == bp_loc_hardware_breakpoint)
+      && stepping_past_instruction_at (bl->pspace->aspace,
+				       bl->address))
+    return 0;
+
   return 1;
 }
 
@@ -6792,12 +6795,9 @@  watchpoint_locations_match (struct bp_location *loc1,
 	  && loc1->length == loc2->length);
 }
 
-/* Returns true if {ASPACE1,ADDR1} and {ASPACE2,ADDR2} represent the
-   same breakpoint location.  In most targets, this can only be true
-   if ASPACE1 matches ASPACE2.  On targets that have global
-   breakpoints, the address space doesn't really matter.  */
+/* See breakpoint.h.  */
 
-static int
+int
 breakpoint_address_match (struct address_space *aspace1, CORE_ADDR addr1,
 			  struct address_space *aspace2, CORE_ADDR addr2)
 {
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index bf1f52a..b4abcb8 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1135,6 +1135,16 @@  extern int hardware_watchpoint_inserted_in_range (struct address_space *,
 extern int breakpoint_thread_match (struct address_space *, 
 				    CORE_ADDR, ptid_t);
 
+/* Returns true if {ASPACE1,ADDR1} and {ASPACE2,ADDR2} represent the
+   same breakpoint location.  In most targets, this can only be true
+   if ASPACE1 matches ASPACE2.  On targets that have global
+   breakpoints, the address space doesn't really matter.  */
+
+extern int breakpoint_address_match (struct address_space *aspace1,
+				     CORE_ADDR addr1,
+				     struct address_space *aspace2,
+				     CORE_ADDR addr2);
+
 extern void until_break_command (char *, int, int);
 
 /* Initialize a struct bp_location.  */
diff --git a/gdb/inferior.h b/gdb/inferior.h
index b15f692..64a78ce 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -222,6 +222,12 @@  void set_step_info (struct frame_info *frame, struct symtab_and_line sal);
 
 extern void clear_exit_convenience_vars (void);
 
+/* Returns true if we're trying to step past the instruction at
+   ADDRESS in ASPACE.  */
+
+extern int stepping_past_instruction_at (struct address_space *aspace,
+					 CORE_ADDR address);
+
 /* From infcmd.c */
 
 extern void post_create_inferior (struct target_ops *, int);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index f189a86..ed8ec30 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -977,6 +977,76 @@  static CORE_ADDR singlestep_pc;
 static ptid_t saved_singlestep_ptid;
 static int stepping_past_singlestep_breakpoint;
 
+/* Info about an instruction that is being stepped over.  Invalid if
+   ASPACE is NULL.  */
+
+struct step_over_info
+{
+  /* The instruction's address space.  */
+  struct address_space *aspace;
+
+  /* The instruction's address.  */
+  CORE_ADDR address;
+};
+
+/* The step-over info of the location that is being stepped over.
+
+   Note that with async/breakpoint always-inserted mode, a user might
+   set a new breakpoint/watchpoint/etc. exactly while a breakpoint is
+   being stepped over.  As setting a new breakpoint inserts all
+   breakpoints, we need to make sure the breakpoint being stepped over
+   isn't inserted then.  We do that by only clearing the step-over
+   info when the step-over is actually finished (or aborted).
+
+   Presently GDB can only step over one breakpoint at any given time.
+   Given threads that can't run code in the same address space as the
+   breakpoint's can't really miss the breakpoint, GDB could be taught
+   to step-over at most one breakpoint per address space (so this info
+   could move to the address space object if/when GDB is extended).
+   The set of breakpoints being stepped over will normally be much
+   smaller than the set of all breakpoints, so a flag in the
+   breakpoint location structure would be wasteful.  A separate list
+   also saves complexity and run-time, as otherwise we'd have to go
+   through all breakpoint locations clearing their flag whenever we
+   start a new sequence.  Similar considerations weigh against storing
+   this info in the thread object.  Plus, not all step overs actually
+   have breakpoint locations -- e.g., stepping past a single-step
+   breakpoint, or stepping to complete a non-continuable
+   watchpoint.  */
+static struct step_over_info step_over_info;
+
+/* Record the address of the breakpoint/instruction we're currently
+   stepping over.  */
+
+static void
+set_step_over_info (struct address_space *aspace, CORE_ADDR address)
+{
+  step_over_info.aspace = aspace;
+  step_over_info.address = address;
+}
+
+/* Called when we're not longer stepping over a breakpoint / an
+   instruction, so all breakpoints are free to be (re)inserted.  */
+
+static void
+clear_step_over_info (void)
+{
+  step_over_info.aspace = NULL;
+  step_over_info.address = 0;
+}
+
+/* See inferior.h.  */
+
+int
+stepping_past_instruction_at (struct address_space *aspace,
+			      CORE_ADDR address)
+{
+  return (step_over_info.aspace != NULL
+	  && breakpoint_address_match (aspace, address,
+				       step_over_info.aspace,
+				       step_over_info.address));
+}
+
 
 /* Displaced stepping.  */
 
@@ -1852,8 +1922,10 @@  a command like `return' or `jump' to continue execution."));
       remove_single_step_breakpoints ();
       singlestep_breakpoints_inserted_p = 0;
 
-      insert_breakpoints ();
+      clear_step_over_info ();
       tp->control.trap_expected = 0;
+
+      insert_breakpoints ();
     }
 
   if (should_resume)
@@ -1894,12 +1966,7 @@  a command like `return' or `jump' to continue execution."));
 	     hit, by single-stepping the thread with the breakpoint
 	     removed.  In which case, we need to single-step only this
 	     thread, and keep others stopped, as they can miss this
-	     breakpoint if allowed to run.
-
-	     The current code actually removes all breakpoints when
-	     doing this, not just the one being stepped over, so if we
-	     let other threads run, we can actually miss any
-	     breakpoint, not just the one at PC.  */
+	     breakpoint if allowed to run.  */
 	  resume_ptid = inferior_ptid;
 	}
 
@@ -2031,6 +2098,8 @@  clear_proceed_status (void)
 
   stop_after_trap = 0;
 
+  clear_step_over_info ();
+
   observer_notify_about_to_proceed ();
 
   if (stop_registers)
@@ -2217,22 +2286,23 @@  proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
   tp = inferior_thread ();
 
   if (force_step)
+    tp->control.trap_expected = 1;
+
+  /* If we need to step over a breakpoint, and we're not using
+     displaced stepping to do so, insert all breakpoints (watchpoints,
+     etc.) but the one we're stepping over, step one instruction, and
+     then re-insert the breakpoint when that step is finished.  */
+  if (tp->control.trap_expected && !use_displaced_stepping (gdbarch))
     {
-      tp->control.trap_expected = 1;
-      /* If displaced stepping is enabled, we can step over the
-	 breakpoint without hitting it, so leave all breakpoints
-	 inserted.  Otherwise we need to disable all breakpoints, step
-	 one instruction, and then re-add them when that step is
-	 finished.  */
-      if (!use_displaced_stepping (gdbarch))
-	remove_breakpoints ();
+      struct regcache *regcache = get_current_regcache ();
+
+      set_step_over_info (get_regcache_aspace (regcache),
+			  regcache_read_pc (regcache));
     }
+  else
+    clear_step_over_info ();
 
-  /* We can insert breakpoints if we're not trying to step over one,
-     or if we are stepping over one but we're using displaced stepping
-     to do so.  */
-  if (! tp->control.trap_expected || use_displaced_stepping (gdbarch))
-    insert_breakpoints ();
+  insert_breakpoints ();
 
   if (!non_stop)
     {
@@ -3992,9 +4062,6 @@  handle_signal_stop (struct execution_control_state *ecs)
 
       if (thread_hop_needed)
 	{
-	  struct regcache *thread_regcache;
-	  int remove_status = 0;
-
 	  if (debug_infrun)
 	    fprintf_unfiltered (gdb_stdlog, "infrun: thread_hop_needed\n");
 
@@ -4013,35 +4080,17 @@  handle_signal_stop (struct execution_control_state *ecs)
 	      singlestep_breakpoints_inserted_p = 0;
 	    }
 
-	  /* If the arch can displace step, don't remove the
-	     breakpoints.  */
-	  thread_regcache = get_thread_regcache (ecs->ptid);
-	  if (!use_displaced_stepping (get_regcache_arch (thread_regcache)))
-	    remove_status = remove_breakpoints ();
-
-	  /* Did we fail to remove breakpoints?  If so, try
-	     to set the PC past the bp.  (There's at least
-	     one situation in which we can fail to remove
-	     the bp's: On HP-UX's that use ttrace, we can't
-	     change the address space of a vforking child
-	     process until the child exits (well, okay, not
-	     then either :-) or execs.  */
-	  if (remove_status != 0)
-	    error (_("Cannot step over breakpoint hit in wrong thread"));
-	  else
-	    {			/* Single step */
-	      if (!non_stop)
-		{
-		  /* Only need to require the next event from this
-		     thread in all-stop mode.  */
-		  waiton_ptid = ecs->ptid;
-		  infwait_state = infwait_thread_hop_state;
-		}
-
-	      ecs->event_thread->stepping_over_breakpoint = 1;
-	      keep_going (ecs);
-	      return;
+	  if (!non_stop)
+	    {
+	      /* Only need to require the next event from this thread
+		 in all-stop mode.  */
+	      waiton_ptid = ecs->ptid;
+	      infwait_state = infwait_thread_hop_state;
 	    }
+
+	  ecs->event_thread->stepping_over_breakpoint = 1;
+	  keep_going (ecs);
+	  return;
 	}
     }
 
@@ -5695,6 +5744,8 @@  stop_stepping (struct execution_control_state *ecs)
   if (debug_infrun)
     fprintf_unfiltered (gdb_stdlog, "infrun: stop_stepping\n");
 
+  clear_step_over_info ();
+
   /* Let callers know we don't want to wait for the inferior anymore.  */
   ecs->wait_some_more = 0;
 }
@@ -5727,6 +5778,9 @@  keep_going (struct execution_control_state *ecs)
     }
   else
     {
+      volatile struct gdb_exception e;
+      struct regcache *regcache = get_current_regcache ();
+
       /* Either the trap was not expected, but we are continuing
 	 anyway (if we got a signal, the user asked it be passed to
 	 the child)
@@ -5740,33 +5794,30 @@  keep_going (struct execution_control_state *ecs)
 	 already inserted breakpoints.  Therefore, we don't
 	 care if breakpoints were already inserted, or not.  */
 
-      if (ecs->event_thread->stepping_over_breakpoint)
+      /* If we need to step over a breakpoint, and we're not using
+	 displaced stepping to do so, insert all breakpoints
+	 (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->event_thread->stepping_over_breakpoint
+	  && !use_displaced_stepping (get_regcache_arch (regcache)))
 	{
-	  struct regcache *thread_regcache = get_thread_regcache (ecs->ptid);
-
-	  if (!use_displaced_stepping (get_regcache_arch (thread_regcache)))
-	    {
-	      /* Since we can't do a displaced step, we have to remove
-		 the breakpoint while we step it.  To keep things
-		 simple, we remove them all.  */
-	      remove_breakpoints ();
-	    }
+	  set_step_over_info (get_regcache_aspace (regcache),
+			      regcache_read_pc (regcache));
 	}
       else
-	{
-	  volatile struct gdb_exception e;
+	clear_step_over_info ();
 
-	  /* Stop stepping if inserting breakpoints fails.  */
-	  TRY_CATCH (e, RETURN_MASK_ERROR)
-	    {
-	      insert_breakpoints ();
-	    }
-	  if (e.reason < 0)
-	    {
-	      exception_print (gdb_stderr, e);
-	      stop_stepping (ecs);
-	      return;
-	    }
+      /* Stop stepping if inserting breakpoints fails.  */
+      TRY_CATCH (e, RETURN_MASK_ERROR)
+	{
+	  insert_breakpoints ();
+	}
+      if (e.reason < 0)
+	{
+	  exception_print (gdb_stderr, e);
+	  stop_stepping (ecs);
+	  return;
 	}
 
       ecs->event_thread->control.trap_expected
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 1986747..ee71b74 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,14 @@ 
 2014-03-20  Pedro Alves  <palves@redhat.com>
 
+	PR breakpoints/7143
+	* gdb.base/watchpoint.exp: Mention bugzilla bug number instead of
+	old gnats gdb/38.  Remove kfail.  Adjust to use gdb_test instead
+	of gdb_test_multiple.
+	* gdb.cp/annota2.exp: Remove kfail for gdb/38.
+	* gdb.cp/annota3.exp: Remove kfail for gdb/38.
+
+2014-03-20  Pedro Alves  <palves@redhat.com>
+
 	* gdb.threads/step-over-lands-on-breakpoint.c: New file.
 	* gdb.threads/step-over-lands-on-breakpoint.exp: New file.
 
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index 80d75cb..1123824 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -550,21 +550,16 @@  proc test_complex_watchpoint {} {
 proc test_watchpoint_and_breakpoint {} {
     global gdb_prompt
 
-    # This is a test for PR gdb/38, which involves setting a
+    # This is a test for PR breakpoints/7143, which involves setting a
     # watchpoint right after you've reached a breakpoint.
 
     if [runto func3] then {
 	gdb_breakpoint [gdb_get_line_number "second x assignment"]
 	gdb_continue_to_breakpoint "second x assignment"
 	gdb_test "watch x" ".*atchpoint \[0-9\]+: x"
-	gdb_test_multiple "next" "next after watch x" {
-	    -re ".*atchpoint \[0-9\]+: x\r\n\r\nOld value = 0\r\nNew value = 1\r\n.*$gdb_prompt $" {
-		pass "next after watch x"
-	    }
-	    -re "\[0-9\]+\[\t \]+y = 1;\r\n$gdb_prompt $" {
-		kfail "gdb/38" "next after watch x"
-	    }
-	}
+	gdb_test "next" \
+	    ".*atchpoint \[0-9\]+: x\r\n\r\nOld value = 0\r\nNew value = 1\r\n.*" \
+	    "next after watch x"
 
 	gdb_test_no_output "delete \$bpnum" "delete watch x"
     }
diff --git a/gdb/testsuite/gdb.cp/annota2.exp b/gdb/testsuite/gdb.cp/annota2.exp
index 00a6067..6fbf4b5 100644
--- a/gdb/testsuite/gdb.cp/annota2.exp
+++ b/gdb/testsuite/gdb.cp/annota2.exp
@@ -165,9 +165,6 @@  gdb_test_multiple "next" "watch triggered on a.x" {
     -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n${frames_invalid}${breakpoints_invalid}\r\n\032\032watchpoint 3\r\n.*atchpoint 3: a.x\r\n\r\nOld value = 0\r\nNew value = 1\r\n\r\n\032\032frame-begin 0 $hex\r\n\r\n\032\032frame-function-name\r\nmain\r\n\032\032frame-args\r\n \\(\\)\r\n\032\032frame-source-begin\r\n at \r\n\032\032frame-source-file\r\n.*$srcfile\r\n\032\032frame-source-file-end\r\n:\r\n\032\032frame-source-line\r\n$decimal\r\n\032\032frame-source-end\r\n\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n.*$gdb_prompt$" {
 	pass "watch triggered on a.x"
     }
-    -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n${frames_invalid}\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n$gdb_prompt$" {
-	kfail "gdb/38" "watch triggered on a.x"
-    }
 }
 
 
diff --git a/gdb/testsuite/gdb.cp/annota3.exp b/gdb/testsuite/gdb.cp/annota3.exp
index 957d371..bbf9a1e 100644
--- a/gdb/testsuite/gdb.cp/annota3.exp
+++ b/gdb/testsuite/gdb.cp/annota3.exp
@@ -169,9 +169,6 @@  gdb_test_multiple "next" "watch triggered on a.x" {
     -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n\r\n\032\032watchpoint 3\r\n.*atchpoint 3: a.x\r\n\r\nOld value = 0\r\nNew value = 1\r\n\r\n(\032\032frame-begin 0 0x\[0-9a-z\]+\r\n|)main \\(\\) at .*$srcfile:$decimal\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032stopped\r\n.*$gdb_prompt$" { 
 	pass "watch triggered on a.x"
     }
-    -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032stopped\r\n$gdb_prompt$" {
-	kfail "gdb/38" "watch triggered on a.x"
-    }
 }
 
 #