From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17050 invoked by alias); 8 Aug 2002 18:30:22 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 16956 invoked from network); 8 Aug 2002 18:30:19 -0000 Received: from unknown (HELO localhost.redhat.com) (216.138.202.10) by sources.redhat.com with SMTP; 8 Aug 2002 18:30:19 -0000 Received: from ges.redhat.com (localhost [127.0.0.1]) by localhost.redhat.com (Postfix) with ESMTP id A911E3F6E; Thu, 8 Aug 2002 14:30:17 -0400 (EDT) Message-ID: <3D52B8B9.6070409@ges.redhat.com> Date: Thu, 08 Aug 2002 11:30:00 -0000 From: Andrew Cagney User-Agent: Mozilla/5.0 (X11; U; NetBSD macppc; en-US; rv:1.0.0) Gecko/20020802 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Grace Sainsbury Cc: gdb-patches@sources.redhat.com, Eli Zaretskii Subject: Re: [rfa] hardware breakpoints -- remote targets References: <20020806121916.A24815@tomago.toronto.redhat.com> <20020808130648.A27850@tomago.toronto.redhat.com> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2002-08/txt/msg00187.txt.bz2 > I revised the patch to include Mark Salter's earlier patch, as well as > his documentation changes. Lets see, Mark's doco patch was previously approved vis: http://sources.redhat.com/ml/gdb-patches/2000-11/msg00020.html (Someone (me) has even gone through the table and has a patch to fix the formatting pending :-) Hold back on the commit, though, until we've got the below ok. > -char *unpack_varlen_hex (char *buff, int *result); > +char *unpack_varlen_hex (char *buff, ULONGEST *result); I didn't see a changelog entry for this. Anyway, this change (and the corresponding int->ULONGEST tweaks) should be separated out committed independantly. This part is approved. > +/* This is set to the data address of the access causing the target > + * to stop for a watchpoint. */ (NB: no leading ``*'' on the second comment line.) > +static CORE_ADDR remote_watch_data_address; > + > +/* This is non-zero if taregt stopped for a watchpoint. */ > +static int remote_stopped_by_watchpoint_p; > + > + For the above can you please add a ``FIXME: graces/YYYY-MM-DD:'' style comment noteing that, like gdbarch_tdep() for ``struct gdbarch'', these static variables should be bound to an instance of the target object. Only there isn't such a thing :-( > @@ -3025,6 +3035,8 @@ > if (target_wait_loop_hook) > (*target_wait_loop_hook) (); > > + remote_stopped_by_watchpoint_p = 0; > + > switch (buf[0]) > { > case 'E': /* Error of some sort */ > @@ -3048,10 +3060,19 @@ > unsigned char *p1; > char *p_temp; > int fieldsize; > + LONGEST pnum = 0; > > - /* Read the ``P'' register number. */ > - LONGEST pnum = strtol ((const char *) p, &p_temp, 16); > - p1 = (unsigned char *) p_temp; > + /* If this packet is an awatch packet, don't parse the 'a' > + as a register number. */ Add a comment mentioning that ``p1'' is left pointing to ``p'' if there wasn't a register number. > + if (!strstr ((const char *) p, "awatch")) Use ``strncmp (p, "awatch", strlen ("awatch")) != 0''. This is because the string compare needs to be anchored on the first character (otherwize it could miss-parse ``00=a123;awatch''. BTW, it should be possible to safely remove most of those ``(const char*)'' and ``(char*)'' casts. > + { > + /* Read the ``P'' register number. */ > + pnum = strtol ((const char *) p, &p_temp, 16); > + p1 = (unsigned char *) p_temp; > + } > + else > + p1=p; Don't forget the spaces: p1 = p; > if (p1 == p) /* No register number present here */ > { > @@ -3066,6 +3087,21 @@ > record_currthread (thread_num); > p = (unsigned char *) p_temp; > } > + else if ((strncmp ((const char *) p, "watch", p1 - p) == 0) > + || (strncmp ((const char *) p, "rwatch", p1 - p) == 0) > + || (strncmp ((const char *) p, "awatch", p1 - p) == 0)) > + { > + remote_stopped_by_watchpoint_p = 1; > + p = unpack_varlen_hex (++p1, &addr); > + remote_watch_data_address = (CORE_ADDR)addr; > + } > + else > + { > + /* silently skip unknown optional info */ > + p_temp = (unsigned char *) strchr ((const char *) p1+1, ';'); Spaces, ``p1 + 1'', no cast. The comment should be a sentence (hi MarkK ;-) with period and two spaces after it :-) > @@ -3268,10 +3307,19 @@ > unsigned char *p1; > char *p_temp; > int fieldsize; > + long pnum = 0; > > - /* Read the register number */ > - long pnum = strtol ((const char *) p, &p_temp, 16); > - p1 = (unsigned char *) p_temp; > + /* If this packet is an awatch packet, don't parse the 'a' see the comments above ... > +static void > +set_hardware_watchpoint_limit (char *args, int from_tty) > +{ > + if (args) > + { > + char *arg_end; > + int val = strtoul (args, &arg_end, 10); > + if (*args && *arg_end == '\0') > + { > + remote_hw_watchpoint_limit = val; > + return; > + } > + } > + error ("Illegal argument for \"set remote hardware-watchpoint-limit\" command"); > +} Hmm, can you please split out the new commands from the other parts of the patch so that they can be considered separatly. I'm wondering if a better way of doing this is to have GDB automatically detect if another watchpoint is available. Anyway new commands would need doco. Andrew (And thanks for finally creating a patch that adds watchpoint support to the target vector where it belongs!)