From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23279 invoked by alias); 26 Mar 2010 08:24:18 -0000 Received: (qmail 23268 invoked by uid 22791); 26 Mar 2010 08:24:16 -0000 X-SWARE-Spam-Status: No, hits=-0.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_SORBS_WEB,SPF_SOFTFAIL X-Spam-Check-By: sourceware.org Received: from mtaout20.012.net.il (HELO mtaout20.012.net.il) (80.179.55.166) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 26 Mar 2010 08:24:11 +0000 Received: from conversion-daemon.a-mtaout20.012.net.il by a-mtaout20.012.net.il (HyperSendmail v2007.08) id <0KZV00M00R9FQL00@a-mtaout20.012.net.il> for gdb-patches@sourceware.org; Fri, 26 Mar 2010 11:23:08 +0300 (IDT) Received: from HOME-C4E4A596F7 ([77.127.176.135]) by a-mtaout20.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0KZV00KQKRAITY50@a-mtaout20.012.net.il>; Fri, 26 Mar 2010 11:23:07 +0300 (IDT) Date: Fri, 26 Mar 2010 08:24:00 -0000 From: Eli Zaretskii Subject: Re: [PATCH] Tracepoint source strings In-reply-to: <4BABDD35.2000209@codesourcery.com> To: Stan Shebs Cc: gdb-patches@sourceware.org Reply-to: Eli Zaretskii Message-id: <83aatvh4pf.fsf@gnu.org> References: <4BABDD35.2000209@codesourcery.com> X-IsSubscribed: yes 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/msg00872.txt.bz2 > Date: Thu, 25 Mar 2010 15:01:25 -0700 > From: Stan Shebs > > This patch is another step towards the promised land of > accurately-restored tracepoint definitions. For those just tuning in, > the problem is that in both the disconnected tracing and the trace file > cases, it's very difficult to use tracepoints if the definitions in GDB > do not match up with what was used to create the trace frames on the > target or in the file. This patch takes a more audacious approach that > seems to work well in practice; it literally downloads and uploads the > original source form of the tracepoint, whitespace and all. Thanks. > 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? ;-) > ! if (utp->cond && !utp->cond_string) > ! warning ("Uploaded tracepoint %d condition has no source form, ignoring it", What about _() ? > + warning ("Uploaded tracepoint %d actions have no source form, ignoring them", > + utp->number); Ditto. > + remote_get_noisy_reply (&target_buf, &target_buf_size); > + if (strcmp (target_buf, "OK")) > + warning (_("Target does not support source download.")); > + > + if (cmd->control_type == while_control > + || cmd->control_type == while_stepping_control) > + { > + remote_download_command_source (num, addr, *cmd->body_list); > + > + QUIT; /* allow user to bail out with ^C */ > + strcpy (rs->buf, "QTDPsrc:"); > + encode_source_string (num, addr, "cmd", "end", > + rs->buf + strlen (rs->buf)); > + putpkt (rs->buf); > + remote_get_noisy_reply (&target_buf, &target_buf_size); > + if (strcmp (target_buf, "OK")) > + warning (_("Target does not support source download.")); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Can we have this string defined only once, and then use it in all the places where you need it? (The two uses above are not the only ones.) > + 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. > 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? > written = fwrite (&gotten, 4, 1, fp); > ! if (written < 4) > perror_with_name (pathname); > > do_cleanups (cleanup); > --- 2579,2592 ---- > if (gotten == 0) > break; > written = fwrite (buf, gotten, 1, fp); > ! if (written < 1) > perror_with_name (pathname); Same here. > + if (strncmp (srctype, "at:", strlen ("at:")) == 0) > + utp->at_string = xstrdup (buf); > + else if (strncmp (srctype, "cond:", strlen ("cond:")) == 0) > + utp->cond_string = xstrdup (buf); > + else if (strncmp (srctype, "cmd:", strlen ("cmd:")) == 0) Isn't it better to use sizeof here instead of strlen? > ! warning ("Unrecognized tracepoint piece '%c', ignoring", piece); No _() . > + 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? > + This is useful to get accurate reproduction of the tracepoints > + originally downloaded at the beginning of the trace run. It would be good to include here at least some of the text of the mail I'm responding to, where you explain why this feature is useful. > The > + @var{type} is a name of the part I would lose the "The" part, as the usual style is not to have it before parameter names. Also, I think "the name" is more correct, English-wise. Finally, please say something like "see below", because you delay the description of TYPE to several sentences later. > 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. > while @var{bytes} is the string, encoded in > + hexadecimal. @var{start} is a starting offset, while @var{slen} is > + the total length; use a nonzero @var{start} and multiple > + @samp{QTDPsrc} packets when the source string is too long to fit in a > + single packet. Same here: please help the reader understand how to construct each of these elements. > + @value{GDBN} issues a separate packet, in order, for each command in a > + list. What 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_? Thanks.