From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3728 invoked by alias); 24 Jun 2009 19:25:45 -0000 Received: (qmail 3709 invoked by uid 22791); 24 Jun 2009 19:25:44 -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 19:25:38 +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 PAA11980; Wed, 24 Jun 2009 15:25:30 -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 15:25:36 -0400 Message-ID: <4A427DAF.7010007@qnx.com> Date: Wed, 24 Jun 2009 19:25:00 -0000 From: Aleksandar Ristovski User-Agent: Thunderbird 2.0.0.22 (Windows/20090605) MIME-Version: 1.0 To: Pedro Alves CC: gdb-patches@sourceware.org, Doug Evans Subject: Re: [patch] gdbserver: Add support for Z0/Z1 packets References: <200906231700.12402.pedro@codesourcery.com> <4A427584.2090908@qnx.com> <200906242011.16184.pedro@codesourcery.com> In-Reply-To: <200906242011.16184.pedro@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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: 2009-06/txt/msg00659.txt.bz2 Pedro Alves wrote: > 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). > And just another small note: + 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; They should either both be present or none. In the gdb document, there is implementation note that reads: Implementation notes: A remote target shall return an empty string for an un-recognized breakpoint or watchpoint packet type. A remote target shall support either both or neither of a given `Ztype...' and `ztype...' packet pair. To avoid potential problems with duplicate packets, the operations should be imple-mented in an idempotent way. So, I would make it something like I proposed (if either is NULL, it's unsupported - also makes a clear statement to new target implementors). Thanks -- Aleksandar Ristovski QNX Software Systems