[commit] Re: [rfc, gdbserver] Support hardware watchpoints on ARM

["Ulrich Weigand" <uweigand@de.ibm.com>]

Pedro Alves wrote:
> On Wednesday 21 September 2011 15:26:30, Ulrich Weigand wrote:
> > Pedro Alves wrote:
> > > On Wednesday 21 September 2011 14:57:15, Ulrich Weigand wrote:
> > > > Well, since this is global system property that is actually only
> > > > queried once and then returned from a cache, adding a LWP argument
> > > > would appear to be somewhat misleading ...
> > > 
> > > We can always just document what the argument means :-)  In this
> > > case, it'd serve as currently stopped LWP to run ptrace on in case
> > > the cache is not set yet.
> > 
> > That still seems odd to me :-)  I'd rather make the caching explicit,
> > e.g. by retrieving the information once in arm_arch_setup and then
> > just always using it.
> 
> Works for me.  :-)

OK, so here's the patch I committed to address the two issues.

Re-tested with no regressions on arm-linux-gnueabi.

Bye,
Ulrich

ChangeLog:

	* linux-arm-low.c (struct arm_linux_hwbp_cap): Remove.
	(arm_linux_hwbp_cap): New static variable.
	(arm_linux_get_hwbp_cap): Replace by ...
	(arm_linux_init_hwbp_cap): ... this new function.
	(arm_linux_get_hw_breakpoint_count): Use arm_linux_hwbp_cap.
	(arm_linux_get_hw_watchpoint_count): Likewise.
	(arm_linux_get_hw_watchpoint_max_length): Likewise.
	(arm_arch_setup): Call arm_linux_init_hwbp_cap.
	(arm_prepare_to_resume): Use perror_with_name instead of error.


Index: gdb/gdbserver/linux-arm-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-arm-low.c,v
retrieving revision 1.28
diff -u -p -r1.28 linux-arm-low.c
--- gdb/gdbserver/linux-arm-low.c	21 Sep 2011 12:39:50 -0000	1.28
+++ gdb/gdbserver/linux-arm-low.c	21 Sep 2011 14:44:52 -0000
@@ -55,13 +55,13 @@ void init_registers_arm_with_neon (void)
 #endif
 
 /* Information describing the hardware breakpoint capabilities.  */
-struct arm_linux_hwbp_cap
+static struct
 {
   unsigned char arch;
   unsigned char max_wp_length;
   unsigned char wp_count;
   unsigned char bp_count;
-};
+} arm_linux_hwbp_cap;
 
 /* Enum describing the different types of ARM hardware break-/watch-points.  */
 typedef enum
@@ -341,72 +341,49 @@ ps_get_thread_area (const struct ps_proc
 }
 
 
