[patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Thiago Jung Bauermann]

Hi,

This patch adds support for hardware ranged watchpoints, without the
watch-range command. It makes GDB use hardware ranged watchpoints
automatically when the user asks to watch an array or struct and there
are enough hardware watchpoints available.

The code is the same that was reviewed before, , except for fixing the
GNU style regarding whitespace in
ppc_linux_{insert,remove}_ranged_watchpoint.

The NEWS entry is the same that was reviewed before. The manual has the
following new wording in the second half of the paragraph (the first
half is the same):

+You can create an artificial array to watch an arbitrary memory
+region using one of the following commands (@pxref{Expressions}):
+
+@smallexample
+(@value{GDBP}) watch *((char *) @var{ADDRESS})@@@var{LENGTH}
+(@value{GDBP}) watch @{char[@var{LENGTH}]@} @var{ADDRESS}
+@end smallexample

Ok?
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2010-11-24  Sergio Durigan Junior  <sergiodj@linux.vnet.ibm.com>
	    Thiago Jung Bauermann  <bauerman@br.ibm.com>

	Implement support for PowerPC BookE ranged watchpoints.

gdb/
	*NEWS: Mention ranged watchpoint support.
	* breakpoint.c
	(insert_ranged_watchpoint, remove_ranged_watchpoint)
	(extra_resources_needed_ranged_watchpoint)
	(print_one_detail_ranged_watchpoint, print_mention_ranged_watchpoint)
	(print_recreate_ranged_watchpoint): New functions.
	(ranged_watchpoint_breakpoint_ops): New structure.
	(watch_command_1): Check whether a ranged hardware watchpoint
	can be used. Set b->ops according to the type of hardware watchpoint
	being created.
	* breakpoint.h 
	(enum hw_point_flag) <HW_POINT_RANGED_WATCH>: New element.
	* ppc-linux-nat.c (ppc_linux_can_use_special_hw_point): Handle
	HW_POINT_RANGED_WATCH.
	(ppc_linux_region_ok_for_hw_watchpoint): Always handle regions when
	ranged watchpoints are available.
	(ppc_linux_insert_ranged_watchpoint)
	(ppc_linux_remove_ranged_watchpoint): New functions.
	(ppc_linux_hw_point_extra_slot_count): Handle HW_POINT_RANGED_WATCH.
	(_initialize_ppc_linux_nat): Initialize to_insert_ranged_watchpoint
	and to_remove_ranged_watchpoint.
	* target.c (update_current_target): Mention
	to_insert_ranged_watchpoint and to_remove_ranged_watchpoint,.
	(target_insert_ranged_watchpoint)
	(target_remove_ranged_watchpoint): New functions.
	* target.h (struct target_ops) <to_insert_ranged_watchpoint>,
	<to_remove_ranged_watchpoint>: New callbacks.
	(target_insert_ranged_watchpoint)
	(target_remove_ranged_watchpoint): Add prototypes.
	* ui-out.c (ui_out_field_range_core_addr): New function.
	* ui-out.h (ui_out_field_range_core_addr): Declare.

gdb/doc/
	* gdb.texinfo (PowerPC Embedded): Document ranged watchpoints.


diff --git a/gdb/NEWS b/gdb/NEWS
index 3fa763c..0ac97b5 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -93,6 +93,12 @@
   by the inferior against the watchpoint address.  See the "PowerPC Embedded"
   section in the user manual for more details.
 
+* GDB now supports ranged watchpoints, which stop the inferior when it
+  accesses any address within a specified memory range.  The watchpoint
+  is hardware-accelerated on some targets (currently only when locally
+  debugging programs on PowerPC BookE processors running a Linux
+  kernel version 2.6.34 or later).
+
 * New features in the GDB remote stub, GDBserver
 
   ** GDBserver is now supported on PowerPC LynxOS (versions 4.x and 5.x),
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e9d2661..5f4d472 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -8159,6 +8159,103 @@ static struct breakpoint_ops watchpoint_breakpoint_ops =
 };
 
 /* Implement the "insert" breakpoint_ops method for
+   ranged hardware watchpoints.  */
+
+static int
+insert_ranged_watchpoint (struct bp_location *bpt)
+{
+  return target_insert_ranged_watchpoint (bpt->address,
+					  bpt->length,
+					  bpt->watchpoint_type);
+}
+
+/* Implement the "remove" breakpoint_ops method for
+   ranged hardware watchpoints.  */
+
+static int
+remove_ranged_watchpoint (struct bp_location *bpt)
+{
+  return target_remove_ranged_watchpoint (bpt->address, bpt->length,
+					  bpt->watchpoint_type);
+}
+
+/* Implement the "extra_resources_needed" breakpoint_ops method for
+   ranged hardware watchpoints.  */
+
+static int
+extra_resources_needed_ranged_watchpoint (const struct breakpoint *b)
+{
+  return target_hw_point_extra_slot_count (HW_POINT_RANGED_WATCH);
+}
+
+/* Implement the "print_one_detail" breakpoint_ops method for
+   ranged hardware watchpoints.  */
+
+static void
+print_one_detail_ranged_watchpoint (const struct breakpoint *b, struct ui_out *uiout)
+{
+  gdb_assert (b->loc);
+
+  ui_out_text (uiout, "\tmemory range: ");
+  ui_out_field_range_core_addr (uiout, "addr", b->loc->gdbarch,
+				b->loc->address, b->loc->length);
+  ui_out_text (uiout, "\n");
+}
+
+/* Implement the "print_mention" breakpoint_ops method for
+   ranged hardware watchpoints.  */
+
+static void
+print_mention_ranged_watchpoint (struct breakpoint *b)
+{
+  struct cleanup *ui_out_chain;
+
+  switch (b->type)
+    {
+    case bp_watchpoint:
+      ui_out_text (uiout, "Ranged watchpoint ");
+      ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "wpt");
+      break;
+    case bp_hardware_watchpoint:
+      ui_out_text (uiout, "Ranged hardware watchpoint ");
+      ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "wpt");
+      break;
+    case bp_read_watchpoint:
+      ui_out_text (uiout, "Ranged hardware read watchpoint ");
+      ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "hw-rwpt");
+      break;
+    case bp_access_watchpoint:
+      ui_out_text (uiout, "Ranged hardware access (read/write) watchpoint ");
+      ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "hw-awpt");
+      break;
+    default:
+      internal_error (__FILE__, __LINE__,
+		      _("Invalid hardware watchpoint type."));
+    }
+
+  ui_out_field_int (uiout, "number", b->number);
+  ui_out_text (uiout, ": ");
+  ui_out_field_string (uiout, "exp", b->exp_string);
+  do_cleanups (ui_out_chain);
+}
+
+/* The breakpoint_ops structure to be used in ranged hardware watchpoints.  */
+
+static struct breakpoint_ops ranged_watchpoint_breakpoint_ops =
+{
+  insert_ranged_watchpoint,
+  remove_ranged_watchpoint,
+  NULL, /* breakpoint_hit */
+  extra_resources_needed_ranged_watchpoint,
+  NULL, /* works_in_software_mode */
+  NULL, /* print_it */
+  NULL, /* print_one */
+  print_one_detail_ranged_watchpoint,
+  print_mention_ranged_watchpoint,
+  NULL  /* print_recreate */
+};
+
+/* Implement the "insert" breakpoint_ops method for
    masked hardware watchpoints.  */
 
 static int
@@ -8376,6 +8473,9 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
      the hardware watchpoint.  */
   int use_mask = 0;
   CORE_ADDR hw_wp_mask = 0;
+  /* Whether we are watching an array or struct and hence we will
+     try to use ranged hardware watchpoints, if available.  */
+  int use_ranged = 0;
 
   /* Make sure that we actually have parameters to parse.  */
   if (arg != NULL && arg[0] != '\0')
@@ -8538,10 +8638,21 @@ This target does not support the usage of masks with hardware watchpoints."));
     error (_("Expression cannot be implemented with read/access watchpoint."));
   if (mem_cnt != 0)
     {
+      struct type *vtype = check_typedef (value_type (val));
+
       /* If we are going to use masks, then we may need more
 	 slots in order to use the hardware watchpoint.  */
       if (use_mask)
 	mem_cnt += target_hw_point_extra_slot_count (HW_POINT_MASKED_WATCH);
+      /* If we are watching an array or struct, we may be able to do it using
+         a ranged watchpoint.  */
+      else if ((TYPE_CODE (vtype) == TYPE_CODE_STRUCT
+		|| TYPE_CODE (vtype) == TYPE_CODE_ARRAY)
+	       && target_can_use_special_hw_point (HW_POINT_RANGED_WATCH))
+	{
+	  use_ranged = 1;
+	  mem_cnt += target_hw_point_extra_slot_count (HW_POINT_RANGED_WATCH);
+	}
 
       i = hw_watchpoint_used_count (bp_type, &other_type_used);
       target_resources_ok = 
@@ -8563,9 +8674,12 @@ This target does not support the usage of masks with hardware watchpoints."));
 Cannot use masks without enough available hardware watchpoints (need %d)."),
 	       mem_cnt);
       else
-	/* Change the type of breakpoint to an ordinary watchpoint if a
-	   hardware watchpoint could not be set.  */
-	bp_type = bp_watchpoint;
+	{
+	  /* Change the type of breakpoint to an ordinary watchpoint if a
+	     hardware watchpoint could not be set.  */
+	  bp_type = bp_watchpoint;
+	  use_ranged = 0;
+	}
     }
 
   frame = block_innermost_frame (exp_valid_block);
@@ -8640,6 +8754,8 @@ Cannot use masks without enough available hardware watchpoints (need %d)."),
       b->hw_wp_mask = hw_wp_mask;
       b->ops = &masked_watchpoint_breakpoint_ops;
     }
+  else if (use_ranged)
+    b->ops = &ranged_watchpoint_breakpoint_ops;
   else
     b->ops = &watchpoint_breakpoint_ops;
 
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index f7faf17..6d9fa07 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -433,6 +433,7 @@ struct counted_command_line;
 /* Special types of hardware breakpoints/watchpoints.  */
 enum hw_point_flag
   {
+    HW_POINT_RANGED_WATCH,	/* Hardware ranged watchpoint.  */
     HW_POINT_MASKED_WATCH	/* Hardware masked watchpoint.  */
   };
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index b7f26c6..97ec8d5 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -18702,7 +18702,7 @@ The DVC register will be automatically used whenever @value{GDBN} detects
 such pattern in a condition expression.  This feature is available in native
 @value{GDBN} running on a Linux kernel version 2.6.34 or newer.
 
-PowerPC embedded processors support masked watchpoints.
+PowerPC embedded processors support masked watchpoints and ranged watchpoints.
 
 A @dfn{masked watchpoint} specifies a mask in addition to an address
 to watch.  The mask specifies that some bits of an address (the bits
@@ -18719,6 +18719,17 @@ the @code{watch} command (@pxref{Set Watchpoints}), as in:
 (@value{GDBP}) watch *0xdeadbeef mask 0xffffff00
 @end smallexample
 
+A @dfn{ranged watchpoint} watches a contiguous range of addresses.
+@value{GDBN} automatically creates a ranged watchpoint when asked to watch
+an array or struct of known size and there are enough hardware registers
+available.  You can create an artificial array to watch an arbitrary memory
+region using one of the following commands (@pxref{Expressions}):
+
+@smallexample
+(@value{GDBP}) watch *((char *) @var{ADDRESS})@@@var{LENGTH}
+(@value{GDBP}) watch @{char[@var{LENGTH}]@} @var{ADDRESS}
+@end smallexample
+
 @value{GDBN} provides the following PowerPC-specific commands:
 
 @table @code
diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 773a507..3b6a9ca 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1486,11 +1486,18 @@ ppc_linux_can_use_special_hw_point (struct target_ops *ops,
   if (!have_ptrace_booke_interface ())
     return 0;
 
-  if (flag != HW_POINT_MASKED_WATCH)
-    internal_error (__FILE__, __LINE__,
-		    _("Unknown hardware breakpoint/watchpoint type."));
+  features = booke_debug_info.features;
 
-  return booke_debug_info.features & PPC_DEBUG_FEATURE_DATA_BP_MASK;
+  switch (flag)
+    {
+      case HW_POINT_RANGED_WATCH:
+	return features & PPC_DEBUG_FEATURE_DATA_BP_RANGE;
+      case HW_POINT_MASKED_WATCH:
+	return features & PPC_DEBUG_FEATURE_DATA_BP_MASK;
+      default:
+	internal_error (__FILE__, __LINE__,
+			_("Unknown hardware breakpoint/watchpoint type."));
+    }
 }
 
 static int
@@ -1505,9 +1512,16 @@ ppc_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
      to determine the hardcoded watchable region for watchpoints.  */
   if (have_ptrace_booke_interface ())
     {
-      if (booke_debug_info.data_bp_alignment
-	  && (addr + len > (addr & ~(booke_debug_info.data_bp_alignment - 1))
-	      + booke_debug_info.data_bp_alignment))
+      /* DAC-based processors (i.e., embedded processors), like the PowerPC 440
+	 have ranged watchpoints and can watch any access within an arbitrary
+	 memory region.  This is useful to watch arrays and structs, for
+	 instance.  It takes two hardware watchpoints though.  */
+      if (ppc_linux_can_use_special_hw_point (&current_target,
+					      HW_POINT_RANGED_WATCH))
+	return 1;
+      else if (booke_debug_info.data_bp_alignment
+	       && (addr + len > (addr & ~(booke_debug_info.data_bp_alignment - 1))
+		   + booke_debug_info.data_bp_alignment))
 	return 0;
     }
   /* addr+len must fall in the 8 byte watchable region for DABR-based
@@ -1754,6 +1768,61 @@ ppc_linux_remove_mask_watchpoint (struct target_ops *ops, CORE_ADDR addr,
   return 0;
 }
 
+static int
+ppc_linux_insert_ranged_watchpoint (struct target_ops *ops, CORE_ADDR addr,
+				    int len, int rw)
+{
+  ptid_t ptid;
+  struct lwp_info *lp;
+  struct ppc_hw_breakpoint p;
+
+  gdb_assert (have_ptrace_booke_interface ());
+
+  p.version = PPC_DEBUG_CURRENT_VERSION;
+  p.trigger_type = get_trigger_type (rw);
+  p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
+  p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE;
+  p.condition_value = 0;
+
+  /* The watchpoint will trigger if the address of the memory access is
+     within the defined range, as follows: p.addr <= address < p.addr2.
+
+     Note that the above sentence just documents how ptrace interprets
+     its arguments; the watchpoint is set to watch the range defined by
+     the user _inclusively_, as specified by the user interface.  */
+  p.addr = (uint64_t) addr;
+  p.addr2 = (uint64_t) addr + len;
+
+  ALL_LWPS (lp, ptid)
+    booke_insert_point (&p, TIDGET (ptid));
+
+  return 0;
+}
+
+static int
+ppc_linux_remove_ranged_watchpoint (struct target_ops *ops, CORE_ADDR addr,
+				    int len, int rw)
+{
+  ptid_t ptid;
+  struct lwp_info *lp;
+  struct ppc_hw_breakpoint p;
+
+  gdb_assert (have_ptrace_booke_interface ());
+
+  p.version = PPC_DEBUG_CURRENT_VERSION;
+  p.trigger_type = get_trigger_type (rw);
+  p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
+  p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE;
+  p.addr = (uint64_t) addr;
+  p.addr2 = (uint64_t) addr + len;
+  p.condition_value = 0;
+
+  ALL_LWPS (lp, ptid)
+    booke_remove_point (&p, TIDGET (ptid));
+
+  return 0;
+}
+
 /* Check whether we have at least one free DVC register.  */
 static int
 can_use_watchpoint_cond_accel (void)
@@ -2209,7 +2278,7 @@ static int
 ppc_linux_hw_point_extra_slot_count (struct target_ops *target,
 				     enum hw_point_flag flag)
 {
-  return flag == HW_POINT_MASKED_WATCH;
+  return flag == HW_POINT_MASKED_WATCH || flag == HW_POINT_RANGED_WATCH;
 }
 
 static void
@@ -2426,6 +2495,8 @@ _initialize_ppc_linux_nat (void)
   t->to_can_use_special_hw_point = ppc_linux_can_use_special_hw_point;
   t->to_insert_watchpoint = ppc_linux_insert_watchpoint;
   t->to_remove_watchpoint = ppc_linux_remove_watchpoint;
