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

Aleksandar Ristovski
2009-06-23T15:18:52Z

[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]

Pedro Alves wrote:
> On Monday 22 June 2009 20:38:50, Aleksandar Ristovski wrote:
>
>>> 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:
>> That was my initial implementation, prior to proposing the
>> change. Then I looked at target ops in gdb; there we have
>> two different functions for breakpoint and watchpoint so I
>> followed that logic (even though the logic there seems to be
>> incomplete: there is a pair for hw and non-hw breakponts but
>> only one pair for watchpoints).
>
> That's because software watchpoints aren't "inserted".

Yes, silly me.

>
>> But either way is fine with me - just let me know.
>
> I'd prefer the approach I suggested, and worry about splitting
> the breakpoints from watchpoints API if/when we actually need it.
>

Ok, then that version is committed.

I attached what I committed.

ChangeLog:

* server.c (process_serial_event): Add support for Z0 and Z1
packet.
* target.h: Comment for *_watchpoint to make it clear the
functions
can get types '0' and '1'.

Thanks,

--
Aleksandar Ristovski
QNX Software Systems

[-- Attachment #2: gdbserver-Z0Z1support-20090623.diff --]
[-- Type: text/x-patch, Size: 3699 bytes --]

Index: server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.98
diff -u -p -r1.98 server.c
--- server.c 19 Jun 2009 13:35:35 -0000 1.98
+++ server.c 23 Jun 2009 15:06:21 -0000
@@ -2371,66 +2371,56 @@ process_serial_event (void)
signal = 0;
myresume (own_buf, 1, signal);
break;
- case 'Z':
+ case 'Z': /* insert_ ... */
+ /* Fallthrough. */
+ case 'z': /* remove_ ... */
{
char *lenptr;
char *dataptr;
CORE_ADDR addr = strtoul (&own_buf[3], &lenptr, 16);
int len = strtol (lenptr + 1, &dataptr, 16);
char type = own_buf[1];
+ int res;
+ const int insert_ = ch == 'Z';
+
+ /* Type: '0' - software-breakpoint
+ '1' - hardware-breakpoint
+ '2' - write watchpoint
+ '3' - read watchpoint
+ '4' - access watchpoint */

if (the_target->insert_watchpoint == NULL
- || (type < '2' || type > '4'))
- {
- /* No watchpoint support or not a watchpoint command;
- unrecognized either way. */
- own_buf[0] = '\0';
- }
+ || the_target->remove_watchpoint == NULL)
+ res = 1; /* Not supported. */
else
- {
- int res;
-
- require_running (own_buf);
- res = (*the_target->insert_watchpoint) (type, addr, len);
- if (res == 0)
- write_ok (own_buf);
- else if (res == 1)
- /* Unsupported. */
- own_buf[0] = '\0';
- else
- write_enn (own_buf);
- }
- break;
- }
- case 'z':
- {
- char *lenptr;
- char *dataptr;
- CORE_ADDR addr = strtoul (&own_buf[3], &lenptr, 16);
- int len = strtol (lenptr + 1, &dataptr, 16);
- char type = own_buf[1];
+ switch (type)
+ {
+ case '2':
+ /* Fallthrough. */
+ case '3':
+ /* Fallthrough. */
+ case '4':
+ require_running (own_buf);
+ /* Fallthrough. */
+ case '0':
+ /* Fallthrough. */
+ case '1':
+ res = insert_ ? (*the_target->insert_watchpoint) (type, addr,
+ len)
+ : (*the_target->remove_watchpoint) (type, addr,
+ len);
+ break;
+ default:
+ res = -1; /* Unrecognized type. */
+ }

- if (the_target->remove_watchpoint == NULL
- || (type < '2' || type > '4'))
- {
- /* No watchpoint support or not a watchpoint command;
- unrecognized either way. */
- own_buf[0] = '\0';
- }
+ if (res == 0)
+ write_ok (own_buf);
+ else if (res == 1)
+ /* Unsupported. */
+ own_buf[0] = '\0';
else
- {
- int res;
-
- require_running (own_buf);
- res = (*the_target->remove_watchpoint) (type, addr, len);
- if (res == 0)
- write_ok (own_buf);
- else if (res == 1)
- /* Unsupported. */
- own_buf[0] = '\0';
- else
- write_enn (own_buf);
- }
+ write_enn (own_buf);
break;
}
case 'k':
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/target.h,v
retrieving revision 1.37
diff -u -p -r1.37 target.h
--- target.h 19 Jun 2009 13:35:35 -0000 1.37
+++ target.h 23 Jun 2009 15:06:21 -0000
@@ -216,10 +216,11 @@ struct target_ops
/* Insert and remove a hardware watchpoint.
Returns 0 on success, -1 on failure and 1 on unsupported.
The type is coded as follows:
- 2 = write watchpoint
- 3 = read watchpoint
- 4 = access watchpoint
- */
+ '0' - software-breakpoint
+ '1' - hardware-breakpoint
+ '2' - write watchpoint
+ '3' - read watchpoint
+ '4' - access watchpoint */

int (*insert_watchpoint) (char type, CORE_ADDR addr, int len);
int (*remove_watchpoint) (char type, CORE_ADDR addr, int len);