From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1353 invoked by alias); 23 Feb 2012 16:34:48 -0000 Received: (qmail 978 invoked by uid 22791); 23 Feb 2012 16:34:44 -0000 X-SWARE-Spam-Status: No, hits=-6.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 23 Feb 2012 16:33:21 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q1NGWw3f004949 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 23 Feb 2012 11:32:58 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q1NGWuIH020113; Thu, 23 Feb 2012 11:32:57 -0500 Message-ID: <4F466A38.8080109@redhat.com> Date: Thu, 23 Feb 2012 17:25:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20120131 Thunderbird/10.0 MIME-Version: 1.0 To: "Gustavo, Luis" CC: Stan Shebs , gdb-patches@sourceware.org Subject: Re: [rfc target-side break conditions 5/5 v2] GDBServer-side changes References: <4F230A29.3060404@mentor.com> <4F31A249.9000800@earthlink.net> <4F3301C1.9070400@mentor.com> <4F33BCE9.8080900@redhat.com> <4F450705.2010407@mentor.com> In-Reply-To: <4F450705.2010407@mentor.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: 2012-02/txt/msg00507.txt.bz2 Hi Luis, On 02/22/2012 03:17 PM, Luis Gustavo wrote: > On 02/09/2012 10:32 AM, Pedro Alves wrote: >>> Index: gdb/gdb/gdbserver/server.c >>> =================================================================== >>> --- gdb.orig/gdb/gdbserver/server.c 2012-02-08 15:57:07.399075002 -0200 >>> +++ gdb/gdb/gdbserver/server.c 2012-02-08 15:57:33.139074999 -0200 >>> @@ -1621,6 +1621,9 @@ handle_query (char *own_buf, int packet_ >>> strcat (own_buf, ";tracenz+"); >>> } >>> >>> + /* Support target-side breakpoint conditions. */ >>> + strcat (own_buf, ";ConditionalBreakpoints+"); >> >> I still think it's a shame this doesn't mean all Z packets >> understand target side conditionals... > > This probably means all Z packets, but only breakpoints are being implemented now on both gdbserver's and gdb's side. There's no probably :-). GDB will need to be able to tell. Either it does, or it doesn't. If watchpoints aren't supported now, we'll need a new qSupported bit latter when we support them. (*) No biggie, but the documentation should be clear (may it already is though, haven't checked for that). (*) I thought about it a bit, and I had a moment of "damn, this isn't going to work", thinking about the fact that we're assuming the stubs must ignore Z packets for the same addresses, and that with watchpoints that might not work, given that the remote side does reference counting of resources, to handle overlapping watchpoints. Then I remembered that GDB will never send exact duplicates of watchpoints, so it can send 1 watchpoint location for ADDR1;LEN1 and another for ADDR1;LEN2, but never two for ADDR1;LEN1 (assuming same type). So we're good, and supporting this for watchpoints shouldn't be hard. > >> >> >>> +static void >>> +process_point_options (CORE_ADDR point_addr, char **packet) >>> +{ >>> + char *dataptr = *packet; >>> + >>> + while (dataptr[0] == ';') >>> + { >>> + dataptr++; >>> + >>> + if (!strncmp (dataptr, "conditions=", strlen ("conditions="))) >> >> strncmp's return is not a boolean. Please write as >> >> if (strncmp (dataptr, "conditions=", strlen ("conditions=")) == 0) > > Fixed. > >> >>> + { >>> + /* We have conditions to parse. */ >>> + dataptr += strlen ("conditions="); >>> + >>> + /* Insert conditions. */ >>> + do >>> + { >>> + add_breakpoint_condition (point_addr,&dataptr); >>> + } while (*dataptr == 'X'); >>> + } >>> + } >> >> Should we silently ignore unknown options, or error? If the former, >> then you should skip to the next `;' and go from there. If the latter, >> well, and error is missing. :-) > > Silently ignore. I thought further about the "conditions" marker and i decided to drop it. We may want to pass more attributes in the future, and these markers will be an overhead and will possibly get in the way. > > I'm passing plain expressions now, with the first char identifying the action. This is in synch with how tracepoint actions/attributes are passed down to the target. > > This also makes it easier to ignore unknown tokens, which in turn allows people to easily extend the list of attributes in Z packets in the future. > > What do you think? Hmm, thinking more, I don't think GDB should ever send tokens the stub didn't report support for in qSupported, so in practice it doesn't matter that much either way, but in any case: > +/* Process options coming from Z packets for *point at address > + POINT_ADDR. PACKET is the packet buffer. *PACKET is updated > + to point to the first char after the last processed option. */ > + > +static void > +process_point_options (CORE_ADDR point_addr, char **packet) > +{ > + char *dataptr = *packet; > + > + /* Check if data has the correct format. */ > + if (*dataptr != ';') > + return; > + > + dataptr++; > + > + while (*dataptr) > + { > + switch (*dataptr) > + { > + case 'X': > + /* Conditional expression. */ > + fprintf (stderr, "Found breakpoint condition.\n"); > + add_breakpoint_condition (point_addr, &dataptr); > + break; > + default: > + /* Unrecognized token, just skip it. */ > + fprintf (stderr, "Unknown token %c, ignoring.\n", > + *dataptr); > + dataptr++; That's not the proper way to skip an unknown token. You need to skip until some known marker (`;' or `,' or some such). > + } > + } > + *packet = dataptr; > +} Everything else looked good to me now. Thanks. -- Pedro Alves