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

Pedro Alves
2009-06-24T19:10:29Z

On Wednesday 24 June 2009 19:50:44, Aleksandar Ristovski wrote:
> Pedro Alves wrote:
> > On Tuesday 23 June 2009 16:17:58, Aleksandar Ristovski wrote:
> >> 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:
> >
> >>>> 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.
> >
> > Well, we had never seen "that" version
>
> Ok, to rectify this I am attaching two versions: one if I
> revert the changes I committed and the other is diff to what
> is in now.
>
> > ... and you bypassed the "rename" suggestion...
>
> I did not do any renaming - I think it is not terribly
> confusing since both in target.h comment and server.c 'Z'
> case it is made very clear that it handles both breakpoints
> and watchpoints (i.e. I don't find it any clearer if it was
> called "insert_point"... it would still require reading the
> comment in target.h)

Urgh, I was just about to press the send button when I saw
this message of yours. This version corrects a few troubles
with the previous commit (see ChangeLog) (one of them I still
see in your new patch) and I've tested it on x86_64-linux.
Aleksandar, please, please, do run the testsuite (and state
that you have) when posting patches. E.g., we could have caught
the vKill issue that Pierre fixed when we made only linux
report multi-process (the testsuite runs in "target remote"
mode, hence with multi-process off most of the way).

--
Pedro Alves

2009-06-24 Pedro Alves <pedro@codesourcery.com>

* server.c (process_serial_event): Re-return unsupported, not
error, if the type isn't recognized. Re-allow supporting only
insert or remove packets. Also call require_running for
breakpoints. Add missing break statement to default case. Tidy.
* target.h (struct target_ops): Rename insert_watchpoint to
insert_point, and remove_watchpoint to remove_point.

* linux-low.h (struct linux_target_ops): Likewise.
* linux-low.c (linux_insert_watchpoint): Rename to ...
(linux_insert_point): ... this. Adjust.
(linux_remove_watchpoint): Rename to ...
(linux_remove_point): ... this. Adjust.
(linux_target_ops): Adjust.
* linux-crisv32-low.c (cris_insert_watchpoint): Rename to ...
(cris_insert_point): ... this.
(cris_remove_watchpoint): Rename to ...
(cris_remove_point): ... this.
(the_low_target): Adjust.

---
gdb/gdbserver/linux-crisv32-low.c | 8 +++---
gdb/gdbserver/linux-low.c | 21 ++++++++-------
gdb/gdbserver/linux-low.h | 7 +++--
gdb/gdbserver/server.c | 50 ++++++++++++++------------------------
gdb/gdbserver/target.h | 6 ++--
5 files changed, 41 insertions(+), 51 deletions(-)

Index: src/gdb/gdbserver/server.c
===================================================================
--- src.orig/gdb/gdbserver/server.c 2009-06-24 19:48:36.000000000 +0100
+++ src/gdb/gdbserver/server.c 2009-06-24 19:58:34.000000000 +0100
@@ -2383,38 +2383,26 @@ process_serial_event (void)
int len = strtol (lenptr + 1, &dataptr, 16);
char type = own_buf[1];
int res;
- const int insert_ = ch == 'Z';
+ 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
- || the_target->remove_watchpoint == NULL)
- res = 1; /* Not supported. */
- else
- 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. */
- }
+ /* Default to unrecognized/unsupported. */
+ res = 1;
+ switch (type)
+ {
+ case '0': /* software-breakpoint */
+ case '1': /* hardware-breakpoint */
+ case '2': /* write watchpoint */
+ case '3': /* read watchpoint */
+ case '4': /* access watchpoint */
+ require_running (own_buf);
+ if (insert && the_target->insert_point != NULL)
+ res = (*the_target->insert_point) (type, addr, len);
+ else if (!insert && the_target->remove_point != NULL)
+ res = (*the_target->remove_point) (type, addr, len);
+ break;
+ default:
+ break;
+ }

if (res == 0)
write_ok (own_buf);
Index: src/gdb/gdbserver/target.h
===================================================================
--- src.orig/gdb/gdbserver/target.h 2009-06-24 19:48:36.000000000 +0100
+++ src/gdb/gdbserver/target.h 2009-06-24 19:48:37.000000000 +0100
@@ -213,7 +213,7 @@ struct target_ops
int (*read_auxv) (CORE_ADDR offset, unsigned char *myaddr,
unsigned int len);

