Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [rfc, rfa/doc] Multi-threaded watchpoint improvements
@ 2007-09-16 18:40 Daniel Jacobowitz
  2007-09-22  9:03 ` Eli Zaretskii
  2007-10-01  0:20 ` Daniel Jacobowitz
  0 siblings, 2 replies; 27+ messages in thread
From: Daniel Jacobowitz @ 2007-09-16 18:40 UTC (permalink / raw)
  To: gdb-patches

I have (finally, sorry for the delay) finished revamping the
multi-threaded watchpoint patches Jeff submitted ages ago and Luis
resubmitted recently.  This message contains all the changes except
for various native GNU/Linux target files.  It includes manual and
gdbint changes describing what I've figured out to date.

The big change is in the handling of steppable and nonsteppable
watchpoints.  Unlike continuable watchpoints, steppable and
nonsteppable watchpoints trigger before the monitored write (or read)
occurs.  We detect the watchpoint and single step before displaying it
to the user, so that we can show the old and new values.  Before the
attached patch, we called target_stopped_data_address after single
stepping.  This patch rearranges things so that we figure out
which watchpoint triggered, or might have triggered, before
we single step.  The horrible stepped_after_stopped_by_watchpoint
hack in remote.c goes away as a consequence.

infrun.c calls the new watchpoints_triggered function at each trap
event - except for the step after a steppable or nonsteppable
watchpoint, in which case we use the watchpoint status from right
after the trigger event.  If we have not stopped based on a
watchpoint, we mark all hardware watchpoints as not triggered.  If we
have stopped but the target doesn't implement
target_stopped_data_address, we mark each watchpoint as possibly
triggered.  And if it does implement target_stopped_data_address, we
mark each watchpoint as triggered or not triggered based on the
reported address.  When bpstat_stop_status is called, it uses
the triggered status and the type to determine whether we need
to check the watchpoint's value for changes.

A related change was to scope breakpoint handling.  The change I
described in the previous paragraph causes GDB to not always check
each hardware watchpoint at every stop that might have been caused by
a watchpoint.  This meant we weren't getting the expected stop and
message when a watchpoint went out of scope.  I arranged to always
check a watchpoint when its corresponding scope breakpoint triggers.
This was already inconsistent for access and read watchpoints on local
variables, but those are used much less often than hardware
watchpoints, so it was never noticed.

By the way, the current WP_DELETED handling only worked when
STOPPED_BY_WATCHPOINT returned true after stopping on a scope
breakpoint.  And you wouldn't expect it to, since the scope
breakpoint isn't a watchpoint.  But in fact it usually was set.  See
the paragraph I added in gdbint.texinfo about 'sticky' status.
The watchpoint triggered bits in i386's DR_STATUS register are
definitely sticky; they are only reset by "some events" according
to the architecture manual.  I suspect those events are triggers
of other watchpoints or hardware breakpoints.  This means access
watchpoints on i386 trigger too often (a separate bug not fixed
by this patch).

The change for bp_thread_event and bp_overlay_event prevents
displaying a confused message when a watchpoint triggers in
one thread and another thread reaches __nptl_create_event
while we are trying to stop all threads.  The watchthreads.exp
testcase triggers this about half the time.

The code at the end of bpstat_stop_status to reset hardware
watchpoints only worked most of the time.  It checked the
type of the first entry in the bpstat chain, but ordering
might mean that the watchpoint was the second or later
entry.  I rewrote it to check the whole chain.

And last but not least, this patch makes the watchthreads.exp test
case work on targets where the stopped data address is not available;
on these it is unavoidable to detect two threads hitting watchpoints
simultaneously, since we do not know which thread has hit which
watchpoint.  On other targets we serialize the watchpoint hits which
has the very useful benefit of showing the associated source location
for each watchpoint hit.

Do these changes look OK?  If any part is confusing, just ask.  I was
originally planning to submit this as several separate patches, but
the only one of any real size is the watchpoints_triggered patch
and the others are a little hard to separate from it due to textual
overlap.

This version was tested on x86_64-linux; prior versions have been
tested on ia64-linux, powerpc64-linux, s390-linux, and i386-linux.
That's every GNU/Linux target with hardware watchpoint support.  I
can't readily test non-Linux targets, but I am very optimistic that I
didn't break anything.

-- 
Daniel Jacobowitz
CodeSourcery

2007-09-16  Daniel Jacobowitz  <dan@codesourcery.com>
	    Jeff Johnston  <jjohnstn@redhat.com>

	* breakpoint.c (watchpoints_triggered): New.
	(bpstat_stop_status): Remove STOPPED_BY_WATCHPOINT argument.
	Check watchpoint_triggered instead.  Combine handling for software
	and hardware watchpoints.  Do not use target_stopped_data_address
	here.  Always check a watchpoint if its scope breakpoint triggers.
	Do not stop for thread or overlay events.  Improve check for
	triggered watchpoints without a value change.
	(watch_command_1): Insert the scope breakpoint first.  Link the
	scope breakpoint to the watchpoint.
	* breakpoint.h (enum watchpoint_triggered): New.
	(struct breakpoint): Add watchpoint_triggered.
	(bpstat_stop_status): Update prototype.
	(watchpoints_triggered): Declare.
	* infrun.c (enum infwait_status): Add infwait_step_watch_state.
	(stepped_after_stopped_by_watchpoint): Delete.
	(handle_inferior_event): Make stepped_after_stopped_by_watchpoint
	local.  Handle infwait_step_watch_state.  Update calls to
	bpstat_stop_status.  Use watchpoints_triggered to check
	watchpoints.
	* remote.c (stepped_after_stopped_by_watchpoint): Remove extern.
	(remote_stopped_data_address): Do not check it.

2007-09-16  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdb.texinfo (Setting Watchpoints): Adjust warning text about
	multi-threaded watchpoints.
	* gdbint.texinfo (Watchpoints): Describe how watchpoints are
	checked.  Describe sticky notification.  Expand description
	of steppable and continuable watchpoints.
	(Watchpoints and Threads): New subsection.