+  t->to_insert_ranged_watchpoint = ppc_linux_insert_ranged_watchpoint;
+  t->to_remove_ranged_watchpoint = ppc_linux_remove_ranged_watchpoint;
   t->to_insert_mask_watchpoint = ppc_linux_insert_mask_watchpoint;
   t->to_remove_mask_watchpoint = ppc_linux_remove_mask_watchpoint;
   t->to_stopped_by_watchpoint = ppc_linux_stopped_by_watchpoint;
diff --git a/gdb/target.c b/gdb/target.c
index 84b6302..47a0210 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -607,6 +607,8 @@ update_current_target (void)
       INHERIT (to_remove_hw_breakpoint, t);
       INHERIT (to_insert_watchpoint, t);
       INHERIT (to_remove_watchpoint, t);
+      /* Do not inherit to_insert_ranged_watchpoint.  */
+      /* Do not inherit to_remove_ranged_watchpoint.  */
       /* Do not inherit to_insert_mask_watchpoint.  */
       /* Do not inherit to_remove_mask_watchpoint.  */
       INHERIT (to_stopped_data_address, t);
@@ -3351,6 +3353,52 @@ target_can_use_special_hw_point (enum hw_point_flag flag)
 }
 
 int
+target_insert_ranged_watchpoint (CORE_ADDR addr, int len, int rw)
+{
+  struct target_ops *t;
+
+  for (t = current_target.beneath; t != NULL; t = t->beneath)
+    if (t->to_insert_ranged_watchpoint != NULL)
+      {
+	int ret;
+
+	ret = t->to_insert_ranged_watchpoint (t, addr, len, rw);
+
+	if (targetdebug)
+	  fprintf_unfiltered (gdb_stdlog, "\
+target_insert_ranged_watchpoint (%s, %d, %d) = %d\n",
+			      core_addr_to_string (addr), len, rw, ret);
+
+	return ret;
+      }
+
+  return return_minus_one ();
+}
+
+int
+target_remove_ranged_watchpoint (CORE_ADDR addr, int len, int rw)
+{
+  struct target_ops *t;
+
+  for (t = current_target.beneath; t != NULL; t = t->beneath)
+    if (t->to_remove_ranged_watchpoint != NULL)
+      {
+	int ret;
+
+	ret = t->to_remove_ranged_watchpoint (t, addr, len, rw);
+
+	if (targetdebug)
+	  fprintf_unfiltered (gdb_stdlog, "\
+target_remove_ranged_watchpoint (%s, %d, %d) = %d\n",
+			      core_addr_to_string (addr), len, rw, ret);
+
+	return ret;
+      }
+
+  return return_minus_one ();
+}
+
+int
 target_insert_mask_watchpoint (CORE_ADDR addr, CORE_ADDR mask, int rw)
 {
   struct target_ops *t;
diff --git a/gdb/target.h b/gdb/target.h
index 0d4bdcd..dd822a9 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -450,6 +450,10 @@ struct target_ops
     int (*to_remove_watchpoint) (CORE_ADDR, int, int, struct expression *);
     int (*to_insert_watchpoint) (CORE_ADDR, int, int, struct expression *);
 
+    int (*to_insert_ranged_watchpoint) (struct target_ops *,
+					CORE_ADDR, int, int);
+    int (*to_remove_ranged_watchpoint) (struct target_ops *,
+					CORE_ADDR, int, int);
     int (*to_insert_mask_watchpoint) (struct target_ops *,
 				      CORE_ADDR, CORE_ADDR, int);
     int (*to_remove_mask_watchpoint) (struct target_ops *,
@@ -1343,6 +1347,11 @@ extern int target_can_use_special_hw_point (enum hw_point_flag);
 #define	target_remove_watchpoint(addr, len, type, cond) \
      (*current_target.to_remove_watchpoint) (addr, len, type, cond)
 
+/* Hardware ranged watchpoints.  */
+
+extern int target_insert_ranged_watchpoint (CORE_ADDR, int, int);
+extern int target_remove_ranged_watchpoint (CORE_ADDR, int, int);
+
 /* Hardware watchpoints with an associated address mask.  */
 
 extern int target_insert_mask_watchpoint (CORE_ADDR, CORE_ADDR, int);
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 4d3bf0c..cf4b929 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -487,6 +487,46 @@ ui_out_field_fmt_int (struct ui_out *uiout,
 }
 
 void
+ui_out_field_range_core_addr (struct ui_out *uiout,
+			      const char *fldname,
+			      struct gdbarch *gdbarch,
+			      CORE_ADDR address_start,
+			      CORE_ADDR length)
+{
+  char addstr[80];
+  int addr_bit = gdbarch_addr_bit (gdbarch);
+  CORE_ADDR address_end = address_start + length - 1;
+
+  if (addr_bit < (sizeof (CORE_ADDR) * HOST_CHAR_BIT))
+    {
+      address_start &= ((CORE_ADDR) 1 << addr_bit) - 1;
+      address_end &= ((CORE_ADDR) 1 << addr_bit) - 1;
+    }
+
+  /* FIXME: cagney/2002-05-03: Need local_address_string() function
+     that returns the language localized string formatted to a width
+     based on gdbarch_addr_bit.  */
+  if (addr_bit <= 32)
+    {
+      strcpy (addstr, "[");
+      strcat (addstr, hex_string_custom (address_start, 8));
+      strcat (addstr, ", ");
+      strcat (addstr, hex_string_custom (address_end, 8));
+      strcat (addstr, "]");
+    }
+  else
+    {
+      strcpy (addstr, "[");
+      strcat (addstr, hex_string_custom (address_start, 16));
+      strcat (addstr, ", ");
+      strcat (addstr, hex_string_custom (address_end, 16));
+      strcat (addstr, "]");
+    }
+
+  ui_out_field_string (uiout, fldname, addstr);
+}
+
+void
 ui_out_field_core_addr (struct ui_out *uiout,
 			const char *fldname,
 			struct gdbarch *gdbarch,
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index f65f42b..49e21b6 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -113,6 +113,12 @@ extern void ui_out_field_fmt_int (struct ui_out *uiout, int width,
 				  enum ui_align align, const char *fldname, 
 		 		  int value);
 
+extern void ui_out_field_range_core_addr (struct ui_out *uiout,
+					  const char *fldname,
+					  struct gdbarch *gdbarch,
+					  CORE_ADDR address_start,
+					  CORE_ADDR length);
+
 extern void ui_out_field_core_addr (struct ui_out *uiout, const char *fldname,
 				    struct gdbarch *gdbarch, CORE_ADDR address);
 

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Pedro Alves]

On Tuesday 23 November 2010 21:51:40, Thiago Jung Bauermann wrote:
> Ok?

Apologies for jumping in so late, but, I don't think I understand
why does the user need to know that a watchpoint is "ranged"
or not.  All (low-level) watchpoints are ranged, and what's
relevant here is only the width of the range the hardware can watch,
but that is already a parameter to the target_insert_watchpoint
method.  So I guess my question is, why can't the target backend
manage whether to use a range or "normal" watchpoint when asked to
inserted a watchpoint?  Is it the watch resource accounting done
by breakpoint.c (which is known to be something that should just
go away)?

-- 
Pedro Alves

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Thiago Jung Bauermann]

On Tue, 2010-11-23 at 23:16 +0000, Pedro Alves wrote:
> On Tuesday 23 November 2010 21:51:40, Thiago Jung Bauermann wrote:
> > Ok?
> 
> Apologies for jumping in so late, but, I don't think I understand
> why does the user need to know that a watchpoint is "ranged"
> or not.  All (low-level) watchpoints are ranged,

Good point. At the time these patches were made, GDB didn't behave that
way, but it was fixed one year ago already[1]. I guess I just kept the
old mentality...

> and what's
> relevant here is only the width of the range the hardware can watch,
> but that is already a parameter to the target_insert_watchpoint
> method.  So I guess my question is, why can't the target backend
> manage whether to use a range or "normal" watchpoint when asked to
> inserted a watchpoint?  Is it the watch resource accounting done
> by breakpoint.c (which is known to be something that should just
> go away)?

Yes, the resource accounting is an issue. But also, the target needs to
know that it's being asked to watch a region which is expected to have
accesses in the middle of it. This is because of the difference of how
hardware watchpoints work in server and embedded PowerPC processors. In
the former, a hardware watchpoint triggers when any byte within an 8
byte window starting at the given address is accessed. In the latter,
only accesses at the given address trigger the watchpoint.

So for an embedded powerpc processor, it's not enough to know that it
should watch a 4 byte region at a given address. If it represents an
integer and thus it can expect accesses only at that address then a
regular watchpoint is enough. But if it's an array of four chars, then
it needs two watchpoint registers to set up a hardware watchpoint...

So that's way I created a target_insert_ranged_watchpoint. The other
option would be to add a flag to target_insert_watchpoint...
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


[1] - http://sourceware.org/ml/gdb-patches/2009-12/msg00368.html

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Pedro Alves]

On Wednesday 24 November 2010 21:05:55, Thiago Jung Bauermann wrote:
> On Tue, 2010-11-23 at 23:16 +0000, Pedro Alves wrote:
> > On Tuesday 23 November 2010 21:51:40, Thiago Jung Bauermann wrote:
> > > Ok?
> > 
> > Apologies for jumping in so late, but, I don't think I understand
> > why does the user need to know that a watchpoint is "ranged"
> > or not.  All (low-level) watchpoints are ranged,
> 
> Good point. At the time these patches were made, GDB didn't behave that
> way, but it was fixed one year ago already[1]. I guess I just kept the
> old mentality...

That patch helped in the array case (maybe it was even a regression
that was fixed?), but even the structure case, gdb always behaved
the "ranged" way, AFAIK.  E.g., if you "(gdb) watch" an int, assuming
32-bit, gdb is supposed to be watching the range of 4 bytes starting
from the int's address.  And watching "int i;" is supposed to be
the same as watching "struct {int i;} i";

> 
> > and what's
> > relevant here is only the width of the range the hardware can watch,
> > but that is already a parameter to the target_insert_watchpoint
> > method.  So I guess my question is, why can't the target backend
> > manage whether to use a range or "normal" watchpoint when asked to
> > inserted a watchpoint?  Is it the watch resource accounting done
> > by breakpoint.c (which is known to be something that should just
> > go away)?
> 
> Yes, the resource accounting is an issue. 

> But also, the target needs to
> know that it's being asked to watch a region which is expected to have
> accesses in the middle of it. This is because of the difference of how
> hardware watchpoints work in server and embedded PowerPC processors. In
> the former, a hardware watchpoint triggers when any byte within an 8
> byte window starting at the given address is accessed. In the latter,
> only accesses at the given address trigger the watchpoint.

Hmm..  Oh?  Without knowing a think about BookE, that is not what I'd
infer from:

static int
ppc_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
{
  /* Handle sub-8-byte quantities.  */
  if (len <= 0)
    return 0;

  /* The new BookE ptrace interface tells if there are alignment restrictions
     for watchpoints in the processors.  In that case, we use that information
     to determine the hardcoded watchable region for watchpoints.  */
  if (have_ptrace_booke_interface ())
    {
      if (booke_debug_info.data_bp_alignment
	  && (addr + len > (addr & ~(booke_debug_info.data_bp_alignment - 1))
	      + booke_debug_info.data_bp_alignment))
	return 0;
    }
  /* addr+len must fall in the 8 byte watchable region for DABR-based
     processors (i.e., server processors).  Without the new BookE ptrace
     interface, DAC-based processors (i.e., embedded processors) will use
     addresses aligned to 4-bytes due to the way the read/write flags are
     passed in the old ptrace interface.  */
  else if (((ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
	   && (addr + len) > (addr & ~3) + 4)
	   || (addr + len) > (addr & ~7) + 8)
    return 0;

  return 1;
}

The use of "addr + len" in both BookE cases above
seems to allow setting a BookE exact PPC_BREAKPOINT_MODE_EXACT
watchpoint watching [0x..002, 0x..003] (say, eith an
"int foo" global, watch ((char *)&foo)[2]), but, if this
only traps on exact address matches, rather than on accesses
within the watchable regision, why is there a "len" contemplated?
Confusing.

> So for an embedded powerpc processor, it's not enough to know that it
> should watch a 4 byte region at a given address. If it represents an
> integer and thus it can expect accesses only at that address then a
> regular watchpoint is enough. But if it's an array of four chars, then
> it needs two watchpoint registers to set up a hardware watchpoint...

It all looks to me that the "ranged" notion is backwards.
The default watchpoint that GDB expects to handle _is_ ranged.
E.g., that's how it works on x86, and DABR based PPC.

That's why this:

+* GDB now supports ranged watchpoints, which stop the inferior when it
+  accesses any address within a specified memory range.

Is just confusing, because this is not a new GDB feature.

BTW, it's not only useful to watch the whole range of a 
structure or arrays.  It's often useful to watch what random
code is clobbering an int or pointer.  The bad write may well be
to the middle of the integer or pointer.  I bet that
users end up surprised if PPC doesn't catch that with
gdb's "watch" command.

> So that's way I created a target_insert_ranged_watchpoint. The other
> option would be to add a flag to target_insert_watchpoint...

It appears to me that if there should be a new kind of way to
insert watchpoints, it should be to allow setting watchpoints
that only work if the accesses are aligned, that is to support
the behaviour you describe with PPC_BREAKPOINT_MODE_EXACT
in embedded processors.

-- 
Pedro Alves

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Thu, 25 Nov 2010 17:31:57 +0000
> Cc: gdb-patches@sourceware.org,
>  Jan Kratochvil <jan.kratochvil@redhat.com>,
>  Joel Brobecker <brobecker@adacore.com>,
>  Eli Zaretskii <eliz@gnu.org>
> 
> > So that's way I created a target_insert_ranged_watchpoint. The other
> > option would be to add a flag to target_insert_watchpoint...
> 
> It appears to me that if there should be a new kind of way to
> insert watchpoints, it should be to allow setting watchpoints
> that only work if the accesses are aligned

What would be the use-cases where such behavior would be needed?

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Thiago Jung Bauermann]

On Fri, 2010-11-26 at 13:10 +0200, Eli Zaretskii wrote:
> > > So that's way I created a target_insert_ranged_watchpoint. The other
> > > option would be to add a flag to target_insert_watchpoint...
> > 
> > It appears to me that if there should be a new kind of way to
> > insert watchpoints, it should be to allow setting watchpoints
> > that only work if the accesses are aligned
> 
> What would be the use-cases where such behavior would be needed?

In the case of embedded PowerPC processors, that's the only kind of
watchpoint available using a single watchpoint register.

To create a "regular" watchpoint in that platform, you need two
watchpoint registers, from a total of two available in the processor.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Eli Zaretskii]

> From: Thiago Jung Bauermann <bauerman@br.ibm.com>
> Cc: Pedro Alves <pedro@codesourcery.com>, gdb-patches@sourceware.org,
>         jan.kratochvil@redhat.com, brobecker@adacore.com
> Date: Sat, 27 Nov 2010 11:25:15 -0200
> 
> On Fri, 2010-11-26 at 13:10 +0200, Eli Zaretskii wrote:
> > > > So that's way I created a target_insert_ranged_watchpoint. The other
> > > > option would be to add a flag to target_insert_watchpoint...
> > > 
> > > It appears to me that if there should be a new kind of way to
> > > insert watchpoints, it should be to allow setting watchpoints
> > > that only work if the accesses are aligned
> > 
> > What would be the use-cases where such behavior would be needed?
> 
> In the case of embedded PowerPC processors, that's the only kind of
> watchpoint available using a single watchpoint register.

Okay, but I thought Pedro was suggesting to make such "aligned-access"
watchpoints available on platforms that don't have this limitation.  I
was asking why would they be useful on those other platforms.

> To create a "regular" watchpoint in that platform, you need two
> watchpoint registers, from a total of two available in the processor.

Understood.  I would then suggest a platform-specific option,
defaulting to OFF, which, when ON, would cause GDB to use 2 registers
for a watchpoint, in order to be able to catch unaligned accesses.
This is under the assumption that most use-cases want to watch
accesses to variables using the right alignment, and that therefore
using 2 registers by default would be a waste of resources that are at
premium on this platform.

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Thiago Jung Bauermann]

