From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8092 invoked by alias); 24 Jun 2009 18:51:25 -0000 Received: (qmail 8077 invoked by uid 22791); 24 Jun 2009 18:51:24 -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; Wed, 24 Jun 2009 18:51:16 +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 OAA09931; Wed, 24 Jun 2009 14:51:07 -0400 Received: from [127.0.0.1] ([10.42.100.129]) by Nebula.ott.qnx.com with Microsoft SMTPSVC(6.0.3790.3959); Wed, 24 Jun 2009 14:51:13 -0400 Message-ID: <4A427584.2090908@qnx.com> Date: Wed, 24 Jun 2009 18:51:00 -0000 From: Aleksandar Ristovski User-Agent: Thunderbird 2.0.0.22 (Windows/20090605) MIME-Version: 1.0 To: gdb-patches@sourceware.org CC: Pedro Alves , Doug Evans Subject: Re: [patch] gdbserver: Add support for Z0/Z1 packets References: <200906222346.54263.pedro@codesourcery.com> <4A40F226.4080909@qnx.com> <200906231700.12402.pedro@codesourcery.com> In-Reply-To: <200906231700.12402.pedro@codesourcery.com> Content-Type: multipart/mixed; boundary="------------020208000204050009040807" 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/msg00651.txt.bz2 This is a multi-part message in MIME format. --------------020208000204050009040807 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 1969 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) > > Would you care to explain why are watchpoints guarded on > require_running and breakpoints aren't? It's not super > obvious to me. Both proposed versions now check for require_running for any kind of breakpoint. ChangeLog - diff to what is in now. * server.c (process_serial_event): Treat all types of break/watch-points the same. ChangeLog - diff to what was before my commit: * 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'. (attached first diff to what is in already and then diff to what was before commit). Thanks, -- Aleksandar Ristovski QNX Software Systems --------------020208000204050009040807 Content-Type: text/x-patch; name="gdbserver-Z0Z1support-20090624.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="gdbserver-Z0Z1support-20090624.diff" Content-length: 1520 Index: server.c =================================================================== RCS file: /cvs/src/src/gdb/gdbserver/server.c,v retrieving revision 1.99 diff -u -p -r1.99 server.c --- server.c 23 Jun 2009 15:12:44 -0000 1.99 +++ server.c 24 Jun 2009 15:36:11 -0000 @@ -2381,7 +2381,6 @@ process_serial_event (void) 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 @@ -2392,27 +2391,16 @@ process_serial_event (void) if (the_target->insert_watchpoint == NULL || the_target->remove_watchpoint == NULL) res = 1; /* Not supported. */ + else if (type >= '0' && type <= '4') + { + const int insert_ = ch == 'Z'; + + require_running (own_buf); + res = insert_ ? (*the_target->insert_watchpoint) (type, addr, len) + : (*the_target->remove_watchpoint) (type, addr, len); + } 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. */ - } + res = -1; /* Unrecognized type. */ if (res == 0) write_ok (own_buf); --------------020208000204050009040807 Content-Type: text/x-patch; name="gdbserver-Z0Z1support-20090624-1.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="gdbserver-Z0Z1support-20090624-1.diff" Content-length: 3443 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 24 Jun 2009 17:13:04 -0000 @@ -2371,66 +2371,44 @@ 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; + + /* 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'; - } - else + || the_target->remove_watchpoint == NULL) + res = 1; /* Not supported. */ + else if (type >= '0' && type <= '4') { - 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'; + if (ch == 'Z') + res = (*the_target->insert_watchpoint) (type, addr, len); 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]; - - 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'; + res = (*the_target->remove_watchpoint) (type, addr, len); } else - { - int res; + res = -1; /* Unrecognized type. */ - 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); - } + if (res == 0) + write_ok (own_buf); + else if (res == 1) + /* Unsupported. */ + own_buf[0] = '\0'; + else + write_enn (own_buf); break; } case 'k': Index: target.h =================================================================== RCS file: /cvs/src/src/gdb/gdbserver/target.h,v retrieving revision 1.37 retrieving revision 1.38 diff -u -p -r1.37 -r1.38 --- target.h 19 Jun 2009 13:35:35 -0000 1.37 +++ target.h 23 Jun 2009 15:12:44 -0000 1.38 @@ -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); --------------020208000204050009040807--