This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Make watchpoints behave in always-inserted mode too.


This patch makes watchpoints work in always-inserted mode too,
so that watchpoints in non-stop mode aren't missed.

The main ideas here are:

- Teaching update_global_location_list to detect duplicate
  watchpoint locations.  A watchpoint location is duplicate
  of another only if its type (watchpoint, access, read),
  address and length fields match.

- Get rid of the explicit remove/insert breakpoints
  sequence done by bpstat_stop_status.  With the duplicate
  watchpoint locations infrastructure in place, we can just
  use update_watchpoints and update_global_location_list
  directly.

- Have watch_command_1 not create an initial location
  for the watchpoint.  This (useless) location was always
  created with address 0.  update_watchpoint is responsible for
  recreating the real watchpoint locations, but, in
  always inserted mode, it could happen that we'd manage
  to try to insert a watchpoint at 0 in the target, before
  any update_watchpoint call was reached...

- Addressing the get_frame_program_space issue Daniel
  pointed out last week, by avoiding creating locations
  if the target hasn't execution yet.  They're not needed
  otherwise.  This isn't an ideal fix, but has no
  downsides, and gets the ball rolling.  I'll need to take
  a stab at making watchpoints actualy work properly with
  multi inferiors on linux before coming up with a
  better fix.  This needed addressing, due to the new
  call to update_watchpoints at the end of watch_command_1,
  so that we can continue creating watchpoints before the
  target has execution.



Comments?

This applies on top of the always-insert regression fix at
<http://sourceware.org/ml/gdb-patches/2009-11/msg00428.html>

No regressions on x86_64-linux native/gdbserver.


-- 
Pedro Alves

2009-11-20  Pedro Alves  <pedro@codesourcery.com>

	* breakpoint.c (update_watchpoint): Skip creating locations and
	reading the selected frame if there's no execution.
	(bpstat_stop_status): Use is_hardware_watchpoint.  If not
	stopping, update watchpoints and the global location list, instead
	of removing and inserting all breakpoints.
	(breakpoint_address_is_meaningful): Hardware watchpoints also have
	a meaningful target address.
	(watchpoint_locations_match): New.
	(breakpoint_locations_match): New.
	(watch_command_1): Create the watchpoint breakpoint without any
	location initially.  Use update_watchpoint to create the
	watchpoint locations.
	(update_global_location_list): Use breakpoint_locations_match, so
	watchpoint locations are handled too. Also detect duplicate
	watchpoint locations.