On Thu, 2010-11-25 at 17:31 +0000, Pedro Alves wrote:
> On Wednesday 24 November 2010 21:05:55, Thiago Jung Bauermann wrote:
> > On Tue, 2010-11-23 at 23:16 +0000, Pedro Alves wrote:
> > > On Tuesday 23 November 2010 21:51:40, Thiago Jung Bauermann wrote:
> That patch helped in the array case (maybe it was even a regression
> that was fixed?), but even the structure case, gdb always behaved
> the "ranged" way, AFAIK.  E.g., if you "(gdb) watch" an int, assuming
> 32-bit, gdb is supposed to be watching the range of 4 bytes starting
> from the int's address.  And watching "int i;" is supposed to be
> the same as watching "struct {int i;} i";

Is there a difference between "int i" and "struct {int i;} i" besides
having to access the integer using "i.i" instead of "i"?

But I see your point, and agree.

> > But also, the target needs to
> > know that it's being asked to watch a region which is expected to have
> > accesses in the middle of it. This is because of the difference of how
> > hardware watchpoints work in server and embedded PowerPC processors. In
> > the former, a hardware watchpoint triggers when any byte within an 8
> > byte window starting at the given address is accessed. In the latter,
> > only accesses at the given address trigger the watchpoint.
> 
> Hmm..  Oh?  Without knowing a think about BookE, that is not what I'd
> infer from:
> 
> static int
> ppc_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
> {
>   /* Handle sub-8-byte quantities.  */
>   if (len <= 0)
>     return 0;
> 
>   /* The new BookE ptrace interface tells if there are alignment restrictions
>      for watchpoints in the processors.  In that case, we use that information
>      to determine the hardcoded watchable region for watchpoints.  */
>   if (have_ptrace_booke_interface ())
>     {
>       if (booke_debug_info.data_bp_alignment
> 	  && (addr + len > (addr & ~(booke_debug_info.data_bp_alignment - 1))
> 	      + booke_debug_info.data_bp_alignment))

data_bp_alignment tells GDB that the processor only accepts watchpoints
at certain address intervals. This code assumes that if that is the
case, then the watchpoint triggers with any access within the interval.
This covers server processors. For BookE processors, data_bp_alignment
should be 0 (since they can put a watchpoint at any address) and the
function will return 1 regardless of len. Perhaps I should check for
data_bp_alignment > 1 since 1 could also express that a watchpoint can
be put anywere.

> 	return 0;
>     }
>   /* addr+len must fall in the 8 byte watchable region for DABR-based
>      processors (i.e., server processors).  Without the new BookE ptrace
>      interface, DAC-based processors (i.e., embedded processors) will use
>      addresses aligned to 4-bytes due to the way the read/write flags are
>      passed in the old ptrace interface.  */
>   else if (((ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
> 	   && (addr + len) > (addr & ~3) + 4)
> 	   || (addr + len) > (addr & ~7) + 8)
>     return 0;

This code just preserves GDB behaviour when the new BookE ptrace
interface is not available. I agree it's buggy but I didn't think it was
a problem to address in this patch series (also, not sure how to fix it
now that you mention that GDB expects watchpoints to be ranged).

> > So for an embedded powerpc processor, it's not enough to know that it
> > should watch a 4 byte region at a given address. If it represents an
> > integer and thus it can expect accesses only at that address then a
> > regular watchpoint is enough. But if it's an array of four chars, then
> > it needs two watchpoint registers to set up a hardware watchpoint...
> 
> It all looks to me that the "ranged" notion is backwards.
> The default watchpoint that GDB expects to handle _is_ ranged.
> E.g., that's how it works on x86, and DABR based PPC.
> 
> That's why this:
> 
> +* GDB now supports ranged watchpoints, which stop the inferior when it
> +  accesses any address within a specified memory range.
> 
> Is just confusing, because this is not a new GDB feature.

So I should be more specific and say:

"GDB now supports hardware accelerated watchpoints which cover a memory
region on embedded PowerPC processors running the Linux kernel."

WDYT?

> BTW, it's not only useful to watch the whole range of a 
> structure or arrays.  It's often useful to watch what random
> code is clobbering an int or pointer.  The bad write may well be
> to the middle of the integer or pointer.  I bet that
> users end up surprised if PPC doesn't catch that with
> gdb's "watch" command.

Good point.

> > So that's way I created a target_insert_ranged_watchpoint. The other
> > option would be to add a flag to target_insert_watchpoint...
> 
> It appears to me that if there should be a new kind of way to
> insert watchpoints, it should be to allow setting watchpoints
> that only work if the accesses are aligned, that is to support
> the behaviour you describe with PPC_BREAKPOINT_MODE_EXACT
> in embedded processors.

So in your opinion, the watch command should always use two watchpoint
registers to set up a ranged watchpoint in BookE ppc? I'm a bit
reluctant to use all the watchpoint registers to set up one
watchpoint...

Then comes the question of how to support the PPC_BREAKPOINT_MODE_EXACT
behaviour... A new flag or command to let the user "opt in" to a less
capable watchpoint, but one which uses less hardware resources?
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Pedro Alves]

On Friday 26 November 2010 21:15:22, Thiago Jung Bauermann wrote:

> So in your opinion, the watch command should always use two watchpoint
> registers to set up a ranged watchpoint in BookE ppc? I'm a bit
> reluctant to use all the watchpoint registers to set up one
> watchpoint...

Blame the hardware designers, not me.  :-)

> Then comes the question of how to support the PPC_BREAKPOINT_MODE_EXACT
> behaviour... A new flag or command to let the user "opt in" to a less
> capable watchpoint, but one which uses less hardware resources?

Yes, that's what I think we should do.  I can help you code this,
but I'm going computer-less real soon now until the 3rd December,
and I'll only be able to help/review changes then onwards.

Here's what I think we should do in more detail:

 1 - make ppc-linux-nat.c insert ranged watchpoints for len > 1.

 2 - make ppc-linux-nat.c insert exact watchpoints when len == 1.

with the above, watchpoints will behave as core gdb wants,
while we will still use only one register in the case that
is possible.

 3 - add a new command to let the user select that gdb should
     trust that primitive types are always accessed through their
     address (== exact).
     My opinion is that it should be off by default, on the
     grounds that most users aren't and shouldn't be that
     familiar with the debug support of the chip or target
     they're using.  Users want things to Just Work.
     Every case that gdb doesn't behave the way it is
     supposed to by default, and there's diverging behavior
     in different target, is a case for frustration, bug
     reports, and support requests.  Another argument is that
     simulators/emulators (think qemu, for example) may not
     have this limitation, and can implement the RSP
     watchpoint packets as gdb expects (always ranged).

 4 - implementation wise, when the option added in #3 is
     enabled, gdb (breakpoint.c) checks if the memory being watched
     corresponds to a scalar type.  (or, going a step
     further, if wrapped in a structure or union or array,
     that only contains a single scalar, recursively).  If
     true, then the "struct value" gdb used to record the
     expression's current value (for comparison with the
     new value whenever the low level watchpoint triggers) is
     the same as usual, but, the watchpoint location that is associated
     with the watchpoint is set to have length == 1, instead of
     the whole width of the watched memory range.
     This way, the target is requested to watch a single
     byte at the scalar's address.  Note how due to the
     change done in #2, ppc-linux-nat.c will insert an 
     PPC_BREAKPOINT_MODE_EXACT in this case.

     This means that no changes to the target_insert_watchpoint
     interface are required.  This also means that no 
     changes to the remote protocol are required.  If
     the new option is on, and the user wants to watch
     an "int i", whose address is 0xD3ADB33F, the target
     will see a "Z2,D3ADB33F,1" request.  That is, watch
     writes to the 1 byte long range starting at D3ADB33F.
     That is the same as saying trap only on writes to
     D3ADB33F.

     This also means that no new breakpoint_ops type
     is required either.  A new flag in the breakpoint will do
     (so that the "exact" property of watchpoints already
     created is preserved if the user flips the switch).

WDYT?  Sounds reasonable?  I'd prefer doing these changes
as first step.  I haven't looked yet at what kind of masks
masked watchpoints support, but if they only support masks in the form
of (binary) 11110000, 1111111100, etc. (no alternating 1s and 0s),
then no target_insert_watchpoint or remote protocol change is
required either for those.

-- 
Pedro Alves

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Pedro Alves]

On Saturday 27 November 2010 17:47:38, Pedro Alves wrote:
> On Friday 26 November 2010 21:15:22, Thiago Jung Bauermann wrote:
> 
> > So in your opinion, the watch command should always use two watchpoint
> > registers to set up a ranged watchpoint in BookE ppc? I'm a bit
> > reluctant to use all the watchpoint registers to set up one
> > watchpoint...
> 
> Blame the hardware designers, not me.  :-)
> 
> > Then comes the question of how to support the PPC_BREAKPOINT_MODE_EXACT
> > behaviour... A new flag or command to let the user "opt in" to a less
> > capable watchpoint, but one which uses less hardware resources?
> 
> Yes, that's what I think we should do.  I can help you code this,
> but I'm going computer-less real soon now until the 3rd December,
> and I'll only be able to help/review changes then onwards.
> 
> Here's what I think we should do in more detail:
> 
>  1 - make ppc-linux-nat.c insert ranged watchpoints for len > 1.
> 
>  2 - make ppc-linux-nat.c insert exact watchpoints when len == 1.

BTW, optional, but something that the ppc watchpoint support is
lacking.  Step 2.5, ppc-linux-nat.c should be taught to keep a list
of watchpoint requests, and merge watch requests for the same
addresses.  See <http://sourceware.org/ml/gdb-patches/2010-09/msg00470.html>
for an example of what that would fix.

> 
> with the above, watchpoints will behave as core gdb wants,
> while we will still use only one register in the case that
> is possible.
> 
>  3 - add a new command to let the user select that gdb should
>      trust that primitive types are always accessed through their
>      address (== exact).
>      My opinion is that it should be off by default, on the
>      grounds that most users aren't and shouldn't be that
>      familiar with the debug support of the chip or target
>      they're using.  Users want things to Just Work.
>      Every case that gdb doesn't behave the way it is
>      supposed to by default, and there's diverging behavior
>      in different target, is a case for frustration, bug
>      reports, and support requests.  Another argument is that
>      simulators/emulators (think qemu, for example) may not
>      have this limitation, and can implement the RSP
>      watchpoint packets as gdb expects (always ranged).
> 
>  4 - implementation wise, when the option added in #3 is
>      enabled, gdb (breakpoint.c) checks if the memory being watched
>      corresponds to a scalar type.  (or, going a step
>      further, if wrapped in a structure or union or array,
>      that only contains a single scalar, recursively).  If
>      true, then the "struct value" gdb used to record the
>      expression's current value (for comparison with the
>      new value whenever the low level watchpoint triggers) is
>      the same as usual, but, the watchpoint location that is associated
>      with the watchpoint is set to have length == 1, instead of
>      the whole width of the watched memory range.
>      This way, the target is requested to watch a single
>      byte at the scalar's address.  Note how due to the
>      change done in #2, ppc-linux-nat.c will insert an 
>      PPC_BREAKPOINT_MODE_EXACT in this case.
> 
>      This means that no changes to the target_insert_watchpoint
>      interface are required.  This also means that no 
>      changes to the remote protocol are required.  If
>      the new option is on, and the user wants to watch
>      an "int i", whose address is 0xD3ADB33F, the target
>      will see a "Z2,D3ADB33F,1" request.  That is, watch
>      writes to the 1 byte long range starting at D3ADB33F.
>      That is the same as saying trap only on writes to
>      D3ADB33F.
> 
>      This also means that no new breakpoint_ops type
>      is required either.  A new flag in the breakpoint will do
>      (so that the "exact" property of watchpoints already
>      created is preserved if the user flips the switch).
> 
> WDYT?  Sounds reasonable?  I'd prefer doing these changes
> as first step.  I haven't looked yet at what kind of masks
> masked watchpoints support, but if they only support masks in the form
> of (binary) 11110000, 1111111100, etc. (no alternating 1s and 0s),
> then no target_insert_watchpoint or remote protocol change is
> required either for those.

-- 
Pedro Alves

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Thiago Jung Bauermann]

On Sat, 2010-11-27 at 17:47 +0000, Pedro Alves wrote:
> On Friday 26 November 2010 21:15:22, Thiago Jung Bauermann wrote:
> > So in your opinion, the watch command should always use two watchpoint
> > registers to set up a ranged watchpoint in BookE ppc? I'm a bit
> > reluctant to use all the watchpoint registers to set up one
> > watchpoint...
> 
> Blame the hardware designers, not me.  :-)

Well, you know hardware designers. They're transistor counters. :-)

> WDYT?  Sounds reasonable?  I'd prefer doing these changes
> as first step.

Thank you very much for the detailed plans! I'm very sorry about my
delay answering this. I'm waiting for internal feedback about this
option.

> I haven't looked yet at what kind of masks
> masked watchpoints support, but if they only support masks in the form
> of (binary) 11110000, 1111111100, etc. (no alternating 1s and 0s),
> then no target_insert_watchpoint or remote protocol change is
> required either for those.

Masked watchpoints support arbitrary masks, so they warrant their own
place under the sun. :-)  They be considered independently of ranged
watchpoints then.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Thiago Jung Bauermann]

Hi Pedro,

On Sat, 2010-11-27 at 17:47 +0000, Pedro Alves wrote:
> On Friday 26 November 2010 21:15:22, Thiago Jung Bauermann wrote:
> > Then comes the question of how to support the PPC_BREAKPOINT_MODE_EXACT
> > behaviour... A new flag or command to let the user "opt in" to a less
> > capable watchpoint, but one which uses less hardware resources?
> 
> Yes, that's what I think we should do.  I can help you code this,
> but I'm going computer-less real soon now until the 3rd December,
> and I'll only be able to help/review changes then onwards.
> 
> Here's what I think we should do in more detail:

This patch implements your suggestions. The flag it adds is called "set
powerpc exact-watchpoints".

It applies on top of the "convert watchpoints to use breakpoint_ops"
patch (i.e., I changed the order of my patch series, now this patch
comes before the masked watchpoints patch).

There are no regressions on ppc-linux, ppc64-linux and i386-linux. Ok?
I had to make changes to the documentation, so it needs a doc review
too.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2010-12-23  Sergio Durigan Junior  <sergiodj@linux.vnet.ibm.com>
	    Thiago Jung Bauermann  <bauerman@br.ibm.com>

	Implement support for PowerPC BookE ranged watchpoints.

gdb/
	* breakpoint.h 
	(struct breakpoint_ops) <extra_resources_needed>: New methods
	Initialize to NULL in all existing breakpoint_ops instances.
	(struct breakpoint) <exact>: New field.
	(target_exact_watchpoints): Declare external global.
	* breakpoint.c (target_exact_watchpoints): New global flag.
	(update_watchpoint): Call breakpoint's
	breakpoint_ops.extra_resources_needed if available.
	(hw_watchpoint_used_count): Call breakpoint's
	breakpoint_ops.extra_resources_needed if available.  Change to
	count number of bp_locations, instead of number of high-level
	breakpoints.
	(insert_watchpoint, remove_watchpoint): Use fixed length of 1 byte
	if the watchpoint is exact.
	(extra_resources_needed_watchpoint): New function.
	(watchpoint_breakpoint_ops): Add extra_resources_needed_watchpoint.
	(watch_command_1): Set b->exact if the user asked for an exact
	watchpoint and one can be set.
	(can_use_hardware_watchpoint): Accept exact_watchpoints argument.
	Pass fixed length of 1 to target_region_ok_for_hw_watchpoint if
	the user asks for an exact watchpoint and one can be set.  Return
	number of needed debug registers to watch the expression.
	* gdbtypes.c (is_scalar_type, is_scalar_type_recursive): New functions.
	* gdbtypes.h (is_scalar_type_recursive): Declare.
	* ppc-linux-nat.c (ppc_linux_region_ok_for_hw_watchpoint): Always
	handle regions when ranged watchpoints are available.
	(create_watchpoint_request): New function.
	(ppc_linux_insert_watchpoint, ppc_linux_remove_watchpoint): Use
	create_watchpoint_request.
	* rs6000-tdep.c (show_powerpc_exact_watchpoints): New function.
	(_initialize_rs6000_tdep): Add `exact-watchpoints' boolean to the
	`set powerpc' and `show powerpc' commands.
	* target.h (struct target_ops) <to_region_ok_for_hw_watchpoint>:
	Mention documentation comment in the target macro.
	(target_region_ok_for_hw_watchpoint): Document return value.

