This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Tracepoint source strings


> Date: Thu, 25 Mar 2010 15:01:25 -0700
> From: Stan Shebs <stan@codesourcery.com>
> 
> 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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]