From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17153 invoked by alias); 26 Mar 2010 17:51:21 -0000 Received: (qmail 17137 invoked by uid 22791); 26 Mar 2010 17:51:20 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 26 Mar 2010 17:51:16 +0000 Received: (qmail 9509 invoked from network); 26 Mar 2010 17:51:14 -0000 Received: from unknown (HELO macbook-2.local) (stan@127.0.0.2) by mail.codesourcery.com with ESMTPA; 26 Mar 2010 17:51:14 -0000 Message-ID: <4BACF406.7080701@codesourcery.com> Date: Fri, 26 Mar 2010 17:51:00 -0000 From: Stan Shebs User-Agent: Thunderbird 2.0.0.24 (Macintosh/20100228) MIME-Version: 1.0 To: Eli Zaretskii CC: Stan Shebs , gdb-patches@sourceware.org Subject: Re: [PATCH] Tracepoint source strings References: <4BABDD35.2000209@codesourcery.com> <83aatvh4pf.fsf@gnu.org> In-Reply-To: <83aatvh4pf.fsf@gnu.org> 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: 2010-03/txt/msg00907.txt.bz2 Eli Zaretskii wrote: >> struct breakpoint * >> create_tracepoint_from_upload (struct uploaded_tp *utp) >> { >> ! char *addr_str, small_buf[100]; >> [...] >> ! sprintf (small_buf, "*%s", hex_string (utp->addr)); >> > > Tz-tz-tz... Using a constant-size buffer in sprintf without any check > for overflow? Are you sure that calling the buffer ``small'' will > magically keep you from trouble? ;-) > Presumably even a hypothetical future 128-bit address won't need more than 65 chars to print. :-) >> ! if (utp->cond && !utp->cond_string) >> ! warning ("Uploaded tracepoint %d condition has no source form, ignoring it", >> > > What about _() ? > > Oops, yeah. > >> + void >> + encode_source_string (int tpnum, ULONGEST addr, >> + char *srctype, char *src, char *buf) >> + { >> + sprintf (buf, "%x:%s:%s:%x:%x:", >> + tpnum, phex_nz (addr, sizeof (addr)), srctype, 0, (int) strlen (src)); >> > > Again, sprintf on a buffer whose size is not even known. > Hmmm, I'll see what I can do. >> written = fwrite ("\x7fTRACE0\n", 8, 1, fp); >> ! if (written < 8) >> perror_with_name (pathname); >> >> /* Write descriptive info. */ >> --- 2468,2474 ---- >> binary file, plus a hint as what this file is, and a version >> number in case of future needs. */ >> written = fwrite ("\x7fTRACE0\n", 8, 1, fp); >> ! if (written < 1) >> perror_with_name (pathname); >> > > Why did you change this to accept partial writes? > I was hoping to fix a major brain cramp of mine without anybody noticing - oh well. :-) The two numeric arguments to fwrite are semi-redundant a la calloc, and the return result is based on the *second* argument, which is the number of "items". The other way to fix is to swap the two arguments; about the only advantage of this way is that we always test against 1, instead of repeating the fwrite argument. >> + Specify a source string of tracepoint @var{n} at address @var{var}. >> > ^^^^^^^^^ > You meant @var{addr}, I presume. > > Anyway, can we have several tracepoints with the same number? If not, > why do we need to give the address as well? > Yes and yes. >> such as @samp{cond} for the >> + conditional expression >> > > Conditional expression for what or from where? I'm guessing this is > somehow related to the original definition of the tracepoint, but > please connect the dots more explicitly. > Yes, it's the tracepoint's condition, I can make an xref. I tend to think we can be a little more abbreviated when describing the protocol, because presumably anybody writing a target agent is going to know GDB concepts pretty well. >> + @value{GDBN} issues a separate packet, in order, for each command in a >> + list. >> > > What list? > Command list. >> The target does not need to do anything with source strings >> + except report them back as part of the @samp{qTfP}/@samp{qTsP} >> + queries. >> > > "As part of queries" or as part or _responses_? > As part of replies. I've glossed over details of the qT[fs]P reply to date, so as to not to have to rewrite so much as the rest of the tracepoint patches go in. The grand plan is that there is a notion of "tracepoint description" with a syntax that is common between remote download, remote upload, and trace file, and it should go into its own section that is referenced from both protocol and file format descriptions. Stan