---
 gdb/breakpoint.c |  133 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 86 insertions(+), 47 deletions(-)

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2009-11-20 01:02:18.000000000 +0000
+++ src/gdb/breakpoint.c	2009-11-20 01:45:30.000000000 +0000
@@ -1019,7 +1019,6 @@ update_watchpoint (struct breakpoint *b,
   struct bp_location *loc;
   int frame_saved;
   bpstat bs;
-  struct program_space *frame_pspace;
 
   /* If this is a local watchpoint, we only want to check if the
      watchpoint frame is in scope if the current thread is the thread
@@ -1058,8 +1057,6 @@ update_watchpoint (struct breakpoint *b,
 	select_frame (fi);
     }
 
-  frame_pspace = get_frame_program_space (get_selected_frame (NULL));
-
   if (within_current_scope && reparse)
     {
       char *s;
@@ -1084,9 +1081,16 @@ update_watchpoint (struct breakpoint *b,
      don't try to insert watchpoint.  We don't automatically delete
      such watchpoint, though, since failure to parse expression
      is different from out-of-scope watchpoint.  */
-  if (within_current_scope && b->exp)
+  if ( !target_has_execution)
+    {
+      /* Without execution, memory can't change.  No use to try and
+	 set watchpoint locations.  The watchpoint will be reset when
+	 the target gains execution, through breakpoint_re_set.  */
+    }
+  else if (within_current_scope && b->exp)
     {
       struct value *val_chain, *v, *result, *next;
+      struct program_space *frame_pspace;
 
       fetch_watchpoint_value (b->exp, &v, &result, &val_chain);
 
@@ -1125,6 +1129,8 @@ update_watchpoint (struct breakpoint *b,
 	      }
 	  }
 
+      frame_pspace = get_frame_program_space (get_selected_frame (NULL));
+
       /* Look at each value on the value chain.  */
       for (v = val_chain; v; v = next)
 	{
@@ -3575,22 +3581,17 @@ bpstat_stop_status (struct address_space
     for (bs = root_bs->next; bs != NULL; bs = bs->next)
       if (!bs->stop
 	  && bs->breakpoint_at->owner
-	  && (bs->breakpoint_at->owner->type == bp_hardware_watchpoint
-	      || bs->breakpoint_at->owner->type == bp_read_watchpoint
-	      || bs->breakpoint_at->owner->type == bp_access_watchpoint))
-	{
-	  /* remove/insert can invalidate bs->breakpoint_at, if this
-	     location is no longer used by the watchpoint.  Prevent
-	     further code from trying to use it.  */
+	  && is_hardware_watchpoint (bs->breakpoint_at->owner))
+	{
+	  update_watchpoint (bs->breakpoint_at->owner, 0 /* don't reparse. */);
+	  /* Updating watchpoints invalidates bs->breakpoint_at.
+	     Prevent further code from trying to use it.  */
 	  bs->breakpoint_at = NULL;
 	  need_remove_insert = 1;
 	}
 
   if (need_remove_insert)
-    {
-      remove_breakpoints ();
-      insert_breakpoints ();
-    }
+    update_global_location_list (1);
 
   return root_bs->next;
 }
@@ -4570,21 +4571,28 @@ set_default_breakpoint (int valid, struc
    these types to be a duplicate of an actual breakpoint at address zero:
 
       bp_watchpoint
-      bp_hardware_watchpoint
-      bp_read_watchpoint
-      bp_access_watchpoint
-      bp_catchpoint */
+      bp_catchpoint
+
+*/
 
 static int
 breakpoint_address_is_meaningful (struct breakpoint *bpt)
 {
   enum bptype type = bpt->type;
 
-  return (type != bp_watchpoint
-	  && type != bp_hardware_watchpoint
-	  && type != bp_read_watchpoint
-	  && type != bp_access_watchpoint
-	  && type != bp_catchpoint);
+  return (type != bp_watchpoint && type != bp_catchpoint);
+}
+
+/* Assuming LOC1 and LOC2's owners are hardware watchpoints, returns
+   true if LOC1 and LOC2 represent the same watchpoint location.  */
+
+static int
+watchpoint_locations_match (struct bp_location *loc1, struct bp_location *loc2)
+{
+  return (loc1->owner->type == loc2->owner->type
+	  && loc1->pspace->aspace == loc2->pspace->aspace
+	  && loc1->address == loc2->address
+	  && loc1->length == loc2->length);
 }
 
 /* Returns true if {ASPACE1,ADDR1} and {ASPACE2,ADDR2} represent the
@@ -4601,6 +4609,22 @@ breakpoint_address_match (struct address
 	  && addr1 == addr2);
 }
 
+
+static int
+breakpoint_locations_match (struct bp_location *loc1, struct bp_location *loc2)
+{
+  int hw_point1 = is_hardware_watchpoint (loc1->owner);
+  int hw_point2 = is_hardware_watchpoint (loc2->owner);
+
+  if (hw_point1 != hw_point2)
+    return 0;
+  else if (hw_point1)
+    return watchpoint_locations_match (loc1, loc2);
+  else
+    return breakpoint_address_match (loc1->pspace->aspace, loc1->address,
+				     loc2->pspace->aspace, loc2->address);
+}
+
 static void
 breakpoint_adjustment_warning (CORE_ADDR from_addr, CORE_ADDR to_addr,
                                int bnum, int have_bnum)
@@ -6982,7 +7006,6 @@ watch_command_1 (char *arg, int accessfl
 {
   struct gdbarch *gdbarch = get_current_arch ();
   struct breakpoint *b, *scope_breakpoint = NULL;
-  struct symtab_and_line sal;
   struct expression *exp;
   struct block *exp_valid_block;
   struct value *val, *mark;
@@ -6993,14 +7016,11 @@ watch_command_1 (char *arg, int accessfl
   int toklen;
   char *cond_start = NULL;
   char *cond_end = NULL;
-  struct expression *cond = NULL;
   int i, other_type_used, target_resources_ok = 0;
   enum bptype bp_type;
   int mem_cnt = 0;
   int thread = -1;
 
-  init_sal (&sal);		/* initialize to zeroes */
-
   /* Make sure that we actually have parameters to parse.  */
   if (arg != NULL && arg[0] != '\0')
     {
@@ -7062,8 +7082,6 @@ watch_command_1 (char *arg, int accessfl
         }
     }
 
-  sal.pspace = current_program_space;
-
   /* Parse the rest of the arguments.  */
   innermost_block = NULL;
   exp_start = arg;
@@ -7092,8 +7110,11 @@ watch_command_1 (char *arg, int accessfl
   toklen = end_tok - tok;
   if (toklen >= 1 && strncmp (tok, "if", toklen) == 0)
     {
+      struct expression *cond;
+
       tok = cond_start = end_tok + 1;
       cond = parse_exp_1 (&tok, 0, 0);
+      xfree (cond);
       cond_end = tok;
     }
   if (*tok)
@@ -7163,7 +7184,7 @@ watch_command_1 (char *arg, int accessfl
     }
 
   /* Now set up the breakpoint.  */
-  b = set_raw_breakpoint (gdbarch, sal, bp_type);
+  b = set_raw_breakpoint_without_location (NULL, bp_type);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
   b->thread = thread;
@@ -7173,7 +7194,6 @@ watch_command_1 (char *arg, int accessfl
   b->exp_string = savestring (exp_start, exp_end - exp_start);
   b->val = val;
   b->val_valid = 1;
-  b->loc->cond = cond;
   if (cond_start)
     b->cond_string = savestring (cond_start, cond_end - cond_start);
   else
@@ -7199,6 +7219,11 @@ watch_command_1 (char *arg, int accessfl
     }
 
   value_free_to_mark (mark);
+
+  /* Finally update the new watchpoint.  This creates the locations
+     that should be inserted.  */
+  update_watchpoint (b, 1);
+
   mention (b);
   update_global_location_list (1);
 }
@@ -8208,7 +8233,10 @@ update_global_location_list (int should_
 
   /* The first bp_location being the only one non-DUPLICATE for the current run
      of the same ADDRESS.  */
-  struct bp_location *loc_first;
+  struct bp_location *bp_loc_first;
+  struct bp_location *wp_loc_first;
+  struct bp_location *awp_loc_first;
+  struct bp_location *rwp_loc_first;
 
   /* Saved former bp_location array which we compare against the newly built
      bp_location from the current state of ALL_BREAKPOINTS.  */
@@ -8307,10 +8335,7 @@ update_global_location_list (int should_
 		    {
 		      struct bp_location *loc2 = *loc2p;
 
-		      if (breakpoint_address_match (loc2->pspace->aspace,
-						    loc2->address,
-						    old_loc->pspace->aspace,
-						    old_loc->address))
+		      if (breakpoint_locations_match (loc2, old_loc))
 			{
 			  /* For the sake of should_be_inserted.
 			     Duplicates check below will fix up this later.  */
@@ -8377,12 +8402,19 @@ update_global_location_list (int should_
      This is so that the bpt instruction is only inserted once.
      If we have a permanent breakpoint at the same place as BPT, make
      that one the official one, and the rest as duplicates.  Permanent
-     breakpoints are sorted first for the same address.  */
+     breakpoints are sorted first for the same address.
+
+     Do the same for hardware watchpoints, but also considering the
+     watchpoint type (regular/access/read) and length.  */
 
-  loc_first = NULL;
+  bp_loc_first = NULL;
+  wp_loc_first = NULL;
+  awp_loc_first = NULL;
+  rwp_loc_first = NULL;
   ALL_BP_LOCATIONS (loc, locp)
     {
       struct breakpoint *b = loc->owner;
+      struct bp_location **loc_first;
 
       if (b->enable_state == bp_disabled
 	  || b->enable_state == bp_call_disabled
@@ -8398,21 +8430,28 @@ update_global_location_list (int should_
 			_("allegedly permanent breakpoint is not "
 			"actually inserted"));
 
-      if (loc_first == NULL
-	  || (overlay_debugging && loc->section != loc_first->section)
-	  || !breakpoint_address_match (loc->pspace->aspace, loc->address,
-					loc_first->pspace->aspace,
-					loc_first->address))
+      if (b->type == bp_hardware_watchpoint)
+	loc_first = &wp_loc_first;
+      else if (b->type == bp_read_watchpoint)
+	loc_first = &rwp_loc_first;
+      else if (b->type == bp_access_watchpoint)
+	loc_first = &awp_loc_first;
+      else
+	loc_first = &bp_loc_first;
+
+      if (*loc_first == NULL
+	  || (overlay_debugging && loc->section != (*loc_first)->section)
+	  || !breakpoint_locations_match (loc, *loc_first))
 	{
-	  loc_first = loc;
+	  *loc_first = loc;
 	  loc->duplicate = 0;
 	  continue;
 	}
 
       loc->duplicate = 1;
 
-      if (loc_first->owner->enable_state == bp_permanent && loc->inserted
-          && b->enable_state != bp_permanent)
+      if ((*loc_first)->owner->enable_state == bp_permanent && loc->inserted
+	  && b->enable_state != bp_permanent)
 	internal_error (__FILE__, __LINE__,
 			_("another breakpoint was inserted on top of "
 			"a permanent breakpoint"));


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]