gdb/doc/
	* gdb.texinfo (PowerPC Embedded): Document ranged watchpoints and
	the "set powerpc exact-watchpoints" flag.


diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 7ea01c7..3a2a266 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -10821,6 +10821,7 @@ static struct breakpoint_ops catch_exception_breakpoint_ops =
   NULL, /* insert */
   NULL, /* remove */
   NULL, /* breakpoint_hit */
+  NULL, /* extra_resources_needed */
   print_it_catch_exception,
   print_one_catch_exception,
   print_mention_catch_exception,
@@ -10859,6 +10860,7 @@ static struct breakpoint_ops catch_exception_unhandled_breakpoint_ops = {
   NULL, /* insert */
   NULL, /* remove */
   NULL, /* breakpoint_hit */
+  NULL, /* extra_resources_needed */
   print_it_catch_exception_unhandled,
   print_one_catch_exception_unhandled,
   print_mention_catch_exception_unhandled,
@@ -10895,6 +10897,7 @@ static struct breakpoint_ops catch_assert_breakpoint_ops = {
   NULL, /* insert */
   NULL, /* remove */
   NULL, /* breakpoint_hit */
+  NULL, /* extra_resources_needed */
   print_it_catch_assert,
   print_one_catch_assert,
   print_mention_catch_assert,
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 003899c..9c10b30 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -98,7 +98,7 @@ static void clear_command (char *, int);
 
 static void catch_command (char *, int);
 
-static int can_use_hardware_watchpoint (struct value *);
+static int can_use_hardware_watchpoint (struct value *, int);
 
 static void break_command_1 (char *, int, int);
 
@@ -345,6 +345,9 @@ static int executing_breakpoint_commands;
 /* Are overlay event breakpoints enabled? */
 static int overlay_events_enabled;
 
+/* See description in breakpoint.h. */
+int target_exact_watchpoints = 0;
+
 /* Walk the following statement or block through all breakpoints.
    ALL_BREAKPOINTS_SAFE does so even if the statment deletes the current
    breakpoint.  */
@@ -1394,25 +1397,33 @@ update_watchpoint (struct breakpoint *b, int reparse)
 	if ((b->type == bp_watchpoint || b->type == bp_hardware_watchpoint)
 	    && reparse)
 	  {
-	    int i, mem_cnt, other_type_used;
-
-	    /* We need to determine how many resources are already used
-	       for all other hardware watchpoints to see if we still have
-	       enough resources to also fit this watchpoint in as well.
-	       To avoid the hw_watchpoint_used_count call below from counting
-	       this watchpoint, make sure that it is marked as a software
-	       watchpoint.  */
-	    b->type = bp_watchpoint;
-	    i = hw_watchpoint_used_count (bp_hardware_watchpoint,
-					  &other_type_used);
-	    mem_cnt = can_use_hardware_watchpoint (val_chain);
-
-	    if (!mem_cnt)
+	    int reg_cnt;
+
+	    reg_cnt = can_use_hardware_watchpoint (val_chain, b->exact);
+
+	    if (!reg_cnt)
 	      b->type = bp_watchpoint;
 	    else
 	      {
-		int target_resources_ok = target_can_use_hardware_watchpoint
-		  (bp_hardware_watchpoint, i + mem_cnt, other_type_used);
+		int i, other_type_used, target_resources_ok;
+		enum bptype orig_type;
+
+		if (b->ops && b->ops->extra_resources_needed)
+		  reg_cnt += b->ops->extra_resources_needed (b, NULL);
+
+		/* We need to determine how many resources are already used
+		   for all other hardware watchpoints to see if we still have
+		   enough resources to also fit this watchpoint in as well.
+		   To avoid the hw_watchpoint_used_count call below from
+		   counting this watchpoint, make sure that it is marked as a
+		   software watchpoint.  */
+		orig_type = b->type;
+		b->type = bp_watchpoint;
+		i = hw_watchpoint_used_count (bp_hardware_watchpoint,
+					      &other_type_used);
+
+		target_resources_ok = target_can_use_hardware_watchpoint
+		  (bp_hardware_watchpoint, i + reg_cnt, other_type_used)
 		if (target_resources_ok <= 0)
 		  b->type = bp_watchpoint;
 		else
@@ -6004,6 +6015,7 @@ static struct breakpoint_ops catch_fork_breakpoint_ops =
   insert_catch_fork,
   remove_catch_fork,
   breakpoint_hit_catch_fork,
+  NULL, /* extra_resources_needed */
   print_it_catch_fork,
   print_one_catch_fork,
   print_mention_catch_fork,
@@ -6095,6 +6107,7 @@ static struct breakpoint_ops catch_vfork_breakpoint_ops =
   insert_catch_vfork,
   remove_catch_vfork,
   breakpoint_hit_catch_vfork,
+  NULL, /* extra_resources_needed */
   print_it_catch_vfork,
   print_one_catch_vfork,
   print_mention_catch_vfork,
@@ -6376,6 +6389,7 @@ static struct breakpoint_ops catch_syscall_breakpoint_ops =
   insert_catch_syscall,
   remove_catch_syscall,
   breakpoint_hit_catch_syscall,
+  NULL, /* extra_resources_needed */
   print_it_catch_syscall,
   print_one_catch_syscall,
   print_mention_catch_syscall,
@@ -6529,6 +6543,7 @@ static struct breakpoint_ops catch_exec_breakpoint_ops =
   insert_catch_exec,
   remove_catch_exec,
   breakpoint_hit_catch_exec,
+  NULL, /* extra_resources_needed */
   print_it_catch_exec,
   print_one_catch_exec,
   print_mention_catch_exec,
@@ -6570,19 +6585,29 @@ static int
 hw_watchpoint_used_count (enum bptype type, int *other_type_used)
 {
   struct breakpoint *b;
+  struct bp_location *bl;
   int i = 0;
 
   *other_type_used = 0;
   ALL_BREAKPOINTS (b)
-  {
-    if (breakpoint_enabled (b))
-      {
+    {
+      if (!breakpoint_enabled (b))
+	continue;
+
 	if (b->type == type)
-	  i++;
+	  for (bl = b->loc; bl; bl = bl->next)
+	    {
+	      i++;
+
+	      /* Special types of hardware watchpoints may use more than
+		 one register.  */
+	      if (b->ops && b->ops->extra_resources_needed)
+		i += b->ops->extra_resources_needed (b, bl);
+	    }
 	else if (is_hardware_watchpoint (b))
 	  *other_type_used = 1;
-      }
-  }
+    }
+
   return i;
 }
 
@@ -8119,8 +8144,10 @@ watchpoint_exp_is_const (const struct expression *exp)
 static int
 insert_watchpoint (struct bp_location *bl)
 {
-  return target_insert_watchpoint (bl->address, bl->length,
-				   bl->watchpoint_type, bl->owner->cond_exp);
+  int length = bl->owner->exact? 1 : bl->length;
+
+  return target_insert_watchpoint (bl->address, length, bl->watchpoint_type,
+				   bl->owner->cond_exp);
 }
 
 /* Implement the "remove" breakpoint_ops method for hardware watchpoints.  */
@@ -8128,8 +8155,31 @@ insert_watchpoint (struct bp_location *bl)
 static int
 remove_watchpoint (struct bp_location *bl)
 {
-  return target_remove_watchpoint (bl->address, bl->length,
-				   bl->watchpoint_type, bl->owner->cond_exp);
+  int length = bl->owner->exact? 1 : bl->length;
+
+  return target_remove_watchpoint (bl->address, length, bl->watchpoint_type,
+				   bl->owner->cond_exp);
+}
+
+/* Implement the "extra_resources_needed" breakpoint_ops method for
+   hardware watchpoints.  */
+
+static int
+extra_resources_needed_watchpoint (const struct breakpoint *b,
+				   const struct bp_location *bl)
+{
+  if (bl)
+    {
+      int ret, length = bl->owner->exact? 1 : bl->length;
+
+      ret = target_region_ok_for_hw_watchpoint (bl->address, length);
+
+      gdb_assert (ret > 0);
+
+      return ret - 1;
+    }
+  else
+    return 0;
 }
 
 /* The breakpoint_ops structure to be used in hardware watchpoints.  */
@@ -8139,6 +8189,7 @@ static struct breakpoint_ops watchpoint_breakpoint_ops =
   insert_watchpoint,
   remove_watchpoint,
   NULL, /* breakpoint_hit */
+  extra_resources_needed_watchpoint,
   NULL, /* print_it */
   NULL, /* print_one */
   NULL, /* print_mention */
@@ -8165,7 +8216,7 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
   char *cond_end = NULL;
   int i, other_type_used, target_resources_ok = 0;
   enum bptype bp_type;
-  int mem_cnt = 0;
+  int reg_cnt = 0;
   int thread = -1;
   int pc = 0;
 
@@ -8300,14 +8351,14 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
   else
     bp_type = bp_hardware_watchpoint;
 
-  mem_cnt = can_use_hardware_watchpoint (val);
-  if (mem_cnt == 0 && bp_type != bp_hardware_watchpoint)
+  reg_cnt = can_use_hardware_watchpoint (val, target_exact_watchpoints);
+  if (reg_cnt == 0 && bp_type != bp_hardware_watchpoint)
     error (_("Expression cannot be implemented with read/access watchpoint."));
-  if (mem_cnt != 0)
+  if (reg_cnt != 0)
     {
       i = hw_watchpoint_used_count (bp_type, &other_type_used);
       target_resources_ok = 
-	target_can_use_hardware_watchpoint (bp_type, i + mem_cnt, 
+	target_can_use_hardware_watchpoint (bp_type, i + reg_cnt,
 					    other_type_used);
       if (target_resources_ok == 0 && bp_type != bp_hardware_watchpoint)
 	error (_("Target does not support this type of hardware watchpoint."));
@@ -8318,7 +8369,7 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
 
   /* Change the type of breakpoint to an ordinary watchpoint if a hardware
      watchpoint could not be set.  */
-  if (!mem_cnt || target_resources_ok <= 0)
+  if (!reg_cnt || target_resources_ok <= 0)
     bp_type = bp_watchpoint;
 
   frame = block_innermost_frame (exp_valid_block);
@@ -8389,6 +8440,10 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
   b->val_valid = 1;
   b->ops = &watchpoint_breakpoint_ops;
 
+  /* Use an exact watchpoint only when there's one memory region to be
+     watched, which needs only one debug register.  */
+  b->exact = target_exact_watchpoints && reg_cnt == 1;
+
   if (cond_start)
     b->cond_string = savestring (cond_start, cond_end - cond_start);
   else
@@ -8428,12 +8483,15 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
   update_global_location_list (1);
 }
 
-/* Return count of locations need to be watched and can be handled
-   in hardware.  If the watchpoint can not be handled
-   in hardware return zero.  */
+/* Return count of debug registers need to watch the given expression.
+   If EXACT_WATCHPOINTS is 1, then consider that only the address of
+   the start of the watched region will be monitored (i.e., all accesses
+   will be aligned).  This uses less debug registers on some targets.
+
+   If the watchpoint can not be handled in hardware return zero.  */
 
 static int
-can_use_hardware_watchpoint (struct value *v)
+can_use_hardware_watchpoint (struct value *v, int exact_watchpoints)
 {
   int found_memory_cnt = 0;
   struct value *head = v;
@@ -8486,12 +8544,18 @@ can_use_hardware_watchpoint (struct value *v)
 		      && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
 		{
 		  CORE_ADDR vaddr = value_address (v);
-		  int       len   = TYPE_LENGTH (value_type (v));
+		  int len;
+		  int num_regs;
+
+		  len = (exact_watchpoints
+			 && is_scalar_type_recursive (vtype))?
+		    1 : TYPE_LENGTH (value_type (v));
 
-		  if (!target_region_ok_for_hw_watchpoint (vaddr, len))
+		  num_regs = target_region_ok_for_hw_watchpoint (vaddr, len);
+		  if (!num_regs)
 		    return 0;
 		  else
-		    found_memory_cnt++;
+		    found_memory_cnt += num_regs;
 		}
 	    }
 	}
@@ -8915,6 +8979,7 @@ static struct breakpoint_ops gnu_v3_exception_catchpoint_ops = {
   NULL, /* insert */
   NULL, /* remove */
   NULL, /* breakpoint_hit */
+  NULL, /* extra_resources_needed */
   print_exception_catchpoint,
   print_one_exception_catchpoint,
   print_mention_exception_catchpoint,
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 12c2df2..8e68532 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -372,6 +372,16 @@ struct breakpoint_ops
      breakpoint was hit.  */
   int (*breakpoint_hit) (struct breakpoint *);
 
+  /* Tell how many additional hardware resources (debug registers) are needed
+     for this breakpoint.  The bp_location argument may be NULL if the method
+     is called at a point where no location is assigned to the breakpoint yet.
+
+     We always count at least one resource, so if this function returns 1, then
+     GDB will consider that the breakpoint needes 2 registers.  If this element
+     is NULL, then no additional resources are needed.  */
+  int (*extra_resources_needed) (const struct breakpoint *,
+				 const struct bp_location *);
+
   /* The normal print routine for this breakpoint, called when we
      hit it.  */
   enum print_stop_action (*print_it) (struct breakpoint *);
@@ -411,6 +421,13 @@ DEF_VEC_P(bp_location_p);
    detail to the breakpoints module.  */
 struct counted_command_line;
 
+/* Some targets (eg, embedded PowerPC) need two debug registers to set
+   a watchpoint over a memory region.  If this flag is true, GDB will use
+   only one register per watchpoint, thus assuming that all acesses that
+   modify a memory location happen at its starting address. */
+
+extern int target_exact_watchpoints;
+
 /* 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
@@ -577,7 +594,10 @@ struct breakpoint
        can sometimes be NULL for enabled GDBs as not all breakpoint
        types are tracked by the Python scripting API.  */
     struct breakpoint_object *py_bp_object;
-};
+
+    /* Whether this watchpoint is exact (see target_exact_watchpoints).  */
+    int exact;
+  };
 
 typedef struct breakpoint *breakpoint_p;
 DEF_VEC_P(breakpoint_p);
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 0e1d553..cef320e 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -18728,9 +18728,25 @@ implement in hardware simple hardware watchpoint conditions of the form:
   if  @var{ADDRESS|VARIABLE} == @var{CONSTANT EXPRESSION}
 @end smallexample
 
-The DVC register will be automatically used whenever @value{GDBN} detects
-such pattern in a condition expression.  This feature is available in native
-@value{GDBN} running on a Linux kernel version 2.6.34 or newer.
+The DVC register will be automatically used when @value{GDBN} detects
+such pattern in a condition expression, and the created watchpoint uses one
+debug register (either the @code{exact-watchpoints} option is on and the
+variable is scalar, or the variable has a length of one byte).  This feature
+is available in native @value{GDBN} running on a Linux kernel version 2.6.34
+or newer.
+
+When running on PowerPC embedded processors @value{GDBN} automatically uses
+ranged hardware watchpoints unless the @code{exact-watchpoints} option is on,
+in which case watchpoints using only one debug register are created when
+watching variables of scalar types.
+
+You can create an artificial array to watch an arbitrary memory
+region using one of the following commands (@pxref{Expressions}):
+
+@smallexample
+(@value{GDBP}) watch *((char *) @var{ADDRESS})@@@var{LENGTH}
+(@value{GDBP}) watch @{char[@var{LENGTH}]@} @var{ADDRESS}
+@end smallexample
 
 @value{GDBN} provides the following PowerPC-specific commands:
 
@@ -18751,6 +18767,12 @@ arguments and return values.  The valid options are @samp{auto};
 registers.  By default, @value{GDBN} selects the calling convention
 based on the selected architecture and the provided executable file.
 
+@item set powerpc exact-watchpoints
+@itemx show powerpc exact-watchpoints
+Allow @value{GDBN} to use only one debug register when watching a variable
+of scalar type, thus assuming that all acesses that modify that variable
+happen at its starting address.
+
 @kindex target dink32
 @item target dink32 @var{dev}
 DINK32 ROM monitor.
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index b651098..4e48d06 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1955,6 +1955,49 @@ is_integral_type (struct type *t)
 	 || (TYPE_CODE (t) == TYPE_CODE_BOOL)));
 }
 