2007-09-16  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdb.threads/watchthreads.c (thread_function): Sleep between
	iterations.
	* gdb.threads/watchthreads.exp: Allow two watchpoints to trigger
	at once for S/390.  Generate matching fails and passes.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.263
diff -u -p -r1.263 breakpoint.c
--- breakpoint.c	29 Aug 2007 22:07:47 -0000	1.263
+++ breakpoint.c	16 Sep 2007 18:05:27 -0000
@@ -2519,6 +2519,83 @@ bpstat_alloc (struct breakpoint *b, bpst
   return bs;
 }
 \f
+/* The target has stopped with waitstatus WS.  Check if any hardware
+   watchpoints have triggered, according to the target.  */
+
+int
+watchpoints_triggered (struct target_waitstatus *ws)
+{
+  int stopped_by_watchpoint = STOPPED_BY_WATCHPOINT (*ws);
+  CORE_ADDR addr;
+  struct breakpoint *b;
+
+  if (!stopped_by_watchpoint)
+    {
+      /* We were not stopped by a watchpoint.  Mark all watchpoints
+	 as not triggered.  */
+      ALL_BREAKPOINTS (b)
+	if (b->type == bp_hardware_watchpoint
+	    || b->type == bp_read_watchpoint
+	    || b->type == bp_access_watchpoint)
+	  b->watchpoint_triggered = watch_triggered_no;
+
+      return 0;
+    }
+
+  if (!target_stopped_data_address (&current_target, &addr))
+    {
+      /* We were stopped by a watchpoint, but we don't know where.
+	 Mark all watchpoints as unknown.  */
+      ALL_BREAKPOINTS (b)
+	if (b->type == bp_hardware_watchpoint
+	    || b->type == bp_read_watchpoint
+	    || b->type == bp_access_watchpoint)
+	  b->watchpoint_triggered = watch_triggered_unknown;
+
+      return stopped_by_watchpoint;
+    }
+
+  /* The target could report the data address.  Mark watchpoints
+     affected by this data address as triggered, and all others as not
+     triggered.  */
+
+  ALL_BREAKPOINTS (b)
+    if (b->type == bp_hardware_watchpoint
+	|| b->type == bp_read_watchpoint
+	|| b->type == bp_access_watchpoint)
+      {
+	struct value *v;
+
+	b->watchpoint_triggered = watch_triggered_no;
+	for (v = b->val_chain; v; v = value_next (v))
+	  {
+	    if (VALUE_LVAL (v) == lval_memory && ! value_lazy (v))
+	      {
+		struct type *vtype = check_typedef (value_type (v));
+
+		if (v == b->val_chain
+		    || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
+			&& TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
+		  {
+		    CORE_ADDR vaddr;
+
+		    vaddr = VALUE_ADDRESS (v) + value_offset (v);
+		    /* Exact match not required.  Within range is
+		       sufficient.  */
+		    if (addr >= vaddr
+			&& addr < vaddr + TYPE_LENGTH (value_type (v)))
+		      {
+			b->watchpoint_triggered = watch_triggered_yes;
+			break;
+		      }
+		  }
+	      }
+	  }
+      }
+
+  return 1;
+}
+
 /* Possible return values for watchpoint_check (this can't be an enum
    because of check_errors).  */
 /* The watchpoint has been deleted.  */
@@ -2637,11 +2714,9 @@ which its expression is valid.\n");     
 }
 
 /* Get a bpstat associated with having just stopped at address
-   BP_ADDR in thread PTID.  STOPPED_BY_WATCHPOINT is 1 if the
-   target thinks we stopped due to a hardware watchpoint, 0 if we
-   know we did not trigger a hardware watchpoint, and -1 if we do not know.  */
+   BP_ADDR in thread PTID.
 
-/* Determine whether we stopped at a breakpoint, etc, or whether we
+   Determine whether we stopped at a breakpoint, etc, or whether we
    don't understand this stop.  Result is a chain of bpstat's such that:
 
    if we don't understand the stop, the result is a null pointer.
@@ -2656,7 +2731,7 @@ which its expression is valid.\n");     
    commands, FIXME??? fields.  */
 
 bpstat
-bpstat_stop_status (CORE_ADDR bp_addr, ptid_t ptid, int stopped_by_watchpoint)
+bpstat_stop_status (CORE_ADDR bp_addr, ptid_t ptid)
 {
   struct breakpoint *b, *temp;
   /* True if we've hit a breakpoint (as opposed to a watchpoint).  */
@@ -2691,16 +2766,17 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
 	  continue;
       }
 
-    /* Continuable hardware watchpoints are treated as non-existent if the 
-       reason we stopped wasn't a hardware watchpoint (we didn't stop on 
-       some data address).  Otherwise gdb won't stop on a break instruction 
-       in the code (not from a breakpoint) when a hardware watchpoint has 
-       been defined.  */
+    /* Continuable hardware watchpoints are treated as non-existent if the
+       reason we stopped wasn't a hardware watchpoint (we didn't stop on
+       some data address).  Otherwise gdb won't stop on a break instruction
+       in the code (not from a breakpoint) when a hardware watchpoint has
+       been defined.  Also skip watchpoints which we know did not trigger
+       (did not match the data address).  */
 
     if ((b->type == bp_hardware_watchpoint
 	 || b->type == bp_read_watchpoint
 	 || b->type == bp_access_watchpoint)
-	&& !stopped_by_watchpoint)
+	&& b->watchpoint_triggered == watch_triggered_no)
       continue;
 
     if (b->type == bp_hardware_breakpoint)
@@ -2766,82 +2842,33 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
     bs->stop = 1;
     bs->print = 1;
 
