Re: [patch] gdbserver: Add support for Z0/Z1 packets

[Pedro Alves <pedro at codesourcery dot com>]

On Friday 19 June 2009 14:55:56, Aleksandar Ristovski wrote:

> RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
> retrieving revision 1.36
> diff -u -p -r1.36 target.h
> --- target.h????1 Apr 2009 22:50:24 -0000???????1.36
> +++ target.h????19 Jun 2009 13:47:44 -0000
> @@ -213,6 +213,14 @@ struct target_ops
> ? ?int (*read_auxv) (CORE_ADDR offset, unsigned char *myaddr,
> ???????????????? ? ?unsigned int len);
> ?
> + ?/* Insert and remove breakpoint. ?Argument TYPE can be:
> + ? ? ? '0' = software-breakpoint
> + ? ? ? '1' = hardware-breakpoint 

                                   ^ spurious space

> + ? ? Returns 0 if successful, 1 otherwise. ?*/
> +
> + ?int (*insert_breakpoint) (char type, CORE_ADDR addr);
> + ?int (*remove_breakpoint) (char type, CORE_ADDR addr);

Z0 and Z1 breakpoints also take a 'len' argument, just
like Z2-Z4.  You should also pass those down.

But, Let's take a step back --- why not just rename the
insert_watchpoint|remove_watchpoint functions to insert_point,remove_point,
and relax the type checks in server.c:

	if (the_target->insert_watchpoint == NULL
	    || (type < '2' || type > '4'))
	  {

... to also allow type == '0' or '1' ?

Looking at your nto-low.c file in the original patch, you were
doing this:

+static int
+nto_insert_breakpoint (char type, CORE_ADDR addr)
            ^^^^^^^^^^
+{
+  TRACE ("%s\n", __func__);
+
+  return nto_insert_watchpoint (type, addr, 1);
                     ^^^^^^^^^^
+}

In the latest patch, you're doing this:

+static int
+nto_insert_breakpoint (char type, CORE_ADDR addr)
+{
+  TRACE ("%s\n", __func__);
+  switch (type)
+    {
+    case '0': /* software-breakpoint */
+      return nto_breakpoint (addr, _DEBUG_BREAK_EXEC, 0);
+    case '1': /* hardware-breakpoint */
+      return nto_breakpoint (addr, _DEBUG_BREAK_EXEC | _DEBUG_BREAK_HW, 0);
+    }
+  return 1; /* Not supported.  */
+}

+static int
+nto_insert_watchpoint (char type, CORE_ADDR addr, int len)
+{
+  int wtype = _DEBUG_BREAK_HW; /* Always request HW.  */
+
+  TRACE ("%s type:%c\n", __func__, type);
+  switch (type)
+    {
+    case '2':  /* write watchpoint */
+      wtype |= _DEBUG_BREAK_RW;
+      break;
+    case '3':  /* read watchpoint */
+      wtype |= _DEBUG_BREAK_RD
+      break;
+    case '4':  /* access watchpoint */
+      wtype |= _DEBUG_BREAK_RW;
+      break;
+    default:
+      return 1; /* Not supported.  */
+    }
+  return nto_breakpoint (addr, wtype, 0);
+}

That is, still always deferring to a single function
(nto_breakpoint).

This suggests that the insert_breakpoint and insert_watchpoint
interface distintion is unnecessary.

-- 
Pedro Alves