+static int
+is_scalar_type (struct type *t)
+{
+  CHECK_TYPEDEF (t);
+  return (is_integral_type (t) || ((t != NULL)
+	  && (TYPE_CODE (t) == TYPE_CODE_FLT)));
+}
+
+int
+is_scalar_type_recursive (struct type *t)
+{
+  CHECK_TYPEDEF (t);
+
+  if (is_scalar_type (t))
+    return 1;
+  else if ((TYPE_CODE (t) == TYPE_CODE_ARRAY
+	    || TYPE_CODE (t) == TYPE_CODE_STRING) && TYPE_NFIELDS (t) == 1
+	   && TYPE_CODE (TYPE_INDEX_TYPE (t)) == TYPE_CODE_RANGE)
+    {
+      LONGEST low_bound, high_bound;
+      struct type *elt_type = check_typedef (TYPE_TARGET_TYPE (t));
+
+      get_discrete_bounds (TYPE_INDEX_TYPE (t), &low_bound, &high_bound);
+
+      return high_bound == low_bound && is_scalar_type_recursive (elt_type);
+    }
+  else if (TYPE_CODE (t) == TYPE_CODE_STRUCT && TYPE_NFIELDS (t) == 1)
+    return is_scalar_type_recursive (TYPE_FIELD_TYPE (t, 0));
+  else if (TYPE_CODE (t) == TYPE_CODE_UNION)
+    {
+      int i, n = TYPE_NFIELDS (t);
+
+      /* If all elements of the union are scalar, then the union is scalar.  */
+      for (i = 0; i < n; i++)
+	if (!is_scalar_type_recursive (TYPE_FIELD_TYPE (t, i)))
+	  return 0;
+
+      return 1;
+    }
+
+  return 0;
+}
+
 /* A helper function which returns true if types A and B represent the
    "same" class type.  This is true if the types have the same main
    type, or the same name.  */
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 1ce2d91..3d27bb4 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1475,6 +1475,8 @@ extern int can_dereference (struct type *);
 
 extern int is_integral_type (struct type *);
 
+extern int is_scalar_type_recursive (struct type *);
+
 extern void maintenance_print_type (char *, int);
 
 extern htab_t create_copied_types_hash (struct objfile *objfile);
diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 18ddee7..7c853a9 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1489,9 +1489,16 @@ ppc_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
      to determine the hardcoded watchable region for watchpoints.  */
   if (have_ptrace_booke_interface ())
     {
-      if (booke_debug_info.data_bp_alignment
-	  && (addr + len > (addr & ~(booke_debug_info.data_bp_alignment - 1))
-	      + booke_debug_info.data_bp_alignment))
+      /* DAC-based processors (i.e., embedded processors), like the PowerPC 440
+	 have ranged watchpoints and can watch any access within an arbitrary
+	 memory region.  This is useful to watch arrays and structs, for
+	 instance.  It takes two hardware watchpoints though.  */
+      if (len > 1
+	  && booke_debug_info.features & PPC_DEBUG_FEATURE_DATA_BP_RANGE)
+	return 2;
+      else if (booke_debug_info.data_bp_alignment
+	       && (addr + len > (addr & ~(booke_debug_info.data_bp_alignment - 1))
+		   + booke_debug_info.data_bp_alignment))
 	return 0;
     }
   /* addr+len must fall in the 8 byte watchable region for DABR-based
@@ -1878,6 +1885,51 @@ ppc_linux_can_accel_watchpoint_condition (CORE_ADDR addr, int len, int rw,
 	  && check_condition (addr, cond, &data_value));
 }
 
+/* Set up P with the parameters necessary to request a watchpoint covering
+   LEN bytes starting at ADDR and if possible with condition expression COND
+   evaluated by hardware.  */
+
+static void
+create_watchpoint_request (struct ppc_hw_breakpoint *p, CORE_ADDR addr,
+			   int len, int rw, struct expression *cond)
+{
+  if (len == 1)
+    {
+      CORE_ADDR data_value;
+
+      if (cond && can_use_watchpoint_cond_accel ()
+	  && check_condition (addr, cond, &data_value))
+	calculate_dvc (addr, len, data_value, &p->condition_mode,
+		       &p->condition_value);
+      else
+	{
+	  p->condition_mode = PPC_BREAKPOINT_CONDITION_NONE;
+	  p->condition_value = 0;
+	}
+
+      p->addr_mode = PPC_BREAKPOINT_MODE_EXACT;
+      p->addr2 = 0;
+    }
+  else
+    {
+      p->addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
+      p->condition_mode = PPC_BREAKPOINT_CONDITION_NONE;
+      p->condition_value = 0;
+
+      /* The watchpoint will trigger if the address of the memory access is
+	 within the defined range, as follows: p->addr <= address < p->addr2.
+
+	 Note that the above sentence just documents how ptrace interprets
+	 its arguments; the watchpoint is set to watch the range defined by
+	 the user _inclusively_, as specified by the user interface.  */
+      p->addr2 = (uint64_t) addr + len;
+    }
+
+  p->version = PPC_DEBUG_CURRENT_VERSION;
+  p->trigger_type = get_trigger_type (rw);
+  p->addr = (uint64_t) addr;
+}
+
 static int
 ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw,
 			     struct expression *cond)
@@ -1889,23 +1941,8 @@ ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw,
   if (have_ptrace_booke_interface ())
     {
       struct ppc_hw_breakpoint p;
-      CORE_ADDR data_value;
 
-      if (cond && can_use_watchpoint_cond_accel ()
-	  && check_condition (addr, cond, &data_value))
-	calculate_dvc (addr, len, data_value, &p.condition_mode,
-		       &p.condition_value);
-      else
-	{
-	  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
-	  p.condition_value = 0;
-	}
-
-      p.version         = PPC_DEBUG_CURRENT_VERSION;
-      p.trigger_type    = get_trigger_type (rw);
-      p.addr_mode       = PPC_BREAKPOINT_MODE_EXACT;
-      p.addr            = (uint64_t) addr;
-      p.addr2           = 0;
+      create_watchpoint_request (&p, addr, len, rw, cond);
 
       ALL_LWPS (lp, ptid)
 	booke_insert_point (&p, TIDGET (ptid));
@@ -1973,23 +2010,8 @@ ppc_linux_remove_watchpoint (CORE_ADDR addr, int len, int rw,
   if (have_ptrace_booke_interface ())
     {
       struct ppc_hw_breakpoint p;
-      CORE_ADDR data_value;
-
-      if (cond && booke_debug_info.num_condition_regs > 0
-	  && check_condition (addr, cond, &data_value))
-	calculate_dvc (addr, len, data_value, &p.condition_mode,
-		       &p.condition_value);
-      else
-	{
-	  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
-	  p.condition_value = 0;
-	}
 
-      p.version         = PPC_DEBUG_CURRENT_VERSION;
-      p.trigger_type    = get_trigger_type (rw);
-      p.addr_mode       = PPC_BREAKPOINT_MODE_EXACT;
-      p.addr            = (uint64_t) addr;
-      p.addr2           = 0;
+      create_watchpoint_request (&p, addr, len, rw, cond);
 
       ALL_LWPS (lp, ptid)
 	booke_remove_point (&p, TIDGET (ptid));
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 81a99b6..1152d04 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -4177,6 +4177,15 @@ powerpc_set_vector_abi (char *args, int from_tty,
     internal_error (__FILE__, __LINE__, "could not update architecture");
 }
 
+static void
+show_powerpc_exact_watchpoints (struct ui_file *file, int from_tty,
+				struct cmd_list_element *c,
+				const char *value)
+{
+  fprintf_filtered (file, _("\
+Use of just one debug register per watchpoint is %s.\n"), value);
+}
+
 /* Initialization code.  */
 
 extern initialize_file_ftype _initialize_rs6000_tdep; /* -Wmissing-prototypes */
@@ -4233,4 +4242,17 @@ _initialize_rs6000_tdep (void)
 			_("Show the vector ABI."),
 			NULL, powerpc_set_vector_abi, NULL,
 			&setpowerpccmdlist, &showpowerpccmdlist);
+
+  add_setshow_boolean_cmd ("exact-watchpoints", class_support,
+			   &target_exact_watchpoints,
+			   _("\
+Set whether to use just one debug register per watchpoint."),
+			   _("\
+Show whether to use just one debug register per watchpoint."),
+			   _("\
+If true, GDB will use only one debug register when watching a variable of\n\
+scalar type, thus assuming that all acesses that modify that variable happen\n\
+at its starting address."),
+			   NULL, show_powerpc_exact_watchpoints,
+			   &setpowerpccmdlist, &showpowerpccmdlist);
 }
diff --git a/gdb/target.h b/gdb/target.h
index bc70107..2ef5c56 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -452,7 +452,11 @@ struct target_ops
     int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *);
     int (*to_watchpoint_addr_within_range) (struct target_ops *,
 					    CORE_ADDR, CORE_ADDR, int);
+
+    /* Documentation of this routine is provided with the corresponding
+       target_* macro.  */
     int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR, int);
+
     int (*to_can_accel_watchpoint_condition) (CORE_ADDR, int, int,
 					      struct expression *);
     void (*to_terminal_init) (void);
@@ -1313,6 +1317,9 @@ extern char *normal_pid_to_str (ptid_t ptid);
 #define target_can_use_hardware_watchpoint(TYPE,CNT,OTHERTYPE) \
  (*current_target.to_can_use_hw_breakpoint) (TYPE, CNT, OTHERTYPE);
 
+/* Returns the number of debug registers needed to watch the given
+   memory region, or zero if not supported.  */
+
 #define target_region_ok_for_hw_watchpoint(addr, len) \
     (*current_target.to_region_ok_for_hw_watchpoint) (addr, len)
 

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Eli Zaretskii]

> From: Thiago Jung Bauermann <bauerman@br.ibm.com>
> Cc: gdb-patches@sourceware.org, Jan Kratochvil <jan.kratochvil@redhat.com>,
>         Joel Brobecker <brobecker@adacore.com>, Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 23 Dec 2010 16:49:42 -0200
> 
> +When running on PowerPC embedded processors @value{GDBN} automatically uses
                                              ^
Comma here, please.

> +ranged hardware watchpoints unless the @code{exact-watchpoints} option is on,
                              ^
And here.

> +@item set powerpc exact-watchpoints
> +@itemx show powerpc exact-watchpoints
> +Allow @value{GDBN} to use only one debug register when watching a variable
> +of scalar type, thus assuming that all acesses that modify that variable
> +happen at its starting address.

I suggest a slight rewording of the last sentence:

  ... thus assuming that the variable is accessed through the address
  of its first byte.

Is that wording accurate?  If it is, the patch for the manual is okay
with those changes.

Thanks.

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Thiago Jung Bauermann]

Hi Eli,

On Thu, 2010-12-23 at 21:04 +0200, Eli Zaretskii wrote:
> > From: Thiago Jung Bauermann <bauerman@br.ibm.com>
> > Cc: gdb-patches@sourceware.org, Jan Kratochvil <jan.kratochvil@redhat.com>,
> >         Joel Brobecker <brobecker@adacore.com>, Eli Zaretskii <eliz@gnu.org>
> > Date: Thu, 23 Dec 2010 16:49:42 -0200
> > 
> > +When running on PowerPC embedded processors @value{GDBN} automatically uses
>                                               ^
> Comma here, please.

Fixed.

> > +ranged hardware watchpoints unless the @code{exact-watchpoints} option is on,
>                               ^
> And here.

Fixed

> > +@item set powerpc exact-watchpoints
> > +@itemx show powerpc exact-watchpoints
> > +Allow @value{GDBN} to use only one debug register when watching a variable
> > +of scalar type, thus assuming that all acesses that modify that variable
> > +happen at its starting address.
> 
> I suggest a slight rewording of the last sentence:
> 
>   ... thus assuming that the variable is accessed through the address
>   of its first byte.
> 
> Is that wording accurate?  If it is, the patch for the manual is okay
> with those changes.

Yes, it is accurate. Adopted your wording. Thanks for the super-quick
review!
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Pedro Alves]

I think this looks great.  Thanks a million for following through!

The resource accounting bits, I still say that they should all just
go away (at some point), and gdb should just try to insert the
watchpoint immediately, and see if the target refuses.  E.g., how could
one sanely implement the accounting for remote targets?  The target
is free to do all sorts of smart merging, and resource reusing, and
in fact, x86 gdbserver does so (which is why the x86 and most
other ports don't actually make use of the resource accounting
interfaces, they just always accept watchpoints).

A few directed comments only:

On Thursday 23 December 2010 18:49:42, Thiago Jung Bauermann wrote:
> +int
> +is_scalar_type_recursive (struct type *t)
> +{
> +  CHECK_TYPEDEF (t);
> +
> +  if (is_scalar_type (t))
> +    return 1;
> +  else if ((TYPE_CODE (t) == TYPE_CODE_ARRAY
> +           || TYPE_CODE (t) == TYPE_CODE_STRING) && TYPE_NFIELDS (t) == 1
> +          && TYPE_CODE (TYPE_INDEX_TYPE (t)) == TYPE_CODE_RANGE)
> +    {
> +      LONGEST low_bound, high_bound;
> +      struct type *elt_type = check_typedef (TYPE_TARGET_TYPE (t));
> +
> +      get_discrete_bounds (TYPE_INDEX_TYPE (t), &low_bound, &high_bound);
> +
> +      return high_bound == low_bound && is_scalar_type_recursive (elt_type);
> +    }

What does the "TYPE_NFIELDS (t) == 1" do ?

I think you're missing at least pointers and references.  These should
also get the "exact" treatment, no?  Take a look at valprint.c:scalar_type_p.

Could you please add small describing comments above these new
functions?  ("return true if type T is a blah, blah".)

> +static void
> +show_powerpc_exact_watchpoints (struct ui_file *file, int from_tty,
> +                               struct cmd_list_element *c,
> +                               const char *value)
> +{
> +  fprintf_filtered (file, _("\
> +Use of just one debug register per watchpoint is %s.\n"), value);
> +}
> +

Should be "Use of exact watchpoints is ...", I think?  Thus getting rid
of the small lie that not all watchpoints (in the user perspective) will
use just one debug register, leaving the just one debug register
issue explanation to the help string (as you already have).

-- 
Pedro Alves

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Joel Brobecker]

> The resource accounting bits, I still say that they should all just
> go away (at some point), and gdb should just try to insert the
> watchpoint immediately, and see if the target refuses.  E.g., how could
> one sanely implement the accounting for remote targets?  The target
> is free to do all sorts of smart merging, and resource reusing, and
> in fact, x86 gdbserver does so (which is why the x86 and most
> other ports don't actually make use of the resource accounting
> interfaces, they just always accept watchpoints).

This sounds like a great idea to me!

And Thank You, Pedro, for handling this series of patches. You are
much more thorough and knowledgeable in that area than I could ever be.
Congrats to Thiago for seeing this through...

-- 
Joel

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Thu, 23 Dec 2010 20:16:45 +0000
> Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>,
>  Jan Kratochvil <jan.kratochvil@redhat.com>,
>  Joel Brobecker <brobecker@adacore.com>,
>  Eli Zaretskii <eliz@gnu.org>
> 
> The resource accounting bits, I still say that they should all just
> go away (at some point), and gdb should just try to insert the
> watchpoint immediately, and see if the target refuses.

I agree -- assuming, that is, that all the targets we support that can
do hardware watchpoints allow us to insert the watchpoints separately
from resuming the debuggee.  If there are targets that actually insert
the watchpoints as part as resuming the debuggee, those targets will
be unable to tell us anything useful about the watchpoint resources
until we actually resume the debuggee.  For those targets, we will
need to have some resource accounting in GDB, to give the user
reasonable feedback about resource over-booking.

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Thiago Jung Bauermann]