-    if (b->type == bp_watchpoint ||
-	b->type == bp_hardware_watchpoint)
-      {
-	char *message = xstrprintf ("Error evaluating expression for watchpoint %d\n",
-				    b->number);
-	struct cleanup *cleanups = make_cleanup (xfree, message);
-	int e = catch_errors (watchpoint_check, bs, message, 
-			      RETURN_MASK_ALL);
-	do_cleanups (cleanups);
-	switch (e)
-	  {
-	  case WP_DELETED:
-	    /* We've already printed what needs to be printed.  */
-	    /* Actually this is superfluous, because by the time we
-               call print_it_typical() the wp will be already deleted,
-               and the function will return immediately. */
-	    bs->print_it = print_it_done;
-	    /* Stop.  */
-	    break;
-	  case WP_VALUE_CHANGED:
-	    /* Stop.  */
-	    ++(b->hit_count);
-	    break;
-	  case WP_VALUE_NOT_CHANGED:
-	    /* Don't stop.  */
-	    bs->print_it = print_it_noop;
-	    bs->stop = 0;
-	    continue;
-	  default:
-	    /* Can't happen.  */
-	    /* FALLTHROUGH */
-	  case 0:
-	    /* Error from catch_errors.  */
-	    printf_filtered (_("Watchpoint %d deleted.\n"), b->number);
-	    if (b->related_breakpoint)
-	      b->related_breakpoint->disposition = disp_del_at_next_stop;
-	    b->disposition = disp_del_at_next_stop;
-	    /* We've already printed what needs to be printed.  */
-	    bs->print_it = print_it_done;
-
-	    /* Stop.  */
-	    break;
-	  }
-      }
-    else if (b->type == bp_read_watchpoint || 
-	     b->type == bp_access_watchpoint)
+    if (b->type == bp_watchpoint
+	|| b->type == bp_read_watchpoint
+	|| b->type == bp_access_watchpoint
+	|| b->type == bp_hardware_watchpoint)
       {
 	CORE_ADDR addr;
 	struct value *v;
-	int found = 0;
+	int must_check_value = 0;
 
-	if (!target_stopped_data_address (&current_target, &addr))
-	  continue;
-	for (v = b->val_chain; v; v = value_next (v))
-	  {
-	    if (VALUE_LVAL (v) == lval_memory
-		&& ! value_lazy (v))
-	      {
-		struct type *vtype = check_typedef (value_type (v));
-
-		if (v == b->val_chain
-		    || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
-			&& TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
-		  {
-		    CORE_ADDR vaddr;
+ 	if (b->type == bp_watchpoint)
+	  /* For a software watchpoint, we must always check the
+	     watched value.  */
+	  must_check_value = 1;
+	else if (b->watchpoint_triggered == watch_triggered_yes)
+	  /* We have a hardware watchpoint (read, write, or access)
+	     and the target earlier reported an address watched by
+	     this watchpoint.  */
+	  must_check_value = 1;
+	else if (b->watchpoint_triggered == watch_triggered_unknown
+		 && b->type == bp_hardware_watchpoint)
+	  /* We were stopped by a hardware watchpoint, but the target could
+	     not report the data address.  We must check the watchpoint's
+	     value.  Access and read watchpoints are out of luck; without
+	     a data address, we can't figure it out.  */
+	  must_check_value = 1;
 
-		    vaddr = VALUE_ADDRESS (v) + value_offset (v);
-		    /* Exact match not required.  Within range is
-                       sufficient.  */
-		    if (addr >= vaddr &&
-			addr < vaddr + TYPE_LENGTH (value_type (v)))
-		      found = 1;
-		  }
-	      }
-	  }
-	if (found)
+ 	if (must_check_value)
 	  {
 	    char *message = xstrprintf ("Error evaluating expression for watchpoint %d\n",
 					b->number);
@@ -2869,6 +2896,15 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
 		++(b->hit_count);
 		break;
 	      case WP_VALUE_NOT_CHANGED:
+		if (b->type == bp_hardware_watchpoint
+		    || b->type == bp_watchpoint)
+		  {
+		    /* Don't stop: write watchpoints shouldn't fire if
+		       the value hasn't changed.  */
+		    bs->print_it = print_it_noop;
+		    bs->stop = 0;
+		    continue;
+		  }
 		/* Stop.  */
 		++(b->hit_count);
 		break;
@@ -2885,12 +2921,12 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
 		break;
 	      }
 	  }
-	else	/* found == 0 */
+	else	/* must_check_value == 0 */
 	  {
-	    /* This is a case where some watchpoint(s) triggered,
-	       but not at the address of this watchpoint (FOUND
-	       was left zero).  So don't print anything for this
-	       watchpoint.  */
+	    /* This is a case where some watchpoint(s) triggered, but
+	       not at the address of this watchpoint, or else no
+	       watchpoint triggered after all.  So don't print
+	       anything for this watchpoint.  */
 	    bs->print_it = print_it_noop;
 	    bs->stop = 0;
             continue;
@@ -2912,6 +2948,13 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
       {
 	int value_is_zero = 0;
 
+	/* If this is a scope breakpoint, mark the associated
+	   watchpoint as triggered so that we will handle the
+	   out-of-scope event.  We'll get to the watchpoint next
+	   iteration.  */
+	if (b->type == bp_watchpoint_scope)
+	  b->related_breakpoint->watchpoint_triggered = watch_triggered_yes;
+
 	if (b->cond)
 	  {
 	    /* Need to select the frame, with all that implies
@@ -2942,6 +2985,9 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
 	    annotate_ignore_count_change ();
 	    bs->stop = 0;
 	  }
+	else if (b->type == bp_thread_event || b->type == bp_overlay_event)
+	  /* We do not stop for these.  */
+	  bs->stop = 0;
 	else
 	  {
 	    /* We will stop here */
@@ -2968,17 +3014,27 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
   bs->next = NULL;		/* Terminate the chain */
   bs = root_bs->next;		/* Re-grab the head of the chain */
 
-  /* The value of a hardware watchpoint hasn't changed, but the
-     intermediate memory locations we are watching may have.  */
-  if (bs && !bs->stop &&
-      (bs->breakpoint_at->type == bp_hardware_watchpoint ||
-       bs->breakpoint_at->type == bp_read_watchpoint ||
-       bs->breakpoint_at->type == bp_access_watchpoint))
-    {
-      remove_breakpoints ();
-      insert_breakpoints ();
-    }
-  return bs;
+  /* If we aren't stopping, the value of some hardware watchpoint may
+     not have changed, but the intermediate memory locations we are
+     watching may have.  Don't bother if we're stopping; this will get
+     done later.  */
+  for (bs = root_bs->next; bs != NULL; bs = bs->next)
+    if (bs->stop)
+      break;
+
+  if (bs == NULL)
+    for (bs = root_bs->next; bs != NULL; bs = bs->next)
+      if (!bs->stop
+	  && (bs->breakpoint_at->type == bp_hardware_watchpoint
+	      || bs->breakpoint_at->type == bp_read_watchpoint
+	      || bs->breakpoint_at->type == bp_access_watchpoint))
+	{
+	  remove_breakpoints ();
+	  insert_breakpoints ();
+	  break;
+	}
+
+  return root_bs->next;
 }
 \f
 /* Tell what to do about this bpstat.  */
@@ -5694,7 +5750,7 @@ stopat_command (char *arg, int from_tty)
 static void
 watch_command_1 (char *arg, int accessflag, int from_tty)
 {
-  struct breakpoint *b;
+  struct breakpoint *b, *scope_breakpoint = NULL;
   struct symtab_and_line sal;
   struct expression *exp;
   struct block *exp_valid_block;
@@ -5772,6 +5828,37 @@ watch_command_1 (char *arg, int accessfl
   if (!mem_cnt || target_resources_ok <= 0)
     bp_type = bp_watchpoint;
 
+  frame = block_innermost_frame (exp_valid_block);
+  if (frame)
+    prev_frame = get_prev_frame (frame);
+  else
+    prev_frame = NULL;
+
+  /* If the expression is "local", then set up a "watchpoint scope"
+     breakpoint at the point where we've left the scope of the watchpoint
+     expression.  Create the scope breakpoint before the watchpoint, so
+     that we will encounter it first in bpstat_stop_status.  */
+  if (innermost_block && prev_frame)
+    {
+      scope_breakpoint = create_internal_breakpoint (get_frame_pc (prev_frame),
+						     bp_watchpoint_scope);
+
+      scope_breakpoint->enable_state = bp_enabled;
+
+      /* Automatically delete the breakpoint when it hits.  */
+      scope_breakpoint->disposition = disp_del;
+
+      /* Only break in the proper frame (help with recursion).  */
+      scope_breakpoint->frame_id = get_frame_id (prev_frame);
+
+      /* Set the address at which we will stop.  */
+      scope_breakpoint->loc->requested_address
+	= get_frame_pc (prev_frame);
+      scope_breakpoint->loc->address
+	= adjust_breakpoint_address (scope_breakpoint->loc->requested_address,
+				     scope_breakpoint->type);
+    }
+
   /* Now set up the breakpoint.  */
   b = set_raw_breakpoint (sal, bp_type);
   set_breakpoint_count (breakpoint_count + 1);
@@ -5787,48 +5874,19 @@ watch_command_1 (char *arg, int accessfl
   else
     b->cond_string = 0;
 
-  frame = block_innermost_frame (exp_valid_block);
   if (frame)
-    {
-      prev_frame = get_prev_frame (frame);
-      b->watchpoint_frame = get_frame_id (frame);
-    }
+    b->watchpoint_frame = get_frame_id (frame);
   else
-    {
-      memset (&b->watchpoint_frame, 0, sizeof (b->watchpoint_frame));
-    }
+    memset (&b->watchpoint_frame, 0, sizeof (b->watchpoint_frame));
 
-  /* If the expression is "local", then set up a "watchpoint scope"
-     breakpoint at the point where we've left the scope of the watchpoint
-     expression.  */
-  if (innermost_block)
+  if (scope_breakpoint != NULL)
     {
-      if (prev_frame)
-	{
-	  struct breakpoint *scope_breakpoint;
-	  scope_breakpoint = create_internal_breakpoint (get_frame_pc (prev_frame),
-							 bp_watchpoint_scope);
-
-	  scope_breakpoint->enable_state = bp_enabled;
-
-	  /* Automatically delete the breakpoint when it hits.  */
-	  scope_breakpoint->disposition = disp_del;
-
-	  /* Only break in the proper frame (help with recursion).  */
-	  scope_breakpoint->frame_id = get_frame_id (prev_frame);
-
-	  /* Set the address at which we will stop.  */
-	  scope_breakpoint->loc->requested_address
-	    = get_frame_pc (prev_frame);
-	  scope_breakpoint->loc->address
-	    = adjust_breakpoint_address (scope_breakpoint->loc->requested_address,
-	                                 scope_breakpoint->type);
-
-	  /* The scope breakpoint is related to the watchpoint.  We
-	     will need to act on them together.  */
-	  b->related_breakpoint = scope_breakpoint;
-	}
+      /* The scope breakpoint is related to the watchpoint.  We will
+	 need to act on them together.  */
+      b->related_breakpoint = scope_breakpoint;
+      scope_breakpoint->related_breakpoint = b;
     }
+
   value_free_to_mark (mark);
   mention (b);
 }
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.47
diff -u -p -r1.47 breakpoint.h
--- breakpoint.h	23 Aug 2007 18:08:26 -0000	1.47
+++ breakpoint.h	16 Sep 2007 18:05:27 -0000
@@ -299,6 +299,19 @@ struct breakpoint_ops 
   void (*print_mention) (struct breakpoint *);
 };
 
+enum watchpoint_triggered
+{
+  /* This watchpoint definitely did not trigger.  */
+  watch_triggered_no = 0,
+
+  /* Some hardware watchpoint triggered, and it might have been this
+     one, but we do not know which it was.  */
+  watch_triggered_unknown,
+
+  /* This hardware watchpoint definitely did trigger.  */
+  watch_triggered_yes  
+};
+
 /* Note that the ->silent field is not currently used by any commands
    (though the code is in there if it was to be, and set_raw_breakpoint
    does set it to 0).  I implemented it because I thought it would be
@@ -378,6 +391,10 @@ struct breakpoint
        should be evaluated on the outermost frame.  */
     struct frame_id watchpoint_frame;
 
+    /* For hardware watchpoints, the triggered status according to the
+       hardware.  */
+    enum watchpoint_triggered watchpoint_triggered;
+
     /* Thread number for thread-specific breakpoint, or -1 if don't care */
     int thread;
 
@@ -440,8 +457,7 @@ extern void bpstat_clear (bpstat *);
    is part of the bpstat is copied as well.  */
 extern bpstat bpstat_copy (bpstat);
 
-extern bpstat bpstat_stop_status (CORE_ADDR pc, ptid_t ptid, 
-				  int stopped_by_watchpoint);
+extern bpstat bpstat_stop_status (CORE_ADDR pc, ptid_t ptid);
 \f
 /* This bpstat_what stuff tells wait_for_inferior what to do with a
    breakpoint (a challenging task).  */
@@ -836,4 +852,8 @@ extern void remove_single_step_breakpoin
 extern void *deprecated_insert_raw_breakpoint (CORE_ADDR);
 extern int deprecated_remove_raw_breakpoint (void *);
 
+/* Check if any hardware watchpoints have triggered, according to the
+   target.  */
+int watchpoints_triggered (struct target_waitstatus *);
+
 #endif /* !defined (BREAKPOINT_H) */
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.245
diff -u -p -r1.245 infrun.c
--- infrun.c	23 Aug 2007 18:08:35 -0000	1.245
+++ infrun.c	16 Sep 2007 18:05:28 -0000
@@ -882,6 +882,7 @@ enum infwait_states
 {
   infwait_normal_state,
   infwait_thread_hop_state,
+  infwait_step_watch_state,
   infwait_nonstep_watch_state
 };
 
@@ -1221,17 +1222,12 @@ adjust_pc_after_break (struct execution_
    by an event from the inferior, figure out what it means and take
    appropriate action.  */
 
-int stepped_after_stopped_by_watchpoint;
-
 void
 handle_inferior_event (struct execution_control_state *ecs)
 {
-  /* NOTE: bje/2005-05-02: If you're looking at this code and thinking
-     that the variable stepped_after_stopped_by_watchpoint isn't used,
-     then you're wrong!  See remote.c:remote_stopped_data_address.  */
-
   int sw_single_step_trap_p = 0;
-  int stopped_by_watchpoint = -1;	/* Mark as unknown.  */
+  int stopped_by_watchpoint;
+  int stepped_after_stopped_by_watchpoint = 0;
 
   /* Cache the last pid/waitstatus. */
   target_last_wait_ptid = ecs->ptid;
@@ -1251,7 +1247,14 @@ handle_inferior_event (struct execution_
     case infwait_normal_state:
       if (debug_infrun)
         fprintf_unfiltered (gdb_stdlog, "infrun: infwait_normal_state\n");
-      stepped_after_stopped_by_watchpoint = 0;
+      break;
+
+    case infwait_step_watch_state:
+      if (debug_infrun)
+        fprintf_unfiltered (gdb_stdlog,
+			    "infrun: infwait_step_watch_state\n");
+
+      stepped_after_stopped_by_watchpoint = 1;
       break;
 
     case infwait_nonstep_watch_state:
@@ -1440,7 +1443,7 @@ handle_inferior_event (struct execution_
 
       stop_pc = read_pc ();
 
-      stop_bpstat = bpstat_stop_status (stop_pc, ecs->ptid, 0);
+      stop_bpstat = bpstat_stop_status (stop_pc, ecs->ptid);
 
       ecs->random_signal = !bpstat_explains_signal (stop_bpstat);
 
@@ -1488,7 +1491,7 @@ handle_inferior_event (struct execution_
       ecs->saved_inferior_ptid = inferior_ptid;
       inferior_ptid = ecs->ptid;
 
-      stop_bpstat = bpstat_stop_status (stop_pc, ecs->ptid, 0);
+      stop_bpstat = bpstat_stop_status (stop_pc, ecs->ptid);
 
       ecs->random_signal = !bpstat_explains_signal (stop_bpstat);
       inferior_ptid = ecs->saved_inferior_ptid;
@@ -1770,24 +1773,20 @@ handle_inferior_event (struct execution_
       singlestep_breakpoints_inserted_p = 0;
     }
 
-  /* It may not be necessary to disable the watchpoint to stop over
-     it.  For example, the PA can (with some kernel cooperation)
-     single step over a watchpoint without disabling the watchpoint.  */
-  if (HAVE_STEPPABLE_WATCHPOINT && STOPPED_BY_WATCHPOINT (ecs->ws))
+  if (stepped_after_stopped_by_watchpoint)
+    stopped_by_watchpoint = 0;
+  else
+    stopped_by_watchpoint = watchpoints_triggered (&ecs->ws);
+
+  /* If necessary, step over this watchpoint.  We'll be back to display
+     it in a moment.  */
+  if (stopped_by_watchpoint
+      && (HAVE_STEPPABLE_WATCHPOINT
+	  || gdbarch_have_nonsteppable_watchpoint (current_gdbarch)))
     {
       if (debug_infrun)
 	fprintf_unfiltered (gdb_stdlog, "infrun: STOPPED_BY_WATCHPOINT\n");
-      resume (1, 0);
-      prepare_to_wait (ecs);
-      return;
-    }
 
-  /* It is far more common to need to disable a watchpoint to step
-     the inferior over it.  FIXME.  What else might a debug
-     register or page protection watchpoint scheme need here?  */
-  if (gdbarch_have_nonsteppable_watchpoint (current_gdbarch)
-      && STOPPED_BY_WATCHPOINT (ecs->ws))
-    {
       /* At this point, we are stopped at an instruction which has
          attempted to write to a piece of memory under control of
          a watchpoint.  The instruction hasn't actually executed
@@ -1797,31 +1796,31 @@ handle_inferior_event (struct execution_
 
          In order to make watchpoints work `right', we really need
          to complete the memory write, and then evaluate the
-         watchpoint expression.  The following code does that by
-         removing the watchpoint (actually, all watchpoints and
-         breakpoints), single-stepping the target, re-inserting
-         watchpoints, and then falling through to let normal
-         single-step processing handle proceed.  Since this
-         includes evaluating watchpoints, things will come to a
-         stop in the correct manner.  */
+         watchpoint expression.  We do this by single-stepping the
+	 target.
 
-      if (debug_infrun)
-	fprintf_unfiltered (gdb_stdlog, "infrun: STOPPED_BY_WATCHPOINT\n");
-      remove_breakpoints ();
+	 It may not be necessary to disable the watchpoint to stop over
+	 it.  For example, the PA can (with some kernel cooperation)
+	 single step over a watchpoint without disabling the watchpoint.
+
+	 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.  */
+	 
+      if (!HAVE_STEPPABLE_WATCHPOINT)
+	remove_breakpoints ();
       registers_changed ();
       target_resume (ecs->ptid, 1, TARGET_SIGNAL_0);	/* Single step */
-
       ecs->waiton_ptid = ecs->ptid;
-      ecs->wp = &(ecs->ws);
-      ecs->infwait_state = infwait_nonstep_watch_state;
+      if (HAVE_STEPPABLE_WATCHPOINT)
+	ecs->infwait_state = infwait_step_watch_state;
+      else
+	ecs->infwait_state = infwait_nonstep_watch_state;
       prepare_to_wait (ecs);
       return;
     }
 
-  /* It may be possible to simply continue after a watchpoint.  */
-  if (HAVE_CONTINUABLE_WATCHPOINT)
-    stopped_by_watchpoint = STOPPED_BY_WATCHPOINT (ecs->ws);
-
   ecs->stop_func_start = 0;
   ecs->stop_func_end = 0;
   ecs->stop_func_name = 0;
@@ -1943,8 +1942,7 @@ handle_inferior_event (struct execution_
       else
 	{
 	  /* See if there is a breakpoint at the current PC.  */
-	  stop_bpstat = bpstat_stop_status (stop_pc, ecs->ptid,
-					    stopped_by_watchpoint);
+	  stop_bpstat = bpstat_stop_status (stop_pc, ecs->ptid);
 
 	  /* Following in case break condition called a
 	     function.  */
Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.266
diff -u -p -r1.266 remote.c
--- remote.c	23 Aug 2007 18:08:36 -0000	1.266
+++ remote.c	16 Sep 2007 18:05:29 -0000
@@ -5410,14 +5410,11 @@ remote_stopped_by_watchpoint (void)
     return remote_stopped_by_watchpoint_p;
 }
 
-extern int stepped_after_stopped_by_watchpoint;
-
 static int
 remote_stopped_data_address (struct target_ops *target, CORE_ADDR *addr_p)
 {
   int rc = 0;
-  if (remote_stopped_by_watchpoint ()
-      || stepped_after_stopped_by_watchpoint)
+  if (remote_stopped_by_watchpoint ())
     {
       *addr_p = remote_watch_data_address;
       rc = 1;
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.428
diff -u -p -r1.428 gdb.texinfo
--- doc/gdb.texinfo	2 Sep 2007 20:02:12 -0000	1.428
+++ doc/gdb.texinfo	16 Sep 2007 18:05:31 -0000
@@ -3290,20 +3290,13 @@ rerun the program, you will need to set 
 way of doing that would be to set a code breakpoint at the entry to the
 @code{main} function and when it breaks, set all the watchpoints.
 
-@quotation
 @cindex watchpoints and threads
 @cindex threads and watchpoints
-@emph{Warning:} In multi-thread programs, watchpoints have only limited
-usefulness.  With the current watchpoint implementation, @value{GDBN}
-can only watch the value of an expression @emph{in a single thread}.  If
-you are confident that the expression can only change due to the current
-thread's activity (and if you are also confident that no other thread
-can become current), then you can use watchpoints as usual.  However,
-@value{GDBN} may not notice when a non-current thread's activity changes
-the expression.
+In multi-threaded programs, watchpoints will detect changes to the
+watched expression from every thread.
 
-@c FIXME: this is almost identical to the previous paragraph.
-@emph{HP-UX Warning:} In multi-thread programs, software watchpoints
+@quotation
+@emph{Warning:} In multi-threaded programs, software watchpoints
 have only limited usefulness.  If @value{GDBN} creates a software
 watchpoint, it can only watch the value of an expression @emph{in a
 single thread}.  If you are confident that the expression can only
Index: doc/gdbint.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v
retrieving revision 1.266
diff -u -p -r1.266 gdbint.texinfo
--- doc/gdbint.texinfo	5 Jul 2007 12:22:32 -0000	1.266
+++ doc/gdbint.texinfo	16 Sep 2007 18:05:32 -0000
@@ -660,15 +660,21 @@ section is mostly irrelevant for softwar
 
 When the inferior stops, @value{GDBN} tries to establish, among other
 possible reasons, whether it stopped due to a watchpoint being hit.
-For a data-write watchpoint, it does so by evaluating, for each
-watchpoint, the expression whose value is being watched, and testing
-whether the watched value has changed.  For data-read and data-access
-watchpoints, @value{GDBN} needs the target to supply a primitive that
-returns the address of the data that was accessed or read (see the
-description of @code{target_stopped_data_address} below): if this
-primitive returns a valid address, @value{GDBN} infers that a
-watchpoint triggered if it watches an expression whose evaluation uses
-that address.
+It first uses @code{STOPPED_BY_WATCHPOINT} to see if any watchpoint
+was hit.  If not, all watchpoint checking is skipped.
+
+Then @value{GDBN} calls @code{target_stopped_data_address} exactly
+once.  This method returns the address of the watchpoint which
+triggered, if the target can determine it.  The returned address is
+compared to each watched memory address in each active watchpoint.
+Data-read and data-access watchpoints rely on this; if the triggered
+address is not available, read and access watchpoints will not work.
+If the triggered address is available, @value{GDBN} will only check
+data-write watchpoints which match the triggered address.  If it is
+not available, @value{GDBN} will consider all data-write watchpoints.
+For each data-write watchpoint that @value{GDBN} considers, it does so
+by evaluating the expression whose value is being watched, and testing
+whether the watched value has changed.
 
 @value{GDBN} uses several macros and primitives to support hardware
 watchpoints:
@@ -721,26 +727,40 @@ These two macros should return 0 for suc
 @item target_stopped_data_address (@var{addr_p})
 If the inferior has some watchpoint that triggered, place the address
 associated with the watchpoint at the location pointed to by
-@var{addr_p} and return non-zero.  Otherwise, return zero.  Note that
-this primitive is used by @value{GDBN} only on targets that support
-data-read or data-access type watchpoints, so targets that have
-support only for data-write watchpoints need not implement these
-primitives.
+@var{addr_p} and return non-zero.  Otherwise, return zero.  This
+is required for data-read and data-access watchpoints.  It is
+not required for data-write watchpoints, but @value{GDBN} uses
+it to improve handling of those also.
+
+@value{GDBN} will only call this method once per watchpoint stop,
+immediately after calling @code{STOPPED_BY_WATCHPOINT}.  If the
+target's watchpoint indication is sticky, i.e., stays set after
+resuming, this method should clear it.  For instance, the x86 debug
+control register has sticky triggered flags.
 
 @findex HAVE_STEPPABLE_WATCHPOINT
 @item HAVE_STEPPABLE_WATCHPOINT
 If defined to a non-zero value, it is not necessary to disable a
-watchpoint to step over it.
+watchpoint to step over it.    Like @code{gdbarch_have_nonsteppable_watchpoint},
+this is usually set when watchpoints trigger at the instruction
+which will perform an interesting read or write.  It should be
+set if there is a temporary disable bit which allows the processor
+to step over the interesting instruction without raising the
+watchpoint exception again.
 
 @findex gdbarch_have_nonsteppable_watchpoint 
 @item int gdbarch_have_nonsteppable_watchpoint (@var{gdbarch})
 If it returns a non-zero value, @value{GDBN} should disable a
-watchpoint to step the inferior over it.
+watchpoint to step the inferior over it.  This is usually set when
+watchpoints trigger at the instruction which will perform an
+interesting read or write.
 
 @findex HAVE_CONTINUABLE_WATCHPOINT
 @item HAVE_CONTINUABLE_WATCHPOINT
 If defined to a non-zero value, it is possible to continue the
-inferior after a watchpoint has been hit.
+inferior after a watchpoint has been hit.  This is usually set
+when watchpoints trigger at the instruction following an interesting
+read or write.
 
 @findex CANNOT_STEP_HW_WATCHPOINTS
 @item CANNOT_STEP_HW_WATCHPOINTS
@@ -763,6 +783,27 @@ determine for sure whether the inferior 
 it could return non-zero ``just in case''.
 @end table
 
+@subsection Watchpoints and Threads
+@cindex watchpoints, with threads
+
+@value{GDBN} only supports process-wide watchpoints.  If the target
+supports threads and per-thread debug registers, it should set the
+per-thread debug registers for all threads to the same value.  On
+@sc{gnu}/Linux native targets, this is accomplished by using
+@code{ALL_LWPS} in @code{target_insert_watchpoint} and
+@code{target_remove_watchpoint} and by using @code{linux_set_new_thread}
+to register a handler for newly created threads.
+
+@value{GDBN}'s @sc{gnu}/Linux support only reports a single event
+at a time, although multiple events can trigger simultaneously for
+multi-threaded programs.  When multiple events occur, @file{linux-nat.c}
+queues subsequent events and returns them the next time the program
+is resumed.  This means that @code{STOPPED_BY_WATCHPOINT} and
+@code{target_stopped_data_address} only need to consult the current
+thread's state---the thread indicated by @code{inferior_ptid}.  If
+two threads have hit watchpoints simultaneously, those routines
+will be called a second time for the second thread.
+
 @subsection x86 Watchpoints
 @cindex x86 debug registers
 @cindex watchpoints, on x86
Index: testsuite/gdb.threads/watchthreads.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.threads/watchthreads.c,v
retrieving revision 1.3
diff -u -p -r1.3 watchthreads.c
--- testsuite/gdb.threads/watchthreads.c	23 Aug 2007 18:08:50 -0000	1.3
+++ testsuite/gdb.threads/watchthreads.c	16 Sep 2007 18:05:32 -0000
@@ -56,7 +56,7 @@ void *thread_function(void *arg) {
     /* Don't run forever.  Run just short of it :)  */
     while (*myp > 0)
       {
-	(*myp) ++;  /* Loop increment.  */
+	(*myp) ++; usleep (1);  /* Loop increment.  */
       }
 
     pthread_exit(NULL);
Index: testsuite/gdb.threads/watchthreads.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.threads/watchthreads.exp,v
retrieving revision 1.4
diff -u -p -r1.4 watchthreads.exp
--- testsuite/gdb.threads/watchthreads.exp	23 Aug 2007 18:14:19 -0000	1.4
+++ testsuite/gdb.threads/watchthreads.exp	16 Sep 2007 18:05:32 -0000
@@ -30,6 +30,10 @@ if [target_info exists gdb,no_hardware_w
     return 0;
 }
 
+proc target_no_stopped_data { } {
+    return [istarget s390*-*-*]
+}
+
 set testfile "watchthreads"
 set srcfile ${testfile}.c
 set binfile ${objdir}/${subdir}/${testfile}
@@ -61,20 +65,56 @@ gdb_test "watch args\[1\]" "Hardware wat
 
 set init_line [expr [gdb_get_line_number "Init value"]+1]
 set inc_line [gdb_get_line_number "Loop increment"]
+set main_loc "main \\\(\\\) at .*watchthreads.c:$init_line"
+set thread0_loc "thread_function \\\(arg=0x0\\\) at .*watchthreads.c:$inc_line"
+set thread1_loc "thread_function \\\(arg=0x1\\\) at .*watchthreads.c:$inc_line"
 
 # Loop and continue to allow both watchpoints to be triggered.
 for {set i 0} {$i < 30} {incr i} {
+  set test_flag_0 0
+  set test_flag_1 0
   set test_flag 0
   gdb_test_multiple "continue" "threaded watch loop" {
-    -re "Hardware watchpoint 2: args\\\[0\\\].*Old value = 0.*New value = 1.*main \\\(\\\) at .*watchthreads.c:$init_line.*$gdb_prompt $"
-       { set args_0 1; set test_flag 1 }
-    -re "Hardware watchpoint 3: args\\\[1\\\].*Old value = 0.*New value = 1.*main \\\(\\\) at .*watchthreads.c:$init_line.*$gdb_prompt $"
-       { set args_1 1; set test_flag 1 }
-    -re "Hardware watchpoint 2: args\\\[0\\\].*Old value = $args_0.*New value = [expr $args_0+1].*in thread_function \\\(arg=0x0\\\) at .*watchthreads.c:$inc_line.*$gdb_prompt $"
-       { set args_0 [expr $args_0+1]; set test_flag 1 }
-    -re "Hardware watchpoint 3: args\\\[1\\\].*Old value = $args_1.*New value = [expr $args_1+1].*in thread_function \\\(arg=0x1\\\) at .*watchthreads.c:$inc_line.*$gdb_prompt $"
-       { set args_1 [expr $args_1+1]; set test_flag 1 }
+    -re "(.*Hardware watchpoint.*)$gdb_prompt $" {
+	# At least one hardware watchpoint was hit.  Check if both were.
+	set string $expect_out(1,string)
+
+	if [regexp "Hardware watchpoint 2: args\\\[0\\\]\[^\r\]*\r\[^\r\]*\r\[^\r\]*Old value = $args_0\[^\r\]*\r\[^\r\]*New value = [expr $args_0+1]\r" $string] {
+	    incr args_0
+	    incr test_flag_0
+	}
+	if [regexp "Hardware watchpoint 3: args\\\[1\\\]\[^\r\]*\r\[^\r\]*\r\[^\r\]*Old value = $args_1\[^\r\]*\r\[^\r\]*New value = [expr $args_1+1]\r" $string] {
+	    incr args_1
+	    incr test_flag_1
+	}
+
+	set expected_loc "bogus location"
+	if { $test_flag_0 == 1 && $test_flag_1 == 0 && $args_0 == 1 } {
+	    set expected_loc $main_loc
+	} elseif { $test_flag_0 == 0 && $test_flag_1 == 1 && $args_1 == 1 } {
+	    set expected_loc $main_loc
+	} elseif { $test_flag_0 == 1 && $test_flag_1 == 0 } {
+	    set expected_loc $thread0_loc
+	} elseif { $test_flag_0 == 0 && $test_flag_1 == 1 } {
+	    set expected_loc $thread1_loc
+	} elseif { $test_flag_0 + $test_flag_1 == 2 } {
+	    # On S/390, or any other system which can not report the
+	    # stopped data address, it is OK to report two watchpoints
+	    # at once in this test.  Make sure the reported location
+	    # corresponds to at least one of the watchpoints (and not,
+	    # e.g., __nptl_create_event).  On other systems, we should
+	    # report the two watchpoints serially.
+	    if { [target_no_stopped_data] } {
+		set expected_loc "($main_loc|$thread0_loc|$thread1_loc)"
+	    }
+	}
+
+	if [ regexp "$expected_loc" $string ] {
+	    set test_flag 1
+	}
+    }
   }
+
   # If we fail above, don't bother continuing loop
   if { $test_flag == 0 } {
     set i 30;
@@ -84,6 +124,8 @@ for {set i 0} {$i < 30} {incr i} {
 # Print success message if loop succeeded.
 if { $test_flag == 1 } {
   pass "threaded watch loop"
+} else {
+  fail "threaded watch loop"
 }
 
 # Verify that we hit first watchpoint in main thread.
@@ -120,7 +162,14 @@ if { $args_1 > 1 } {
 
 # Verify that all watchpoint hits are accounted for.
 set message "combination of threaded watchpoints = 30"
-if { [expr $args_0+$args_1] == 30 } {
+if { [target_no_stopped_data] } {
+    # See above.  If we allow two watchpoints to be hit at once, we
+    # may have more than 30 hits total.
+    set result [expr $args_0 + $args_1 >= 30]
+} else {
+    set result [expr $args_0 + $args_1 == 30]
+}
+if { $result } {
   pass $message 
 } else {
   fail $message 


^ permalink raw reply	[flat|nested] 27+ messages in thread
end of thread, other threads:[~2008-05-01 19:50 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-09-16 18:40 [rfc, rfa/doc] Multi-threaded watchpoint improvements Daniel Jacobowitz
2007-09-22  9:03 ` Eli Zaretskii
2007-09-22 14:04   ` Daniel Jacobowitz
2007-09-22 14:13     ` Eli Zaretskii
2007-09-22 15:37       ` Daniel Jacobowitz
2007-09-22 15:46         ` Eli Zaretskii
2007-09-22 18:28           ` Daniel Jacobowitz
2007-10-01  0:20 ` Daniel Jacobowitz
2007-10-24 14:59   ` Luis Machado
2007-10-24 16:00     ` Daniel Jacobowitz
2007-10-24 16:42       ` Luis Machado
2007-10-24 16:51         ` Daniel Jacobowitz
2007-10-24 18:08           ` Luis Machado
2007-10-24 18:20             ` Daniel Jacobowitz
2008-04-16 22:57   ` Andreas Schwab
2008-04-16 23:05     ` Daniel Jacobowitz
2008-04-16 23:18       ` Andreas Schwab
2008-04-17  0:25         ` Daniel Jacobowitz
2008-04-17  9:52           ` Andreas Schwab
2008-04-17  9:59             ` Andreas Schwab
2008-04-17 10:17           ` Andreas Schwab
2008-04-17 14:53             ` Daniel Jacobowitz
2008-04-20  2:35               ` Andreas Schwab
2008-04-23 11:55                 ` Andreas Schwab
2008-04-23 19:46                   ` Eli Zaretskii
2008-05-01 19:30                   ` Daniel Jacobowitz
2008-05-01 19:50       ` Luis Machado

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox