From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30950 invoked by alias); 24 Jun 2009 19:10:29 -0000 Received: (qmail 30941 invoked by uid 22791); 24 Jun 2009 19:10:28 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 24 Jun 2009 19:10:22 +0000 Received: (qmail 13062 invoked from network); 24 Jun 2009 19:10:19 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 24 Jun 2009 19:10:19 -0000 From: Pedro Alves To: Aleksandar Ristovski Subject: Re: [patch] gdbserver: Add support for Z0/Z1 packets Date: Wed, 24 Jun 2009 19:10:00 -0000 User-Agent: KMail/1.9.10 Cc: gdb-patches@sourceware.org, Doug Evans References: <200906231700.12402.pedro@codesourcery.com> <4A427584.2090908@qnx.com> In-Reply-To: <4A427584.2090908@qnx.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200906242011.16184.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-06/txt/msg00655.txt.bz2 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 * 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, };