On Fri, 2010-12-24 at 13:25 +0200, Eli Zaretskii wrote:
> > From: Pedro Alves <pedro@codesourcery.com>
> > Date: Thu, 23 Dec 2010 20:16:45 +0000
> > Cc: Thiago Jung Bauermann <bauerman@br.ibm.com>,
> >  Jan Kratochvil <jan.kratochvil@redhat.com>,
> >  Joel Brobecker <brobecker@adacore.com>,
> >  Eli Zaretskii <eliz@gnu.org>
> > 
> > The resource accounting bits, I still say that they should all just
> > go away (at some point), and gdb should just try to insert the
> > watchpoint immediately, and see if the target refuses.
> 
> I agree -- assuming, that is, that all the targets we support that can
> do hardware watchpoints allow us to insert the watchpoints separately
> from resuming the debuggee.  If there are targets that actually insert
> the watchpoints as part as resuming the debuggee, those targets will
> be unable to tell us anything useful about the watchpoint resources
> until we actually resume the debuggee.  For those targets, we will
> need to have some resource accounting in GDB, to give the user
> reasonable feedback about resource over-booking.

AFAIK breakpoint.c would have to be changed to distinguish between
software breakpoints/watchpoints and hardware breakpoints/watchpoints,
and install the latter at the moment the user creates it, and the former
would continue to be installed at the time GDB resumes the inferior, as
breakpoint.c works now.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Thiago Jung Bauermann]

On Thu, 2010-12-23 at 20:16 +0000, Pedro Alves wrote:
> I think this looks great.  Thanks a million for following through!

Awesome! Thanks for the review.