-/* Get hold of the Hardware Breakpoint information for the target we are
-   attached to.  Returns NULL if the kernel doesn't support Hardware
-   breakpoints at all, or a pointer to the information structure.  */
-static const struct arm_linux_hwbp_cap *
-arm_linux_get_hwbp_cap (void)
-{
-  /* The info structure we return.  */
-  static struct arm_linux_hwbp_cap info;
-
-  /* Is INFO in a good state?  -1 means that no attempt has been made to
-     initialize INFO; 0 means an attempt has been made, but it failed; 1
-     means INFO is in an initialized state.  */
-  static int available = -1;
-
-  if (available == -1)
-    {
-      int pid = lwpid_of (get_thread_lwp (current_inferior));
-      unsigned int val;
+/* Query Hardware Breakpoint information for the target we are attached to
+   (using PID as ptrace argument) and set up arm_linux_hwbp_cap.  */
+static void
+arm_linux_init_hwbp_cap (int pid)
+{
+  unsigned int val;
 
-      if (ptrace (PTRACE_GETHBPREGS, pid, 0, &val) < 0)
-	available = 0;
-      else
-	{
-	  info.arch = (unsigned char)((val >> 24) & 0xff);
-	  info.max_wp_length = (unsigned char)((val >> 16) & 0xff);
-	  info.wp_count = (unsigned char)((val >> 8) & 0xff);
-	  info.bp_count = (unsigned char)(val & 0xff);
-	  available = (info.arch != 0);
-	}
+  if (ptrace (PTRACE_GETHBPREGS, pid, 0, &val) < 0)
+    return;
 
-      if (available)
-	{
-	  if (info.wp_count > MAX_WPTS)
-	    internal_error (__FILE__, __LINE__,
-			    "Unsupported number of watchpoints");
-	  if (info.bp_count > MAX_BPTS)
-	    internal_error (__FILE__, __LINE__,
-			    "Unsupported number of breakpoints");
-	}
-    }
+  arm_linux_hwbp_cap.arch = (unsigned char)((val >> 24) & 0xff);
+  if (arm_linux_hwbp_cap.arch == 0)
+    return;
 
-  return available == 1 ? &info : NULL;
+  arm_linux_hwbp_cap.max_wp_length = (unsigned char)((val >> 16) & 0xff);
+  arm_linux_hwbp_cap.wp_count = (unsigned char)((val >> 8) & 0xff);
+  arm_linux_hwbp_cap.bp_count = (unsigned char)(val & 0xff);
+
+  if (arm_linux_hwbp_cap.wp_count > MAX_WPTS)
+    internal_error (__FILE__, __LINE__, "Unsupported number of watchpoints");
+  if (arm_linux_hwbp_cap.bp_count > MAX_BPTS)
+    internal_error (__FILE__, __LINE__, "Unsupported number of breakpoints");
 }
 
 /* How many hardware breakpoints are available?  */
 static int
 arm_linux_get_hw_breakpoint_count (void)
 {
-  const struct arm_linux_hwbp_cap *cap = arm_linux_get_hwbp_cap ();
-  return cap != NULL ? cap->bp_count : 0;
+  return arm_linux_hwbp_cap.bp_count;
 }
 
 /* How many hardware watchpoints are available?  */
 static int
 arm_linux_get_hw_watchpoint_count (void)
 {
-  const struct arm_linux_hwbp_cap *cap = arm_linux_get_hwbp_cap ();
-  return cap != NULL ? cap->wp_count : 0;
+  return arm_linux_hwbp_cap.wp_count;
 }
 
 /* Maximum length of area watched by hardware watchpoint.  */
 static int
 arm_linux_get_hw_watchpoint_max_length (void)
 {
-  const struct arm_linux_hwbp_cap *cap = arm_linux_get_hwbp_cap ();
-  return cap != NULL ? cap->max_wp_length : 0;
+  return arm_linux_hwbp_cap.max_wp_length;
 }
 
 /* Initialize an ARM hardware break-/watch-point control register value.
@@ -735,12 +712,12 @@ arm_prepare_to_resume (struct lwp_info *
 	if (arm_hwbp_control_is_enabled (proc_info->bpts[i].control))
 	  if (ptrace (PTRACE_SETHBPREGS, pid, ((i << 1) + 1),
 	      &proc_info->bpts[i].address) < 0)
-	    error (_("Unexpected error setting breakpoint address"));
+	    perror_with_name ("Unexpected error setting breakpoint address");
 
 	if (arm_hwbp_control_is_initialized (proc_info->bpts[i].control))
 	  if (ptrace (PTRACE_SETHBPREGS, pid, ((i << 1) + 2),
 	      &proc_info->bpts[i].control) < 0)
-	    error (_("Unexpected error setting breakpoint"));
+	    perror_with_name ("Unexpected error setting breakpoint");
 
 	lwp_info->bpts_changed[i] = 0;
       }
@@ -753,12 +730,12 @@ arm_prepare_to_resume (struct lwp_info *
 	if (arm_hwbp_control_is_enabled (proc_info->wpts[i].control))
 	  if (ptrace (PTRACE_SETHBPREGS, pid, -((i << 1) + 1),
 	      &proc_info->wpts[i].address) < 0)
-	    error (_("Unexpected error setting watchpoint address"));
+	    perror_with_name ("Unexpected error setting watchpoint address");
 
 	if (arm_hwbp_control_is_initialized (proc_info->wpts[i].control))
 	  if (ptrace (PTRACE_SETHBPREGS, pid, -((i << 1) + 2),
 	      &proc_info->wpts[i].control) < 0)
-	    error (_("Unexpected error setting watchpoint"));
+	    perror_with_name ("Unexpected error setting watchpoint");
 
 	lwp_info->wpts_changed[i] = 0;
       }
@@ -790,6 +767,11 @@ arm_get_hwcap (unsigned long *valp)
 static void
 arm_arch_setup (void)
 {
+  int pid = lwpid_of (get_thread_lwp (current_inferior));
+
+  /* Query hardware watchpoint/breakpoint capabilities.  */
+  arm_linux_init_hwbp_cap (pid);
+
   arm_hwcap = 0;
   if (arm_get_hwcap (&arm_hwcap) == 0)
     {
@@ -805,7 +787,6 @@ arm_arch_setup (void)
 
   if (arm_hwcap & HWCAP_VFP)
     {
-      int pid;
       char *buf;
 
       /* NEON implies either no VFP, or VFPv3-D32.  We only support
@@ -819,7 +800,6 @@ arm_arch_setup (void)
 
       /* Now make sure that the kernel supports reading these
 	 registers.  Support was added in 2.6.30.  */
-      pid = lwpid_of (get_thread_lwp (current_inferior));
       errno = 0;
       buf = xmalloc (32 * 8 + 4);
       if (ptrace (PTRACE_GETVFPREGS, pid, 0, buf) < 0


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com