From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9418 invoked by alias); 7 Dec 2004 02:34:09 -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 9392 invoked from network); 7 Dec 2004 02:34:03 -0000 Received: from unknown (HELO nevyn.them.org) (66.93.172.17) by sourceware.org with SMTP; 7 Dec 2004 02:34:03 -0000 Received: from drow by nevyn.them.org with local (Exim 4.34 #1 (Debian)) id 1CbVAh-000133-LU; Mon, 06 Dec 2004 21:33:47 -0500 Date: Tue, 07 Dec 2004 02:44:00 -0000 From: Daniel Jacobowitz To: Orjan Friberg Cc: gdb-patches@sources.redhat.com Subject: Re: [gdbserver/patch] Z packet support Message-ID: <20041207023346.GA2524@nevyn.them.org> Mail-Followup-To: Orjan Friberg , gdb-patches@sources.redhat.com References: <41ADDF6B.2040601@axis.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <41ADDF6B.2040601@axis.com> User-Agent: Mutt/1.5.5.1+cvs20040105i X-SW-Source: 2004-12/txt/msg00186.txt.bz2 On Wed, Dec 01, 2004 at 04:12:43PM +0100, Orjan Friberg wrote: > A while ago there was a brief discussion concerning Z packet support in the > gdbserver (starting at > http://sources.redhat.com/ml/gdb-patches/2004-05/msg00706.html) where it > was suggested that maybe the watchpoint code should be shared with GDB. > > Well, I implemented Z packet support in the most straight-forward way ever: > separate from GDB (and I have an upcoming CRISv32 port which would use it). > I don't know if the various watchpoint functions need to change to > accommodate other architectures (if memory serves me correctly there were > some issues with the s390 on the host side regarding "stopped data > address"). I'm fine with the approach. The implementation needs a bit of work, though. First of all, your mailer has mangled the patch. A lot of leading whitespace is messed up. Could you fix that? I'd like to be able to play with this for i386 before approving it. Secondly (presumably a separate problem), you've lost a lot of whitespace before open parentheses. > + /* Watchpoint related functions. */ > + int (*insert_watchpoint)(char type, CORE_ADDR addr, int len); > + int (*remove_watchpoint)(char type, CORE_ADDR addr, int len); > + int (*stopped_by_watchpoint) (void); > + CORE_ADDR (*stopped_data_address) (void); More detailed comments, please - at least for the target.h copy. For instance, what are the return values? > +static int linux_insert_watchpoint (char type, CORE_ADDR addr, int len) > +{ > + if (the_low_target.insert_watchpoint != NULL) > + return the_low_target.insert_watchpoint (type, addr, len); > + else > + return -1; > +} Formatting; function names start in column 0. Also, this block of wrapper functions could use at least one comment. > Index: remote-utils.c > =================================================================== > RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v > retrieving revision 1.22 > diff -u -p -r1.22 remote-utils.c > --- remote-utils.c 16 Oct 2004 17:42:00 -0000 1.22 > +++ remote-utils.c 1 Dec 2004 14:49:02 -0000 > @@ -639,6 +639,30 @@ prepare_resume_reply (char *buf, char st > if (status == 'T') > { > const char **regp = gdbserver_expedite_regs; > + > + if (*the_target->stopped_by_watchpoint != NULL I'm surprised that compiles.... the * there is spurious. > + && (*the_target->stopped_by_watchpoint) ()) > + { > + CORE_ADDR addr; > + > + strncpy (buf, "watch:", 6); > + buf += 6; > + > + addr = (*the_target->stopped_data_address) (); > + > + *buf++ = tohex ((addr >> 28) & 0xf); > + *buf++ = tohex ((addr >> 24) & 0xf); > + *buf++ = tohex ((addr >> 20) & 0xf); > + *buf++ = tohex ((addr >> 16) & 0xf); > + > + *buf++ = tohex ((addr >> 12) & 0xf); > + *buf++ = tohex ((addr >> 8) & 0xf); > + *buf++ = tohex ((addr >> 4) & 0xf); > + *buf++ = tohex (addr & 0xf); > + > + *buf++ = ';'; > + } This is broken on 64-bit targets. This will require a certain amount of shuffling to figure out the proper size of an address. > + 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]; > + > + set_desired_inferior (0); > + if (the_target->insert_watchpoint != NULL > + && ((*the_target->insert_watchpoint) (type, addr, len) > + == 0)) > + write_ok (own_buf); > + else > + own_buf[0] = '\0'; > + break; > + } There's two problems here: - Some of the z/Z commands are breakpoints, not watchpoints. Please only invoke watchpoint methods for watchpoint commands. - An empty string is supposed to represent an unrecognized "type". We should distinguish error from unsupported. Why the call to set_desired_inferior? This should presumably affect all threads, since that's how GDB behaves (or is supposed to). Most likely that should be handled in the linux-low wrappers. -- Daniel Jacobowitz