> On Thursday 23 December 2010 18:49:42, Thiago Jung Bauermann wrote:
> > +int
> > +is_scalar_type_recursive (struct type *t)
> > +{
> > +  CHECK_TYPEDEF (t);
> > +
> > +  if (is_scalar_type (t))
> > +    return 1;
> > +  else if ((TYPE_CODE (t) == TYPE_CODE_ARRAY
> > +           || TYPE_CODE (t) == TYPE_CODE_STRING) && TYPE_NFIELDS (t) == 1
> > +          && TYPE_CODE (TYPE_INDEX_TYPE (t)) == TYPE_CODE_RANGE)
> > +    {
> > +      LONGEST low_bound, high_bound;
> > +      struct type *elt_type = check_typedef (TYPE_TARGET_TYPE (t));
> > +
> > +      get_discrete_bounds (TYPE_INDEX_TYPE (t), &low_bound, &high_bound);
> > +
> > +      return high_bound == low_bound && is_scalar_type_recursive (elt_type);
> > +    }
> 
> What does the "TYPE_NFIELDS (t) == 1" do ?

TYPE_NFIELDS (t) == 1 && TYPE_CODE (TYPE_INDEX_TYPE (t)) == TYPE_CODE_RANGE
is a way to determine that the given array type has known bounds.

> I think you're missing at least pointers and references.  These should
> also get the "exact" treatment, no?  Take a look at valprint.c:scalar_type_p.

There are different places in GDB which classify types as scalars and
non-scalars. Some list explicitly the scalar types and everything else
is non-scalar, and some (like scalar_type_p_ do the opposite. I thought
that the former approach was more conservative.

But I didn't know about scalar_type_p and since there's a function
that's used in a central place like val_print, then I can just use it.
This new version of the patch renames scalar_type_p to is_scalar_type
(to match is_integral_type) and moves it to gdbtypes.c.

> Could you please add small describing comments above these new
> functions?  ("return true if type T is a blah, blah".)

I added:

/* Return true if TYPE is scalar.  */                                          
  
int
is_scalar_type (struct type *type)

and

/* Return true if T is scalar, or a composite type which in practice has
   the memory layout of a scalar type. E.g., an array or struct with only one
   scalar element inside it, or a union with only scalar elements.  */

int
is_scalar_type_recursive (struct type *t)

> > +static void
> > +show_powerpc_exact_watchpoints (struct ui_file *file, int from_tty,
> > +                               struct cmd_list_element *c,
> > +                               const char *value)
> > +{
> > +  fprintf_filtered (file, _("\
> > +Use of just one debug register per watchpoint is %s.\n"), value);
> > +}
> > +
> 
> Should be "Use of exact watchpoints is ...", I think?  Thus getting rid
> of the small lie that not all watchpoints (in the user perspective) will
> use just one debug register, leaving the just one debug register
> issue explanation to the help string (as you already have).

Good point. Changed to:

static void
show_powerpc_exact_watchpoints (struct ui_file *file, int from_tty,
                                struct cmd_list_element *c,
                                const char *value)
{
  fprintf_filtered (file, _("Use of exact watchpoints is %s.\n"), value);
}

Also the apropos strings are now:

                           _("\
Set whether to use just one debug register for watchpoints on scalars."),
                           _("\
Show whether to use just one debug register for watchpoints on scalars."),

Ok?
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2010-12-27  Sergio Durigan Junior  <sergiodj@linux.vnet.ibm.com>
	    Thiago Jung Bauermann  <bauerman@br.ibm.com>

	Implement support for PowerPC BookE ranged watchpoints.

gdb/
	* breakpoint.h 
	(struct breakpoint_ops) <extra_resources_needed>: New methods
	Initialize to NULL in all existing breakpoint_ops instances.
	(struct breakpoint) <exact>: New field.
	(target_exact_watchpoints): Declare external global.
	* breakpoint.c (target_exact_watchpoints): New global flag.
	(update_watchpoint): Call breakpoint's
	breakpoint_ops.extra_resources_needed if available.
	(hw_watchpoint_used_count): Call breakpoint's
	breakpoint_ops.extra_resources_needed if available.  Change to
	count number of bp_locations, instead of number of high-level
	breakpoints.
	(insert_watchpoint, remove_watchpoint): Use fixed length of 1 byte
	if the watchpoint is exact.
	(extra_resources_needed_watchpoint): New function.
	(watchpoint_breakpoint_ops): Add extra_resources_needed_watchpoint.
	(watch_command_1): Set b->exact if the user asked for an exact
	watchpoint and one can be set.
	(can_use_hardware_watchpoint): Accept exact_watchpoints argument.
	Pass fixed length of 1 to target_region_ok_for_hw_watchpoint if
	the user asks for an exact watchpoint and one can be set.  Return
	number of needed debug registers to watch the expression.
	* gdbtypes.c (is_scalar_type): Move and rename from
	valprint.c:scalar_type_p.  Update callers.
	(is_scalar_type_recursive): New function.
	* gdbtypes.h (is_scalar_type, is_scalar_type_recursive): Declare.
	* ppc-linux-nat.c (ppc_linux_region_ok_for_hw_watchpoint): Always
	handle regions when ranged watchpoints are available.
	(create_watchpoint_request): New function.
	(ppc_linux_insert_watchpoint, ppc_linux_remove_watchpoint): Use
	create_watchpoint_request.
	* rs6000-tdep.c (show_powerpc_exact_watchpoints): New function.
	(_initialize_rs6000_tdep): Add `exact-watchpoints' boolean to the
	`set powerpc' and `show powerpc' commands.
	* target.h (struct target_ops) <to_region_ok_for_hw_watchpoint>:
	Mention documentation comment in the target macro.
	(target_region_ok_for_hw_watchpoint): Document return value.
	* valprint.c (scalar_type_p): Move and rename to
	gdbtypes.c:is_scalar_type.

gdb/doc/
	* gdb.texinfo (PowerPC Embedded): Document ranged watchpoints and
	the "set powerpc exact-watchpoints" flag.


diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 7ea01c7..3a2a266 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -10821,6 +10821,7 @@ static struct breakpoint_ops catch_exception_breakpoint_ops =
   NULL, /* insert */
   NULL, /* remove */
   NULL, /* breakpoint_hit */
+  NULL, /* extra_resources_needed */
   print_it_catch_exception,
   print_one_catch_exception,
   print_mention_catch_exception,
@@ -10859,6 +10860,7 @@ static struct breakpoint_ops catch_exception_unhandled_breakpoint_ops = {
   NULL, /* insert */
   NULL, /* remove */
   NULL, /* breakpoint_hit */
+  NULL, /* extra_resources_needed */
   print_it_catch_exception_unhandled,
   print_one_catch_exception_unhandled,
   print_mention_catch_exception_unhandled,
@@ -10895,6 +10897,7 @@ static struct breakpoint_ops catch_assert_breakpoint_ops = {
   NULL, /* insert */
   NULL, /* remove */
   NULL, /* breakpoint_hit */
+  NULL, /* extra_resources_needed */
   print_it_catch_assert,
   print_one_catch_assert,
   print_mention_catch_assert,
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 003899c..9c10b30 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -98,7 +98,7 @@ static void clear_command (char *, int);
 
 static void catch_command (char *, int);
 
-static int can_use_hardware_watchpoint (struct value *);
+static int can_use_hardware_watchpoint (struct value *, int);
 
 static void break_command_1 (char *, int, int);
 
@@ -345,6 +345,9 @@ static int executing_breakpoint_commands;
 /* Are overlay event breakpoints enabled? */
 static int overlay_events_enabled;
 
+/* See description in breakpoint.h. */
+int target_exact_watchpoints = 0;
+
 /* Walk the following statement or block through all breakpoints.
    ALL_BREAKPOINTS_SAFE does so even if the statment deletes the current
    breakpoint.  */
@@ -1394,25 +1397,33 @@ update_watchpoint (struct breakpoint *b, int reparse)
 	if ((b->type == bp_watchpoint || b->type == bp_hardware_watchpoint)
 	    && reparse)
 	  {
-	    int i, mem_cnt, other_type_used;
-
-	    /* We need to determine how many resources are already used
-	       for all other hardware watchpoints to see if we still have
-	       enough resources to also fit this watchpoint in as well.
-	       To avoid the hw_watchpoint_used_count call below from counting
-	       this watchpoint, make sure that it is marked as a software
-	       watchpoint.  */
-	    b->type = bp_watchpoint;
-	    i = hw_watchpoint_used_count (bp_hardware_watchpoint,
-					  &other_type_used);
-	    mem_cnt = can_use_hardware_watchpoint (val_chain);
-
-	    if (!mem_cnt)
+	    int reg_cnt;
+
+	    reg_cnt = can_use_hardware_watchpoint (val_chain, b->exact);
+
+	    if (!reg_cnt)
 	      b->type = bp_watchpoint;
 	    else
 	      {
-		int target_resources_ok = target_can_use_hardware_watchpoint
-		  (bp_hardware_watchpoint, i + mem_cnt, other_type_used);
+		int i, other_type_used, target_resources_ok;
+		enum bptype orig_type;
+
+		if (b->ops && b->ops->extra_resources_needed)
+		  reg_cnt += b->ops->extra_resources_needed (b, NULL);
+
+		/* We need to determine how many resources are already used
+		   for all other hardware watchpoints to see if we still have
+		   enough resources to also fit this watchpoint in as well.
+		   To avoid the hw_watchpoint_used_count call below from
+		   counting this watchpoint, make sure that it is marked as a
+		   software watchpoint.  */
+		orig_type = b->type;
+		b->type = bp_watchpoint;
+		i = hw_watchpoint_used_count (bp_hardware_watchpoint,
+					      &other_type_used);
+
+		target_resources_ok = target_can_use_hardware_watchpoint
+		  (bp_hardware_watchpoint, i + reg_cnt, other_type_used)
 		if (target_resources_ok <= 0)
 		  b->type = bp_watchpoint;
 		else
@@ -6004,6 +6015,7 @@ static struct breakpoint_ops catch_fork_breakpoint_ops =
   insert_catch_fork,
   remove_catch_fork,
   breakpoint_hit_catch_fork,
+  NULL, /* extra_resources_needed */
   print_it_catch_fork,
   print_one_catch_fork,
   print_mention_catch_fork,
@@ -6095,6 +6107,7 @@ static struct breakpoint_ops catch_vfork_breakpoint_ops =
   insert_catch_vfork,
   remove_catch_vfork,
   breakpoint_hit_catch_vfork,
+  NULL, /* extra_resources_needed */
   print_it_catch_vfork,
   print_one_catch_vfork,
   print_mention_catch_vfork,
@@ -6376,6 +6389,7 @@ static struct breakpoint_ops catch_syscall_breakpoint_ops =
   insert_catch_syscall,
   remove_catch_syscall,
   breakpoint_hit_catch_syscall,
+  NULL, /* extra_resources_needed */
   print_it_catch_syscall,
   print_one_catch_syscall,
   print_mention_catch_syscall,
@@ -6529,6 +6543,7 @@ static struct breakpoint_ops catch_exec_breakpoint_ops =
   insert_catch_exec,
   remove_catch_exec,
   breakpoint_hit_catch_exec,
+  NULL, /* extra_resources_needed */
   print_it_catch_exec,
   print_one_catch_exec,
   print_mention_catch_exec,
@@ -6570,19 +6585,29 @@ static int
 hw_watchpoint_used_count (enum bptype type, int *other_type_used)
 {
   struct breakpoint *b;
+  struct bp_location *bl;
   int i = 0;
 
   *other_type_used = 0;
   ALL_BREAKPOINTS (b)
-  {
-    if (breakpoint_enabled (b))
-      {
+    {
+      if (!breakpoint_enabled (b))
+	continue;
+
 	if (b->type == type)
-	  i++;
+	  for (bl = b->loc; bl; bl = bl->next)
+	    {
+	      i++;
+
+	      /* Special types of hardware watchpoints may use more than
+		 one register.  */
+	      if (b->ops && b->ops->extra_resources_needed)
+		i += b->ops->extra_resources_needed (b, bl);
+	    }
 	else if (is_hardware_watchpoint (b))
 	  *other_type_used = 1;
-      }
-  }
+    }
+
   return i;
 }
 
@@ -8119,8 +8144,10 @@ watchpoint_exp_is_const (const struct expression *exp)
 static int
 insert_watchpoint (struct bp_location *bl)
 {
-  return target_insert_watchpoint (bl->address, bl->length,
-				   bl->watchpoint_type, bl->owner->cond_exp);
+  int length = bl->owner->exact? 1 : bl->length;
+
+  return target_insert_watchpoint (bl->address, length, bl->watchpoint_type,
+				   bl->owner->cond_exp);
 }
 
 /* Implement the "remove" breakpoint_ops method for hardware watchpoints.  */
@@ -8128,8 +8155,31 @@ insert_watchpoint (struct bp_location *bl)
 static int
 remove_watchpoint (struct bp_location *bl)
 {
-  return target_remove_watchpoint (bl->address, bl->length,
-				   bl->watchpoint_type, bl->owner->cond_exp);
+  int length = bl->owner->exact? 1 : bl->length;
+
+  return target_remove_watchpoint (bl->address, length, bl->watchpoint_type,
+				   bl->owner->cond_exp);
+}
+
+/* Implement the "extra_resources_needed" breakpoint_ops method for
+   hardware watchpoints.  */
+
+static int
+extra_resources_needed_watchpoint (const struct breakpoint *b,
+				   const struct bp_location *bl)
+{
+  if (bl)
+    {
+      int ret, length = bl->owner->exact? 1 : bl->length;
+
+      ret = target_region_ok_for_hw_watchpoint (bl->address, length);
+
+      gdb_assert (ret > 0);
+
+      return ret - 1;
+    }
+  else
+    return 0;
 }
 
 /* The breakpoint_ops structure to be used in hardware watchpoints.  */
@@ -8139,6 +8189,7 @@ static struct breakpoint_ops watchpoint_breakpoint_ops =
   insert_watchpoint,
   remove_watchpoint,
   NULL, /* breakpoint_hit */
+  extra_resources_needed_watchpoint,
   NULL, /* print_it */
   NULL, /* print_one */
   NULL, /* print_mention */
@@ -8165,7 +8216,7 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
   char *cond_end = NULL;
   int i, other_type_used, target_resources_ok = 0;
   enum bptype bp_type;
-  int mem_cnt = 0;
+  int reg_cnt = 0;
   int thread = -1;
   int pc = 0;
 
@@ -8300,14 +8351,14 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
   else
     bp_type = bp_hardware_watchpoint;
 
-  mem_cnt = can_use_hardware_watchpoint (val);
-  if (mem_cnt == 0 && bp_type != bp_hardware_watchpoint)
+  reg_cnt = can_use_hardware_watchpoint (val, target_exact_watchpoints);
+  if (reg_cnt == 0 && bp_type != bp_hardware_watchpoint)
     error (_("Expression cannot be implemented with read/access watchpoint."));
-  if (mem_cnt != 0)
+  if (reg_cnt != 0)
     {
       i = hw_watchpoint_used_count (bp_type, &other_type_used);
       target_resources_ok = 
-	target_can_use_hardware_watchpoint (bp_type, i + mem_cnt, 
+	target_can_use_hardware_watchpoint (bp_type, i + reg_cnt,
 					    other_type_used);
       if (target_resources_ok == 0 && bp_type != bp_hardware_watchpoint)
 	error (_("Target does not support this type of hardware watchpoint."));
@@ -8318,7 +8369,7 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
 
   /* Change the type of breakpoint to an ordinary watchpoint if a hardware
      watchpoint could not be set.  */
-  if (!mem_cnt || target_resources_ok <= 0)
+  if (!reg_cnt || target_resources_ok <= 0)
     bp_type = bp_watchpoint;
 
   frame = block_innermost_frame (exp_valid_block);
@@ -8389,6 +8440,10 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
   b->val_valid = 1;
   b->ops = &watchpoint_breakpoint_ops;
 
+  /* Use an exact watchpoint only when there's one memory region to be
+     watched, which needs only one debug register.  */
+  b->exact = target_exact_watchpoints && reg_cnt == 1;
+
   if (cond_start)
     b->cond_string = savestring (cond_start, cond_end - cond_start);
   else
@@ -8428,12 +8483,15 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
   update_global_location_list (1);
 }
 
-/* Return count of locations need to be watched and can be handled
-   in hardware.  If the watchpoint can not be handled
-   in hardware return zero.  */
+/* Return count of debug registers need to watch the given expression.
+   If EXACT_WATCHPOINTS is 1, then consider that only the address of
+   the start of the watched region will be monitored (i.e., all accesses
+   will be aligned).  This uses less debug registers on some targets.
+
+   If the watchpoint can not be handled in hardware return zero.  */
 
 static int
-can_use_hardware_watchpoint (struct value *v)
+can_use_hardware_watchpoint (struct value *v, int exact_watchpoints)
 {
   int found_memory_cnt = 0;
   struct value *head = v;
@@ -8486,12 +8544,18 @@ can_use_hardware_watchpoint (struct value *v)
 		      && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
 		{
 		  CORE_ADDR vaddr = value_address (v);
-		  int       len   = TYPE_LENGTH (value_type (v));
+		  int len;
+		  int num_regs;
+
+		  len = (exact_watchpoints
+			 && is_scalar_type_recursive (vtype))?
+		    1 : TYPE_LENGTH (value_type (v));
 
-		  if (!target_region_ok_for_hw_watchpoint (vaddr, len))
+		  num_regs = target_region_ok_for_hw_watchpoint (vaddr, len);
+		  if (!num_regs)
 		    return 0;
 		  else
-		    found_memory_cnt++;
+		    found_memory_cnt += num_regs;
 		}
 	    }
 	}
@@ -8915,6 +8979,7 @@ static struct breakpoint_ops gnu_v3_exception_catchpoint_ops = {
   NULL, /* insert */
   NULL, /* remove */
   NULL, /* breakpoint_hit */
+  NULL, /* extra_resources_needed */
   print_exception_catchpoint,
   print_one_exception_catchpoint,
   print_mention_exception_catchpoint,
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 12c2df2..8e68532 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -372,6 +372,16 @@ struct breakpoint_ops
      breakpoint was hit.  */
   int (*breakpoint_hit) (struct breakpoint *);
 
+  /* Tell how many additional hardware resources (debug registers) are needed
+     for this breakpoint.  The bp_location argument may be NULL if the method
+     is called at a point where no location is assigned to the breakpoint yet.
+
+     We always count at least one resource, so if this function returns 1, then
+     GDB will consider that the breakpoint needes 2 registers.  If this element
+     is NULL, then no additional resources are needed.  */
+  int (*extra_resources_needed) (const struct breakpoint *,
+				 const struct bp_location *);
+
   /* The normal print routine for this breakpoint, called when we
      hit it.  */
   enum print_stop_action (*print_it) (struct breakpoint *);
@@ -411,6 +421,13 @@ DEF_VEC_P(bp_location_p);
    detail to the breakpoints module.  */
 struct counted_command_line;
 
+/* Some targets (eg, embedded PowerPC) need two debug registers to set
+   a watchpoint over a memory region.  If this flag is true, GDB will use
+   only one register per watchpoint, thus assuming that all acesses that
+   modify a memory location happen at its starting address. */
+
+extern int target_exact_watchpoints;
+
 /* 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
@@ -577,7 +594,10 @@ struct breakpoint
        can sometimes be NULL for enabled GDBs as not all breakpoint
        types are tracked by the Python scripting API.  */
     struct breakpoint_object *py_bp_object;
-};
+
+    /* Whether this watchpoint is exact (see target_exact_watchpoints).  */
+    int exact;
+  };
 
 typedef struct breakpoint *breakpoint_p;
 DEF_VEC_P(breakpoint_p);
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 0e1d553..b647c53 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -18728,9 +18728,25 @@ implement in hardware simple hardware watchpoint conditions of the form:
   if  @var{ADDRESS|VARIABLE} == @var{CONSTANT EXPRESSION}
 @end smallexample
 
-The DVC register will be automatically used whenever @value{GDBN} detects
-such pattern in a condition expression.  This feature is available in native
-@value{GDBN} running on a Linux kernel version 2.6.34 or newer.
+The DVC register will be automatically used when @value{GDBN} detects
+such pattern in a condition expression, and the created watchpoint uses one
+debug register (either the @code{exact-watchpoints} option is on and the
+variable is scalar, or the variable has a length of one byte).  This feature
+is available in native @value{GDBN} running on a Linux kernel version 2.6.34
+or newer.
+
+When running on PowerPC embedded processors, @value{GDBN} automatically uses
+ranged hardware watchpoints, unless the @code{exact-watchpoints} option is on,
+in which case watchpoints using only one debug register are created when
+watching variables of scalar types.
+
+You can create an artificial array to watch an arbitrary memory
+region using one of the following commands (@pxref{Expressions}):
+
+@smallexample
+(@value{GDBP}) watch *((char *) @var{ADDRESS})@@@var{LENGTH}
+(@value{GDBP}) watch @{char[@var{LENGTH}]@} @var{ADDRESS}
+@end smallexample
 
 @value{GDBN} provides the following PowerPC-specific commands:
 
@@ -18751,6 +18767,12 @@ arguments and return values.  The valid options are @samp{auto};
 registers.  By default, @value{GDBN} selects the calling convention
 based on the selected architecture and the provided executable file.
 
+@item set powerpc exact-watchpoints
+@itemx show powerpc exact-watchpoints
+Allow @value{GDBN} to use only one debug register when watching a variable
+of scalar type, thus assuming that the variable is accessed through the
+address of its first byte.
+
 @kindex target dink32
 @item target dink32 @var{dev}
 DINK32 ROM monitor.
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index b651098..f33b9b8 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1955,6 +1955,74 @@ is_integral_type (struct type *t)
 	 || (TYPE_CODE (t) == TYPE_CODE_BOOL)));
 }
 
+/* Return true if TYPE is scalar.  */
+
+int
+is_scalar_type (struct type *type)
+{
+  CHECK_TYPEDEF (type);
+
+  while (TYPE_CODE (type) == TYPE_CODE_REF)
+    {
+      type = TYPE_TARGET_TYPE (type);
+      CHECK_TYPEDEF (type);
+    }
+
+  switch (TYPE_CODE (type))
+    {
+    case TYPE_CODE_ARRAY:
+    case TYPE_CODE_STRUCT:
+    case TYPE_CODE_UNION:
+    case TYPE_CODE_SET:
+    case TYPE_CODE_STRING:
+    case TYPE_CODE_BITSTRING:
+      return 0;
+    default:
+      return 1;
+    }
+}
+
+/* Return true if T is scalar, or a composite type which in practice has
+   the memory layout of a scalar type. E.g., an array or struct with only one
+   scalar element inside it, or a union with only scalar elements.  */
+
+int
+is_scalar_type_recursive (struct type *t)
+{
+  CHECK_TYPEDEF (t);
+
+  if (is_scalar_type (t))
+    return 1;
+  /* Are we dealing with an array or string of known dimensions?  */
+  else if ((TYPE_CODE (t) == TYPE_CODE_ARRAY
+	    || TYPE_CODE (t) == TYPE_CODE_STRING) && TYPE_NFIELDS (t) == 1
+	   && TYPE_CODE (TYPE_INDEX_TYPE (t)) == TYPE_CODE_RANGE)
+    {
+      LONGEST low_bound, high_bound;
+      struct type *elt_type = check_typedef (TYPE_TARGET_TYPE (t));
+
+      get_discrete_bounds (TYPE_INDEX_TYPE (t), &low_bound, &high_bound);
+
+      return high_bound == low_bound && is_scalar_type_recursive (elt_type);
+    }
+  /* Are we dealing with a struct with one element?  */
+  else if (TYPE_CODE (t) == TYPE_CODE_STRUCT && TYPE_NFIELDS (t) == 1)
+    return is_scalar_type_recursive (TYPE_FIELD_TYPE (t, 0));
+  else if (TYPE_CODE (t) == TYPE_CODE_UNION)
+    {
+      int i, n = TYPE_NFIELDS (t);
+
+      /* If all elements of the union are scalar, then the union is scalar.  */
+      for (i = 0; i < n; i++)
+	if (!is_scalar_type_recursive (TYPE_FIELD_TYPE (t, i)))
+	  return 0;
+
+      return 1;
+    }
+
+  return 0;
+}
+
 /* A helper function which returns true if types A and B represent the
    "same" class type.  This is true if the types have the same main
    type, or the same name.  */
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 1ce2d91..f3534f5 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1475,6 +1475,10 @@ extern int can_dereference (struct type *);
 
 extern int is_integral_type (struct type *);
 
+extern int is_scalar_type (struct type *);
+
+extern int is_scalar_type_recursive (struct type *);
+
 extern void maintenance_print_type (char *, int);
 
 extern htab_t create_copied_types_hash (struct objfile *objfile);
diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 18ddee7..7c853a9 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1489,9 +1489,16 @@ ppc_linux_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
      to determine the hardcoded watchable region for watchpoints.  */
   if (have_ptrace_booke_interface ())
     {
-      if (booke_debug_info.data_bp_alignment
-	  && (addr + len > (addr & ~(booke_debug_info.data_bp_alignment - 1))
-	      + booke_debug_info.data_bp_alignment))
+      /* DAC-based processors (i.e., embedded processors), like the PowerPC 440
+	 have ranged watchpoints and can watch any access within an arbitrary
+	 memory region.  This is useful to watch arrays and structs, for
+	 instance.  It takes two hardware watchpoints though.  */
+      if (len > 1
+	  && booke_debug_info.features & PPC_DEBUG_FEATURE_DATA_BP_RANGE)
+	return 2;
+      else if (booke_debug_info.data_bp_alignment
+	       && (addr + len > (addr & ~(booke_debug_info.data_bp_alignment - 1))
+		   + booke_debug_info.data_bp_alignment))
 	return 0;
     }
   /* addr+len must fall in the 8 byte watchable region for DABR-based
@@ -1878,6 +1885,51 @@ ppc_linux_can_accel_watchpoint_condition (CORE_ADDR addr, int len, int rw,
 	  && check_condition (addr, cond, &data_value));
 }
 
+/* Set up P with the parameters necessary to request a watchpoint covering
+   LEN bytes starting at ADDR and if possible with condition expression COND
+   evaluated by hardware.  */
+
+static void
+create_watchpoint_request (struct ppc_hw_breakpoint *p, CORE_ADDR addr,
+			   int len, int rw, struct expression *cond)
+{
+  if (len == 1)
+    {
+      CORE_ADDR data_value;
+
+      if (cond && can_use_watchpoint_cond_accel ()
+	  && check_condition (addr, cond, &data_value))
+	calculate_dvc (addr, len, data_value, &p->condition_mode,
+		       &p->condition_value);
+      else
+	{
+	  p->condition_mode = PPC_BREAKPOINT_CONDITION_NONE;
+	  p->condition_value = 0;
+	}
+
+      p->addr_mode = PPC_BREAKPOINT_MODE_EXACT;
+      p->addr2 = 0;
+    }
+  else
+    {
+      p->addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
+      p->condition_mode = PPC_BREAKPOINT_CONDITION_NONE;
+      p->condition_value = 0;
+
+      /* The watchpoint will trigger if the address of the memory access is
+	 within the defined range, as follows: p->addr <= address < p->addr2.
+
+	 Note that the above sentence just documents how ptrace interprets
+	 its arguments; the watchpoint is set to watch the range defined by
+	 the user _inclusively_, as specified by the user interface.  */
+      p->addr2 = (uint64_t) addr + len;
+    }
+
+  p->version = PPC_DEBUG_CURRENT_VERSION;
+  p->trigger_type = get_trigger_type (rw);
+  p->addr = (uint64_t) addr;
+}
+
 static int
 ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw,
 			     struct expression *cond)
@@ -1889,23 +1941,8 @@ ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw,
   if (have_ptrace_booke_interface ())
     {
       struct ppc_hw_breakpoint p;
-      CORE_ADDR data_value;
 
-      if (cond && can_use_watchpoint_cond_accel ()
-	  && check_condition (addr, cond, &data_value))
-	calculate_dvc (addr, len, data_value, &p.condition_mode,
-		       &p.condition_value);
-      else
-	{
-	  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
-	  p.condition_value = 0;
-	}
-
-      p.version         = PPC_DEBUG_CURRENT_VERSION;
-      p.trigger_type    = get_trigger_type (rw);
-      p.addr_mode       = PPC_BREAKPOINT_MODE_EXACT;
-      p.addr            = (uint64_t) addr;
-      p.addr2           = 0;
+      create_watchpoint_request (&p, addr, len, rw, cond);
 
       ALL_LWPS (lp, ptid)
 	booke_insert_point (&p, TIDGET (ptid));
@@ -1973,23 +2010,8 @@ ppc_linux_remove_watchpoint (CORE_ADDR addr, int len, int rw,
   if (have_ptrace_booke_interface ())
     {
       struct ppc_hw_breakpoint p;
-      CORE_ADDR data_value;
-
-      if (cond && booke_debug_info.num_condition_regs > 0
-	  && check_condition (addr, cond, &data_value))
-	calculate_dvc (addr, len, data_value, &p.condition_mode,
-		       &p.condition_value);
-      else
-	{
-	  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
-	  p.condition_value = 0;
-	}
 
-      p.version         = PPC_DEBUG_CURRENT_VERSION;
-      p.trigger_type    = get_trigger_type (rw);
-      p.addr_mode       = PPC_BREAKPOINT_MODE_EXACT;
-      p.addr            = (uint64_t) addr;
-      p.addr2           = 0;
+      create_watchpoint_request (&p, addr, len, rw, cond);
 
       ALL_LWPS (lp, ptid)
 	booke_remove_point (&p, TIDGET (ptid));
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 81a99b6..f5f0f72 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -4177,6 +4177,14 @@ powerpc_set_vector_abi (char *args, int from_tty,
     internal_error (__FILE__, __LINE__, "could not update architecture");
 }
 
+static void
+show_powerpc_exact_watchpoints (struct ui_file *file, int from_tty,
+				struct cmd_list_element *c,
+				const char *value)
+{
+  fprintf_filtered (file, _("Use of exact watchpoints is %s.\n"), value);
+}
+
 /* Initialization code.  */
 
 extern initialize_file_ftype _initialize_rs6000_tdep; /* -Wmissing-prototypes */
@@ -4233,4 +4241,17 @@ _initialize_rs6000_tdep (void)
 			_("Show the vector ABI."),
 			NULL, powerpc_set_vector_abi, NULL,
 			&setpowerpccmdlist, &showpowerpccmdlist);
+
+  add_setshow_boolean_cmd ("exact-watchpoints", class_support,
+			   &target_exact_watchpoints,
+			   _("\
+Set whether to use just one debug register per watchpoint on scalars."),
+			   _("\
+Show whether to use just one debug register per watchpoint on scalars."),
+			   _("\
+If true, GDB will use only one debug register when watching a variable of\n\
+scalar type, thus assuming that the variable is accessed through the address\n\
+of its first byte."),
+			   NULL, show_powerpc_exact_watchpoints,
+			   &setpowerpccmdlist, &showpowerpccmdlist);
 }
diff --git a/gdb/target.h b/gdb/target.h
index bc70107..2ef5c56 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -452,7 +452,11 @@ struct target_ops
     int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *);
     int (*to_watchpoint_addr_within_range) (struct target_ops *,
 					    CORE_ADDR, CORE_ADDR, int);
+
+    /* Documentation of this routine is provided with the corresponding
+       target_* macro.  */
     int (*to_region_ok_for_hw_watchpoint) (CORE_ADDR, int);
+
     int (*to_can_accel_watchpoint_condition) (CORE_ADDR, int, int,
 					      struct expression *);
     void (*to_terminal_init) (void);
@@ -1313,6 +1317,9 @@ extern char *normal_pid_to_str (ptid_t ptid);
 #define target_can_use_hardware_watchpoint(TYPE,CNT,OTHERTYPE) \
  (*current_target.to_can_use_hw_breakpoint) (TYPE, CNT, OTHERTYPE);
 
+/* Returns the number of debug registers needed to watch the given
+   memory region, or zero if not supported.  */
+
 #define target_region_ok_for_hw_watchpoint(addr, len) \
     (*current_target.to_region_ok_for_hw_watchpoint) (addr, len)
 
diff --git a/gdb/valprint.c b/gdb/valprint.c
index 5cba023..804b2d2 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -218,33 +218,6 @@ show_addressprint (struct ui_file *file, int from_tty,
 }
 \f
 
-/* A helper function for val_print.  When printing in "summary" mode,
-   we want to print scalar arguments, but not aggregate arguments.
-   This function distinguishes between the two.  */
-
-static int
-scalar_type_p (struct type *type)
-{
-  CHECK_TYPEDEF (type);
-  while (TYPE_CODE (type) == TYPE_CODE_REF)
-    {
-      type = TYPE_TARGET_TYPE (type);
-      CHECK_TYPEDEF (type);
-    }
-  switch (TYPE_CODE (type))
-    {
-    case TYPE_CODE_ARRAY:
-    case TYPE_CODE_STRUCT:
-    case TYPE_CODE_UNION:
-    case TYPE_CODE_SET:
-    case TYPE_CODE_STRING:
-    case TYPE_CODE_BITSTRING:
-      return 0;
-    default:
-      return 1;
-    }
-}
-
 /* Helper function to check the validity of some bits of a value.
 
    If TYPE represents some aggregate type (e.g., a structure), return 1.
@@ -343,7 +316,7 @@ val_print (struct type *type, const gdb_byte *valaddr, int embedded_offset,
 
   /* Handle summary mode.  If the value is a scalar, print it;
      otherwise, print an ellipsis.  */
-  if (options->summary && !scalar_type_p (type))
+  if (options->summary && !is_scalar_type (type))
     {
       fprintf_filtered (stream, "...");
       return 0;

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Joel Brobecker]

> +/* Return true if TYPE is scalar.  */
> +
> +int
> +is_scalar_type (struct type *type)
> +{
> +  CHECK_TYPEDEF (type);
> +
> +  while (TYPE_CODE (type) == TYPE_CODE_REF)
> +    {
> +      type = TYPE_TARGET_TYPE (type);
> +      CHECK_TYPEDEF (type);
> +    }

I suggest you make it explicit how TYPE_CODE_REF types are handled.
In certain cases, I think it might be making a difference (a REF
to a struct might have a different classification than the struct
in terms of argument passing, for instance).

-- 
Joel

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Pedro Alves]

On Tuesday 28 December 2010 05:00:42, Joel Brobecker wrote:
> > +/* Return true if TYPE is scalar.  */
> > +
> > +int
> > +is_scalar_type (struct type *type)
> > +{
> > +  CHECK_TYPEDEF (type);
> > +
> > +  while (TYPE_CODE (type) == TYPE_CODE_REF)
> > +    {
> > +      type = TYPE_TARGET_TYPE (type);
> > +      CHECK_TYPEDEF (type);
> > +    }
> 
> I suggest you make it explicit how TYPE_CODE_REF types are handled.
> In certain cases, I think it might be making a difference (a REF
> to a struct might have a different classification than the struct
> in terms of argument passing, for instance).

Yeah.  For watching purposes, you want to consider the
reference, or really, its underlying pointer representation
as a scalar, nomatter what the reference is bound to.  Let me
try to clarify:

 int i;
 int &ref = i;

Currently, a watchpoint on "ref" only monitors changes
for the address stored in the reference-as-pointer.
So, for exact-watchpoint's purposes, when considering
individual pieces/locations of an expression to watch, the
TYPE_CODE_REF alone is a scalar, even if its target type
is a structure.
I've just tried "watch ref" per above, and I'm surprised, since
I thought a watchpoint on "ref" actually monitored two locations:
the underlying address stored in "ref"; and its target
value, similarly to watching '*ptr' in "int i; int *ptr= &i;".
I think we can actually call this a bug...

Contrast with valprint's purposes, where what matters
is the reference's target type, what the user sees.

-- 
Pedro Alves

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Pedro Alves]

On Monday 27 December 2010 21:19:03, Thiago Jung Bauermann wrote:

> @@ -1394,25 +1397,33 @@ update_watchpoint (struct breakpoint *b, int reparse)
>         if ((b->type == bp_watchpoint || b->type == bp_hardware_watchpoint)
>             && reparse)
>           {
> -           int i, mem_cnt, other_type_used;
> -
> -           /* We need to determine how many resources are already used
> -              for all other hardware watchpoints to see if we still have
> -              enough resources to also fit this watchpoint in as well.
> -              To avoid the hw_watchpoint_used_count call below from counting
> -              this watchpoint, make sure that it is marked as a software
> -              watchpoint.  */
> -           b->type = bp_watchpoint;
> -           i = hw_watchpoint_used_count (bp_hardware_watchpoint,
> -                                         &other_type_used);
> -           mem_cnt = can_use_hardware_watchpoint (val_chain);
> -
> -           if (!mem_cnt)
> +           int reg_cnt;
> +
> +           reg_cnt = can_use_hardware_watchpoint (val_chain, b->exact);
> +
> +           if (!reg_cnt)
>               b->type = bp_watchpoint;
>             else
>               {
> -               int target_resources_ok = target_can_use_hardware_watchpoint
> -                 (bp_hardware_watchpoint, i + mem_cnt, other_type_used);
> +               int i, other_type_used, target_resources_ok;
> +               enum bptype orig_type;
> +
> +               if (b->ops && b->ops->extra_resources_needed)
> +                 reg_cnt += b->ops->extra_resources_needed (b, NULL);

I'm taking a look at the resource accounting changes, which I
mostly ignored before, and I'm confused.  can_use_hardware_watchpoint
(above) calls target_region_ok_for_hw_watchpoint, which has been changed
to return the number of resources normally necessary.  Yet,
extra_resources_needed_watchpoint calls target_region_ok_for_hw_watchpoint
as well (discounting 1, it seems).  Aren't we ending up with gdb
thinking it needs more resources than are really necessary?

> +
> +               /* We need to determine how many resources are already used
> +                  for all other hardware watchpoints to see if we still have
> +                  enough resources to also fit this watchpoint in as well.
> +                  To avoid the hw_watchpoint_used_count call below from
> +                  counting this watchpoint, make sure that it is marked as a
> +                  software watchpoint.  */
> +               orig_type = b->type;
> +               b->type = bp_watchpoint;

Something I notice: It sounds like if you remove this hack above, 

> +               i = hw_watchpoint_used_count (bp_hardware_watchpoint,
> +                                             &other_type_used);

then this above accounts for the new watchpoint too, so...

> +
> +               target_resources_ok = target_can_use_hardware_watchpoint
> +                 (bp_hardware_watchpoint, i + reg_cnt, other_type_used)

... this would just be ", i,", instead of ", i + reg_cnt,".

>                 if (target_resources_ok <= 0)
>                   b->type = bp_watchpoint;
>                 else

> > What does the "TYPE_NFIELDS (t) == 1" do ?
> 
> TYPE_NFIELDS (t) == 1 && TYPE_CODE (TYPE_INDEX_TYPE (t)) == TYPE_CODE_RANGE
> is a way to determine that the given array type has known bounds.

Didn't know about that.  I thought TYPE_NFIELDS only made sense
for structs/classes/unions.  My grep foo isn't finding other similar uses.

I tried:

 compunit1.c:
 extern int array[];

 void foo ()
 {
 ...
 }

 compunit2.c:

 int array[];

and in foo, ptype array gave me array[0], TYPE_NFIELDS==1.

> > Should be "Use of exact watchpoints is ...", I think?  Thus getting rid
> > of the small lie that not all watchpoints (in the user perspective) will
> > use just one debug register, leaving the just one debug register
> > issue explanation to the help string (as you already have).
> 
> Good point. Changed to:

(...)

Thanks.

-- 
Pedro Alves

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Thiago Jung Bauermann]

On Tue, 2010-12-28 at 15:29 +0000, Pedro Alves wrote:
> On Monday 27 December 2010 21:19:03, Thiago Jung Bauermann wrote:
> 
> > @@ -1394,25 +1397,33 @@ update_watchpoint (struct breakpoint *b, int reparse)
> >         if ((b->type == bp_watchpoint || b->type == bp_hardware_watchpoint)
> >             && reparse)
> >           {
> > -           int i, mem_cnt, other_type_used;
> > -
> > -           /* We need to determine how many resources are already used
> > -              for all other hardware watchpoints to see if we still have
> > -              enough resources to also fit this watchpoint in as well.
> > -              To avoid the hw_watchpoint_used_count call below from counting
> > -              this watchpoint, make sure that it is marked as a software
> > -              watchpoint.  */
> > -           b->type = bp_watchpoint;
> > -           i = hw_watchpoint_used_count (bp_hardware_watchpoint,
> > -                                         &other_type_used);
> > -           mem_cnt = can_use_hardware_watchpoint (val_chain);
> > -
> > -           if (!mem_cnt)
> > +           int reg_cnt;
> > +
> > +           reg_cnt = can_use_hardware_watchpoint (val_chain, b->exact);
> > +
> > +           if (!reg_cnt)
> >               b->type = bp_watchpoint;
> >             else
> >               {
> > -               int target_resources_ok = target_can_use_hardware_watchpoint
> > -                 (bp_hardware_watchpoint, i + mem_cnt, other_type_used);
> > +               int i, other_type_used, target_resources_ok;
> > +               enum bptype orig_type;
> > +
> > +               if (b->ops && b->ops->extra_resources_needed)
> > +                 reg_cnt += b->ops->extra_resources_needed (b, NULL);
> 
> I'm taking a look at the resource accounting changes, which I
> mostly ignored before, and I'm confused.

I don't blame you. What I need is beyond what the current resource
accounting code in GDB is capable of doing, and I'm trying to extend it
in the least intrusive way possible. I don't think it's worth rewriting
it, especially after your hints that it should just be removed. This
leads to some hacks in my patch...

>   can_use_hardware_watchpoint
> (above) calls target_region_ok_for_hw_watchpoint, which has been changed
> to return the number of resources normally necessary.  Yet,
> extra_resources_needed_watchpoint calls target_region_ok_for_hw_watchpoint
> as well (discounting 1, it seems).  Aren't we ending up with gdb
> thinking it needs more resources than are really necessary?

What's going on here is this: extra_resources_needed_watchpoint is
called at two slightly different contexts:

In hw_watchpoint_used_count, where it is asked to answer about each
breakpoint location in all the existing watchpoints. This is easy, it
just calls target_region_ok_for_hw_watchpoint (discounting one because
of the convention where I make GDB assume that each bp_location consumes
one resource, so any additional ones are "extra" (I'm not very happy
about this convention, but it allows me not to rewrite the resource
accounting code)).

In update_watchpoint, where it needs to answer about the watchpoint
currently being examined by update_watchpoint. The problem here is that
the watchpoint doesn't have any bp_location yet. The bp_locations will
be created just a few moments later. So at this point
extra_resources_needed_watchpoint just says that no extra resources are
needed (because bl is NULL).

This hack is needed because the resource accounting code examines
high-level breakpoints to decide about resource utilization, but it
should be using breakpoint locations instead. To make it examine
breakpoint locations, the resource accounting functions would have to be
called at different places from where they are called now, where
changing from a hardware watchpoint to a software one, or refusing to
create a hardware breakpoint (for example) would be cumbersome, since
breakpoint locations are created late in the breakpoint/watchpoint
creation process.

It can be done, but it's a fair amount of work which may not be
worthwhile given that the current disposition is to get rid of this
mechanism altogether. Perhaps I should just change ppc-linux-nat.c to
always accept hardware breakpoints and watchpoints, and then just fail
later if it overcommitted (as other ports like i386 do)? Then I believe
this patch would be simpler in this regard.

> > +
> > +               /* We need to determine how many resources are already used
> > +                  for all other hardware watchpoints to see if we still have
> > +                  enough resources to also fit this watchpoint in as well.
> > +                  To avoid the hw_watchpoint_used_count call below from
> > +                  counting this watchpoint, make sure that it is marked as a
> > +                  software watchpoint.  */
> > +               orig_type = b->type;
> > +               b->type = bp_watchpoint;
> 
> Something I notice: It sounds like if you remove this hack above, 
> 
> > +               i = hw_watchpoint_used_count (bp_hardware_watchpoint,
> > +                                             &other_type_used);
> 
> then this above accounts for the new watchpoint too, so...
> 
> > +
> > +               target_resources_ok = target_can_use_hardware_watchpoint
> > +                 (bp_hardware_watchpoint, i + reg_cnt, other_type_used)
> 
> ... this would just be ", i,", instead of ", i + reg_cnt,".

It wouldn't be exactly the same. In this patch I changed
hw_watchpoint_used_count to iterate over the breakpoint locations
instead of the breakpoints (which is what actually makes sense). But
since the watchpoint currently being examined by update_watchpoint
doesn't have any location, then it wouldn't be accounted for.

I could change update_watchpoint to create the breakpoint locations
before doing the resource accounting so that I could remove the hacks
described here, but it was not a trivial change (but not magic either)
and I wondered whether it was worth it.

> >                 if (target_resources_ok <= 0)
> >                   b->type = bp_watchpoint;
> >                 else
> 
> > > What does the "TYPE_NFIELDS (t) == 1" do ?
> > 
> > TYPE_NFIELDS (t) == 1 && TYPE_CODE (TYPE_INDEX_TYPE (t)) == TYPE_CODE_RANGE
> > is a way to determine that the given array type has known bounds.
> 
> Didn't know about that.  I thought TYPE_NFIELDS only made sense
> for structs/classes/unions.  My grep foo isn't finding other similar uses.

I looked at different places in GDB which determined the array size, if
available, and there are different ways to do it. I chose to use the
code from c-lang.c:c_get_string, which I wrote. :-)

gdbtypes.c:create_array_type says:

  TYPE_NFIELDS (result_type) = 1;
  TYPE_FIELDS (result_type) =
    (struct field *) TYPE_ZALLOC (result_type, sizeof (struct field));
  TYPE_INDEX_TYPE (result_type) = range_type;

So I believe it is correct.

> I tried:
> 
>  compunit1.c:
>  extern int array[];
> 
>  void foo ()
>  {
>  ...
>  }
> 
>  compunit2.c:
> 
>  int array[];
> 
> and in foo, ptype array gave me array[0], TYPE_NFIELDS==1.

That information is wrong, since the array is actually of an incomplete
type. But it would not have been a problem for is_scalar_type_recursive
since it only considers arrays with exactly one element.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [patch 2/3] Implement support for PowerPC BookE ranged watchpoints

[Eli Zaretskii]

> From: Thiago Jung Bauermann <bauerman@br.ibm.com>
> Cc: Jan Kratochvil <jan.kratochvil@redhat.com>,
>         Joel Brobecker
>  <brobecker@adacore.com>, Eli Zaretskii <eliz@gnu.org>
> Date: Tue, 23 Nov 2010 19:51:40 -0200
> 
> The manual has the following new wording in the second half of the
> paragraph (the first half is the same):
> 
> +You can create an artificial array to watch an arbitrary memory
> +region using one of the following commands (@pxref{Expressions}):
> +
> +@smallexample
> +(@value{GDBP}) watch *((char *) @var{ADDRESS})@@@var{LENGTH}
> +(@value{GDBP}) watch @{char[@var{LENGTH}]@} @var{ADDRESS}
> +@end smallexample
> 
> Ok?

Yes, but please down-case all the text inside @var.  It is up-cased
automatically by makeinfo, but will look ugly in the printed version,
where the argument of @var is typeset as italics.