- /* Insert and remove a hardware watchpoint.
+ /* Insert and remove a break or watchpoint.
Returns 0 on success, -1 on failure and 1 on unsupported.
The type is coded as follows:
'0' - software-breakpoint
@@ -222,8 +222,8 @@ struct target_ops
'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);
+ int (*insert_point) (char type, CORE_ADDR addr, int len);
+ int (*remove_point) (char type, CORE_ADDR addr, int len);

/* Returns 1 if target was stopped due to a watchpoint hit, 0 otherwise. */

Index: src/gdb/gdbserver/linux-low.h
===================================================================
--- src.orig/gdb/gdbserver/linux-low.h 2009-06-24 19:48:36.000000000 +0100
+++ src/gdb/gdbserver/linux-low.h 2009-06-24 19:48:37.000000000 +0100
@@ -80,9 +80,10 @@ struct linux_target_ops
int decr_pc_after_break;
int (*breakpoint_at) (CORE_ADDR pc);

- /* Watchpoint related functions. See target.h for comments. */
- int (*insert_watchpoint) (char type, CORE_ADDR addr, int len);
- int (*remove_watchpoint) (char type, CORE_ADDR addr, int len);
+ /* Breakpoint and watchpoint related functions. See target.h for
+ comments. */
+ int (*insert_point) (char type, CORE_ADDR addr, int len);
+ int (*remove_point) (char type, CORE_ADDR addr, int len);
int (*stopped_by_watchpoint) (void);
CORE_ADDR (*stopped_data_address) (void);

Index: src/gdb/gdbserver/linux-low.c
===================================================================
--- src.orig/gdb/gdbserver/linux-low.c 2009-06-24 19:48:36.000000000 +0100
+++ src/gdb/gdbserver/linux-low.c 2009-06-24 19:48:37.000000000 +0100
@@ -2671,24 +2671,25 @@ linux_read_auxv (CORE_ADDR offset, unsig
return n;
}

-/* These watchpoint related wrapper functions simply pass on the function call
- if the target has registered a corresponding function. */
+/* These breakpoint and watchpoint related wrapper functions simply
+ pass on the function call if the target has registered a
+ corresponding function. */

static int
-linux_insert_watchpoint (char type, CORE_ADDR addr, int len)
+linux_insert_point (char type, CORE_ADDR addr, int len)
{
- if (the_low_target.insert_watchpoint != NULL)
- return the_low_target.insert_watchpoint (type, addr, len);
+ if (the_low_target.insert_point != NULL)
+ return the_low_target.insert_point (type, addr, len);
else
/* Unsupported (see target.h). */
return 1;
}

static int
-linux_remove_watchpoint (char type, CORE_ADDR addr, int len)
+linux_remove_point (char type, CORE_ADDR addr, int len)
{
- if (the_low_target.remove_watchpoint != NULL)
- return the_low_target.remove_watchpoint (type, addr, len);
+ if (the_low_target.remove_point != NULL)
+ return the_low_target.remove_point (type, addr, len);
else
/* Unsupported (see target.h). */
return 1;
@@ -3030,8 +3031,8 @@ static struct target_ops linux_target_op
linux_look_up_symbols,
linux_request_interrupt,
linux_read_auxv,
- linux_insert_watchpoint,
- linux_remove_watchpoint,
+ linux_insert_point,
+ linux_remove_point,
linux_stopped_by_watchpoint,
linux_stopped_data_address,
#if defined(__UCLIBC__) && defined(HAS_NOMMU)
Index: src/gdb/gdbserver/linux-crisv32-low.c
===================================================================
--- src.orig/gdb/gdbserver/linux-crisv32-low.c 2009-06-24 19:48:35.000000000 +0100
+++ src/gdb/gdbserver/linux-crisv32-low.c 2009-06-24 19:48:37.000000000 +0100
@@ -137,7 +137,7 @@ cris_write_data_breakpoint (int bp, unsi
}

static int
-cris_insert_watchpoint (char type, CORE_ADDR addr, int len)
+cris_insert_point (char type, CORE_ADDR addr, int len)
{
int bp;
unsigned long bp_ctrl;
@@ -220,7 +220,7 @@ cris_insert_watchpoint (char type, CORE_
}

static int
-cris_remove_watchpoint (char type, CORE_ADDR addr, int len)
+cris_remove_point (char type, CORE_ADDR addr, int len)
{
int bp;
unsigned long bp_ctrl;
@@ -375,8 +375,8 @@ struct linux_target_ops the_low_target =
cris_reinsert_addr,
0,
cris_breakpoint_at,
- cris_insert_watchpoint,
- cris_remove_watchpoint,
+ cris_insert_point,
+ cris_remove_point,
cris_stopped_by_watchpoint,
cris_stopped_data_address,
};