From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15362 invoked by alias); 19 Jun 2009 13:56:58 -0000 Received: (qmail 15346 invoked by uid 22791); 19 Jun 2009 13:56:57 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from qnxmail.qnx.com (HELO qnxmail.qnx.com) (209.226.137.76) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 19 Jun 2009 13:56:49 +0000 Received: from Nebula.ott.qnx.com (nebula.ott.qnx.com [10.42.3.30]) by hub.ott.qnx.com (8.9.3/8.9.3) with ESMTP id JAA07402; Fri, 19 Jun 2009 09:56:39 -0400 Received: from [127.0.0.1] ([10.42.100.129]) by Nebula.ott.qnx.com with Microsoft SMTPSVC(6.0.3790.3959); Fri, 19 Jun 2009 09:56:43 -0400 Message-ID: <4A3B98EC.70209@qnx.com> Date: Fri, 19 Jun 2009 13:56:00 -0000 From: Aleksandar Ristovski User-Agent: Thunderbird 2.0.0.21 (Windows/20090302) MIME-Version: 1.0 To: Doug Evans CC: gdb-patches@sources.redhat.com Subject: Re: [patch] gdbserver: Add support for Z0/Z1 packets References: In-Reply-To: Content-Type: multipart/mixed; boundary="------------060806080108070908090707" 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/msg00500.txt.bz2 This is a multi-part message in MIME format. --------------060806080108070908090707 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 2404 Doug Evans wrote: > On Tue, Jun 16, 2009 at 6:31 PM, Aleksandar Ristovski wrote: >> Aleksandar Ristovski wrote: >>> Hello, >>> >>> This adds support for Z0 and Z1 (and z0 and z1) packets. >>> >> I apologize, the patch should be this one. Change log corrected as well. >> >> -- >> Aleksandar Ristovski >> QNX Software Systems >> >> >> ChangeLog: >> >> * target.h (insert_breakpoint, remove_breakpoint): New target functions. >> * server.c (process_serial_event): 'Z' and 'z' packet - add support >> for [Zz]0 and [Zz]1 - software and hardware breakpoints. >> * linux-low.c (linux_target_ops): Add new fields. >> * spu-low.c (spu_target_ops): Likewise. >> * win32-low.c (win32_target_ops): Likewise. > > Hi. > > Minor nits (fwiw): > > Inserting the insert_breakpoint/remove_breakpoint fields *between* the > stopped_by_watchpoint/stopped_data_address fields is odd. > Can they go before insert_watchpoint? > > I think the code in server.c that watches for 0/1/2/3/4 should use a switch. > > The code previously caught invalid values (e.g. type == '5'), but you > accidentally removed that (IIUC). > > The code in server.c does this: > > + if (the_target->insert_watchpoint == NULL) > + { > + /* No watchpoint support or not a watchpoint command; > + unrecognized either way. */ > + own_buf[0] = '\0'; > + res = 1; > + } > > and later does this: > > + if (res == 0) > + write_ok (own_buf); > + else if (res == 1) > + /* Unsupported. */ > + own_buf[0] = '\0'; > + else > + write_enn (own_buf); > > How about just setting res = 1 in the first code snippet above? > [This appears for both 'z' and 'Z' packets.] > Ok, I made changes accordingly. Note that since the code in 'Z' and 'z' differs only in whether "insert_" or "remove_" version of function is called, I merged the two with the distinction being made at the calling point. -- Aleksandar Ristovski QNX Software Systems ChangeLog: * target.h (insert_breakpoint, remove_breakpoint): New target functions. * server.c (process_serial_event): 'Z' and 'z' packet - add support for [Zz]0 and [Zz]1 - software and hardware breakpoints. * linux-low.c (linux_target_ops): Add new fields. * spu-low.c (spu_target_ops): Likewise. * win32-low.c (win32_target_ops): Likewise. --------------060806080108070908090707 Content-Type: text/x-patch; name="gdbserver-Z0Z1support-20090619.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="gdbserver-Z0Z1support-20090619.diff" Content-length: 5534 Index: target.h =================================================================== 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 + Returns 0 if successful, 1 otherwise. */ + + int (*insert_breakpoint) (char type, CORE_ADDR addr); + int (*remove_breakpoint) (char type, CORE_ADDR addr); + /* Insert and remove a hardware watchpoint. Returns 0 on success, -1 on failure and 1 on unsupported. The type is coded as follows: Index: server.c =================================================================== RCS file: /cvs/src/src/gdb/gdbserver/server.c,v retrieving revision 1.97 diff -u -p -r1.97 server.c --- server.c 24 May 2009 21:06:53 -0000 1.97 +++ server.c 19 Jun 2009 13:47:44 -0000 @@ -2366,66 +2370,64 @@ 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'; - 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'; - } - else - { - int res; + /* Type: '0' - software-breakpoint + '1' - hardware-breakpoint + '2' - write watchpoint + '3' - read watchpoint + '4' - access watchpoint */ - 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'; + switch (type) + { + case '0': + /* Fallthrough. */ + case '1': + if ((insert_ ? the_target->insert_breakpoint + : the_target->remove_breakpoint) == NULL) + res = 1; /* Not supported. */ else - write_enn (own_buf); + res = insert_ ? (*the_target->insert_breakpoint) (type, addr) + : (*the_target->remove_breakpoint) (type, addr); + break; + case '2': + /* Fallthrough. */ + case '3': + /* Fallthrough. */ + case '4': + if ((insert_ ? the_target->insert_watchpoint + : the_target->remove_watchpoint) == NULL) + res = 1; /* Not supported. */ + else + { + require_running (own_buf); + res = insert_ ? (*the_target->insert_watchpoint) (type, addr, + len) + : (*the_target->remove_watchpoint) (type, addr, + len); + } + break; + default: + res = -1; /* Unrecognized type. */ } - 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]; - 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: linux-low.c =================================================================== RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v retrieving revision 1.105 diff -u -p -r1.105 linux-low.c --- linux-low.c 24 May 2009 17:44:19 -0000 1.105 +++ linux-low.c 19 Jun 2009 13:47:45 -0000 @@ -3024,6 +3030,8 @@ static struct target_ops linux_target_op linux_look_up_symbols, linux_request_interrupt, linux_read_auxv, + NULL, /* insert_breakpoint */ + NULL, /* remove_breakpoint */ linux_insert_watchpoint, linux_remove_watchpoint, linux_stopped_by_watchpoint, Index: spu-low.c =================================================================== RCS file: /cvs/src/src/gdb/gdbserver/spu-low.c,v retrieving revision 1.24 diff -u -p -r1.24 spu-low.c --- spu-low.c 3 Apr 2009 14:38:39 -0000 1.24 +++ spu-low.c 19 Jun 2009 13:47:45 -0000 @@ -612,6 +612,8 @@ static struct target_ops spu_target_ops spu_look_up_symbols, spu_request_interrupt, NULL, + NULL, /* insert_breakpoint */ + NULL, /* remove_breakpoint */ NULL, NULL, NULL, Index: win32-low.c =================================================================== RCS file: /cvs/src/src/gdb/gdbserver/win32-low.c,v retrieving revision 1.35 diff -u -p -r1.35 win32-low.c --- win32-low.c 1 Apr 2009 22:50:24 -0000 1.35 +++ win32-low.c 19 Jun 2009 13:47:45 -0000 @@ -1697,6 +1697,8 @@ static struct target_ops win32_target_op NULL, win32_request_interrupt, NULL, + NULL, /* insert_breakpoint */ + NULL, /* remove_breakpoint */ NULL, NULL, NULL, --------------060806080108070908090707--