[PATCH] Tracepoint source strings

[PATCH] Tracepoint source strings

[Stan Shebs]

[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]

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.  As freaky 
as it sounds, it works because if the source strings don't parse 
(because the executable has changed, for instance), then there is almost 
no chance that the trace frame data is going to be meaningful at the 
symbolic level.

This is not quite the end of the story, the heuristic matchup of new/old 
tracepoints still needs work, and there are some minor patches still 
queued up.  For testing, I'll add to the tfile.exp test after 
incorporating Pedro's feedback from the previous patch.  I'll hold off 
committing this for a couple days in case anybody has comments.

Stan

2010-03-25  Stan Shebs  <stan@codesourcery.com>

        * tracepoint.h (struct uploaded_string): New struct.
        (struct uploaded_tp): New fields for source strings.
        * breakpoint.c (this_utp, next_cmd): New globals.
        (read_uploaded_action): New function.
        (create_tracepoint_from_upload): Fill in more parts
        of a tracepoint.
        * tracepoint.c (encode_source_string): New function.
        (trace_save): Write out source strings, fix error checks.
        (parse_tracepoint_definition): Add source string parsing.
        * remote.c (PACKET_TracepointSource): New packet type.
        (remote_download_command_source): New function.
        (remote_download_tracepoint): Download source pieces also.
        (_initialize_remote): Add packet config command.

        * gdb.texinfo (Tracepoint Packets): Describe QTDPsrc.
        (General Query Packets): Describe TracepointSource.



[-- Attachment #2: tpsrc-patch-1 --]
[-- Type: text/plain, Size: 22314 bytes --]

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.468
diff -p -r1.468 breakpoint.c
*** breakpoint.c	25 Mar 2010 20:48:52 -0000	1.468
--- breakpoint.c	25 Mar 2010 21:31:44 -0000
*************** ftrace_command (char *arg, int from_tty)
*** 10364,10369 ****
--- 10364,10389 ----
      set_tracepoint_count (breakpoint_count);
  }
  
+ /* Set up a fake reader function that gets command lines from a linked
+    list that was acquired during tracepoint uploading.  */
+ 
+ static struct uploaded_tp *this_utp;
+ static struct uploaded_string *next_cmd;
+ 
+ static char *
+ read_uploaded_action (void)
+ {
+   char *rslt;
+ 
+   if (!next_cmd)
+     return NULL;
+ 
+   rslt = next_cmd->str;
+   next_cmd = next_cmd->next;
+ 
+   return rslt;
+ }
+ 
  /* Given information about a tracepoint as recorded on a target (which
     can be either a live system or a trace file), attempt to create an
     equivalent GDB tracepoint.  This is not a reliable process, since
*************** ftrace_command (char *arg, int from_tty)
*** 10373,10387 ****
  struct breakpoint *
  create_tracepoint_from_upload (struct uploaded_tp *utp)
  {
!   char buf[100];
    struct breakpoint *tp;
  
!   /* In the absence of a source location, fall back to raw address.  */
!   sprintf (buf, "*%s", paddress (get_current_arch(), utp->addr));
  
    if (!create_breakpoint (get_current_arch (),
! 			  buf,
! 			  NULL, 0, 1 /* parse arg */,
  			  0 /* tempflag */,
  			  (utp->type == bp_fast_tracepoint) /* hardwareflag */,
  			  1 /* traceflag */,
--- 10393,10423 ----
  struct breakpoint *
  create_tracepoint_from_upload (struct uploaded_tp *utp)
  {
!   char *addr_str, small_buf[100];
    struct breakpoint *tp;
  
!   if (utp->at_string)
!     addr_str = utp->at_string;
!   else
!     {
!       /* In the absence of a source location, fall back to raw
! 	 address.  Since there is no way to confirm that the address
! 	 means the same thing as when the trace was started, warn the
! 	 user.  */
!       warning ("Uploaded tracepoint %d has no source location, using raw address",
! 	       utp->number);
!       sprintf (small_buf, "*%s", hex_string (utp->addr));
!       addr_str = small_buf;
!     }
! 
!   /* There's not much we can do with a sequence of bytecodes.  */
!   if (utp->cond && !utp->cond_string)
!     warning ("Uploaded tracepoint %d condition has no source form, ignoring it",
! 	     utp->number);
  
    if (!create_breakpoint (get_current_arch (),
! 			  addr_str,
! 			  utp->cond_string, -1, 0 /* parse cond/thread */,
  			  0 /* tempflag */,
  			  (utp->type == bp_fast_tracepoint) /* hardwareflag */,
  			  1 /* traceflag */,
*************** create_tracepoint_from_upload (struct up
*** 10394,10423 ****
  
    set_tracepoint_count (breakpoint_count);
    
    tp = get_tracepoint (tracepoint_count);
    gdb_assert (tp != NULL);
  
    if (utp->pass > 0)
      {
!       sprintf (buf, "%d %d", utp->pass, tp->number);
  
!       trace_pass_command (buf, 0);
      }
  
!   if (utp->cond)
      {
!       printf_filtered ("Want to restore a condition\n");
!     }
  
!   if (utp->numactions > 0)
!     {
!       printf_filtered ("Want to restore action list\n");
!     }
  
!   if (utp->num_step_actions > 0)
!     {
!       printf_filtered ("Want to restore action list\n");
      }
  
    return tp;
    }
--- 10430,10464 ----
  
    set_tracepoint_count (breakpoint_count);
    
+   /* Get the tracepoint we just created.  */
    tp = get_tracepoint (tracepoint_count);
    gdb_assert (tp != NULL);
  
    if (utp->pass > 0)
      {
!       sprintf (small_buf, "%d %d", utp->pass, tp->number);
  
!       trace_pass_command (small_buf, 0);
      }
  
!   /* If we have uploaded versions of the original commands, set up a
!      special-purpose "reader" function and call the usual command line
!      reader, then pass the result to the breakpoint command-setting
!      function.  */
!   if (utp->cmd_strings)
      {
!       struct command_line *cmd_list;
  
!       this_utp = utp;
!       next_cmd = utp->cmd_strings;
  
!       cmd_list = read_command_lines_1 (read_uploaded_action, 1, NULL, NULL);
! 
!       breakpoint_set_commands (tp, cmd_list);
      }
+   else if (utp->numactions > 0 || utp->num_step_actions > 0)
+     warning ("Uploaded tracepoint %d actions have no source form, ignoring them",
+ 	     utp->number);
  
    return tp;
    }
Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.394
diff -p -r1.394 remote.c
*** remote.c	24 Mar 2010 01:12:13 -0000	1.394
--- remote.c	25 Mar 2010 21:31:44 -0000
*************** enum {
*** 1154,1159 ****
--- 1154,1160 ----
    PACKET_FastTracepoints,
    PACKET_bc,
    PACKET_bs,
+   PACKET_TracepointSource,
    PACKET_MAX
  };
  
*************** static struct protocol_feature remote_pr
*** 3462,3467 ****
--- 3463,3470 ----
      PACKET_bc },
    { "ReverseStep", PACKET_DISABLE, remote_supported_packet,
      PACKET_bs },
+   { "TracepointSource", PACKET_DISABLE, remote_supported_packet,
+     PACKET_TracepointSource },
  };
  
  static void
*************** free_actions_list (char **actions_list)
*** 9267,9278 ****
    xfree (actions_list);
  }
  
  static void
  remote_download_tracepoint (struct breakpoint *t)
  {
    struct bp_location *loc;
    CORE_ADDR tpaddr;
!   char tmp[40];
    char buf[2048];
    char **tdp_actions;
    char **stepping_actions;
--- 9270,9319 ----
    xfree (actions_list);
  }
  
+ /* Recursive routine to walk through command list including loops, and
+    download packets for each command.  */
+ 
+ static void
+ remote_download_command_source (int num, ULONGEST addr,
+ 				struct command_line *cmds)
+ {
+   struct remote_state *rs = get_remote_state ();
+   struct command_line *cmd;
+ 
+   for (cmd = cmds; cmd; cmd = cmd->next)
+     {
+       QUIT;	/* allow user to bail out with ^C */
+       strcpy (rs->buf, "QTDPsrc:");
+       encode_source_string (num, addr, "cmd", cmd->line,
+ 			    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."));
+ 
+       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."));
+ 	}
+     }
+ }
+ 
  static void
  remote_download_tracepoint (struct breakpoint *t)
  {
    struct bp_location *loc;
    CORE_ADDR tpaddr;
!   char addrbuf[40];
    char buf[2048];
    char **tdp_actions;
    char **stepping_actions;
*************** remote_download_tracepoint (struct break
*** 9293,9301 ****
        (void) make_cleanup (free_actions_list_cleanup_wrapper, stepping_actions);
  
        tpaddr = loc->address;
!       sprintf_vma (tmp, (loc ? tpaddr : 0));
        sprintf (buf, "QTDP:%x:%s:%c:%lx:%x", t->number, 
! 	       tmp, /* address */
  	       (t->enable_state == bp_enabled ? 'E' : 'D'),
  	       t->step_count, t->pass_count);
        /* Fast tracepoints are mostly handled by the target, but we can
--- 9334,9342 ----
        (void) make_cleanup (free_actions_list_cleanup_wrapper, stepping_actions);
  
        tpaddr = loc->address;
!       sprintf_vma (addrbuf, tpaddr);
        sprintf (buf, "QTDP:%x:%s:%c:%lx:%x", t->number, 
! 	       addrbuf, /* address */
  	       (t->enable_state == bp_enabled ? 'E' : 'D'),
  	       t->step_count, t->pass_count);
        /* Fast tracepoints are mostly handled by the target, but we can
*************** remote_download_tracepoint (struct break
*** 9352,9360 ****
        if (strcmp (target_buf, "OK"))
  	error (_("Target does not support tracepoints."));
  
-   if (!t->commands && !*default_collect)
- 	continue;
- 
        /* do_single_steps (t); */
        if (tdp_actions)
  	{
--- 9393,9398 ----
*************** remote_download_tracepoint (struct break
*** 9362,9368 ****
  	    {
  	      QUIT;	/* allow user to bail out with ^C */
  	      sprintf (buf, "QTDP:-%x:%s:%s%c",
! 		       t->number, tmp, /* address */
  		       tdp_actions[ndx],
  		       ((tdp_actions[ndx + 1] || stepping_actions)
  			? '-' : 0));
--- 9400,9406 ----
  	    {
  	      QUIT;	/* allow user to bail out with ^C */
  	      sprintf (buf, "QTDP:-%x:%s:%s%c",
! 		       t->number, addrbuf, /* address */
  		       tdp_actions[ndx],
  		       ((tdp_actions[ndx + 1] || stepping_actions)
  			? '-' : 0));
*************** remote_download_tracepoint (struct break
*** 9379,9385 ****
  	    {
  	      QUIT;	/* allow user to bail out with ^C */
  	      sprintf (buf, "QTDP:-%x:%s:%s%s%s",
! 		       t->number, tmp, /* address */
  		       ((ndx == 0) ? "S" : ""),
  		       stepping_actions[ndx],
  		       (stepping_actions[ndx + 1] ? "-" : ""));
--- 9417,9423 ----
  	    {
  	      QUIT;	/* allow user to bail out with ^C */
  	      sprintf (buf, "QTDP:-%x:%s:%s%s%s",
! 		       t->number, addrbuf, /* address */
  		       ((ndx == 0) ? "S" : ""),
  		       stepping_actions[ndx],
  		       (stepping_actions[ndx + 1] ? "-" : ""));
*************** remote_download_tracepoint (struct break
*** 9390,9395 ****
--- 9428,9460 ----
  		error (_("Error on target while setting tracepoints."));
  	    }
  	}
+ 
+       if (remote_protocol_packets[PACKET_TracepointSource].support == PACKET_ENABLE)
+ 	{
+ 	  if (t->addr_string)
+ 	    {
+ 	      strcpy (buf, "QTDPsrc:");
+ 	      encode_source_string (t->number, loc->address,
+ 				    "at", t->addr_string, buf + strlen (buf));
+ 	      putpkt (buf);
+ 	      remote_get_noisy_reply (&target_buf, &target_buf_size);
+ 	      if (strcmp (target_buf, "OK"))
+ 		warning (_("Target does not support source download."));
+ 	    }
+ 	  if (t->cond_string)
+ 	    {
+ 	      strcpy (buf, "QTDPsrc:");
+ 	      encode_source_string (t->number, loc->address,
+ 				    "cond", t->cond_string, buf + strlen (buf));
+ 	      putpkt (buf);
+ 	      remote_get_noisy_reply (&target_buf, &target_buf_size);
+ 	      if (strcmp (target_buf, "OK"))
+ 		warning (_("Target does not support source download."));
+ 	    }
+ 	  remote_download_command_source (t->number, loc->address,
+ 					  t->commands->commands);
+ 	}
+ 
        do_cleanups (old_chain);
      }
  }
*************** Show the maximum size of the address (in
*** 10231,10236 ****
--- 10296,10304 ----
    add_packet_config_cmd (&remote_protocol_packets[PACKET_FastTracepoints],
  			 "FastTracepoints", "fast-tracepoints", 0);
  
+   add_packet_config_cmd (&remote_protocol_packets[PACKET_TracepointSource],
+ 			 "TracepointSource", "TracepointSource", 0);
+ 
    /* Keep the old ``set remote Z-packet ...'' working.  Each individual
       Z sub-packet has its own set and show commands, but users may
       have sets to this variable in their .gdbinit files (or in their
Index: tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.c,v
retrieving revision 1.155
diff -p -r1.155 tracepoint.c
*** tracepoint.c	24 Mar 2010 21:12:18 -0000	1.155
--- tracepoint.c	25 Mar 2010 21:31:44 -0000
*************** trace_dump_command (char *args, int from
*** 2403,2408 ****
--- 2403,2421 ----
    discard_cleanups (old_cleanups);
  }
  
+ /* Encode a piece of a tracepoint's source-level definition in a form
+    that is suitable for both protocol and saving in files.  */
+ /* This version does not make multiple packets for long strings.  FIXME  */
+ 
+ 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));
+   bin2hex (src, buf + strlen (buf), 0);
+ }
+ 
  extern int trace_regblock_size;
  
  /* Save tracepoint data to file named FILENAME.  If TARGET_DOES_SAVE is
*************** trace_save (const char *filename, int ta
*** 2420,2425 ****
--- 2433,2439 ----
    struct uploaded_tp *uploaded_tps = NULL, *utp;
    struct uploaded_tsv *uploaded_tsvs = NULL, *utsv;
    int a;
+   struct uploaded_string *cmd;
    LONGEST gotten = 0;
    ULONGEST offset = 0;
  #define MAX_TRACE_UPLOAD 2000
*************** trace_save (const char *filename, int ta
*** 2454,2460 ****
       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 < 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);
  
    /* Write descriptive info.  */
*************** trace_save (const char *filename, int ta
*** 2529,2534 ****
--- 2543,2565 ----
  	fprintf (fp, "tp S%x:%s:%s\n",
  		 utp->number, phex_nz (utp->addr, sizeof (utp->addr)),
  		 utp->step_actions[a]);
+       if (utp->at_string)
+ 	{
+ 	  encode_source_string (utp->number, utp->addr,
+ 				"at", utp->at_string, buf);
+ 	  fprintf (fp, "tp Z%s\n", buf);
+ 	}
+       if (utp->cond_string)
+ 	{
+ 	  encode_source_string (utp->number, utp->addr,
+ 				"cond", utp->cond_string, buf);
+ 	  fprintf (fp, "tp Z%s\n", buf);
+ 	}
+       for (cmd = utp->cmd_strings; cmd; cmd = cmd->next)
+ 	{
+ 	  encode_source_string (utp->number, utp->addr, "cmd", cmd->str, buf);
+ 	  fprintf (fp, "tp Z%s\n", buf);
+ 	}
      }
  
    free_uploaded_tps (&uploaded_tps);
*************** trace_save (const char *filename, int ta
*** 2548,2561 ****
        if (gotten == 0)
  	break;
        written = fwrite (buf, gotten, 1, fp);
!       if (written < gotten)
  	perror_with_name (pathname);
        offset += gotten;
      }
  
!   /* Mark the end of trace data.  */
    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);
        offset += gotten;
      }
  
!   /* Mark the end of trace data.  (We know that gotten is 0 at this point.)  */
    written = fwrite (&gotten, 4, 1, fp);
!   if (written < 1)
      perror_with_name (pathname);
  
    do_cleanups (cleanup);
*************** Status line: '%s'\n"), p, line);
*** 3197,3214 ****
      }
  }
  
! /* Given a line of text defining a tracepoint or tracepoint action, parse
!    it into an "uploaded tracepoint".  */
  
  void
  parse_tracepoint_definition (char *line, struct uploaded_tp **utpp)
  {
    char *p;
    char piece;
!   ULONGEST num, addr, step, pass, orig_size, xlen;
!   int enabled, i;
    enum bptype type;
!   char *cond;
    struct uploaded_tp *utp = NULL;
  
    p = line;
--- 3228,3245 ----
      }
  }
  
! /* Given a line of text defining a part of a tracepoint, parse it into
!    an "uploaded tracepoint".  */
  
  void
  parse_tracepoint_definition (char *line, struct uploaded_tp **utpp)
  {
    char *p;
    char piece;
!   ULONGEST num, addr, step, pass, orig_size, xlen, start;
!   int enabled, i, end;
    enum bptype type;
!   char *cond, *srctype, *src, *buf;
    struct uploaded_tp *utp = NULL;
  
    p = line;
*************** parse_tracepoint_definition (char *line,
*** 3268,3276 ****
        utp = get_uploaded_tp (num, addr, utpp);
        utp->step_actions[utp->num_step_actions++] = xstrdup (p);
      }
    else
      {
!       error ("Invalid tracepoint piece");
      }
  }
  
--- 3299,3347 ----
        utp = get_uploaded_tp (num, addr, utpp);
        utp->step_actions[utp->num_step_actions++] = xstrdup (p);
      }
+   else if (piece == 'Z')
+     {
+       /* Parse a chunk of source form definition.  */
+       utp = get_uploaded_tp (num, addr, utpp);
+       srctype = p;
+       p = strchr (p, ':');
+       p++;  /* skip a colon */
+       p = unpack_varlen_hex (p, &start);
+       p++;  /* skip a colon */
+       p = unpack_varlen_hex (p, &xlen);
+       p++;  /* skip a colon */
+ 
+       buf = alloca (strlen (line));
+ 
+       end = hex2bin (p, (gdb_byte *) buf, strlen (p) / 2);
+       buf[end] = '\0';
+ 
+       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)
+ 	{
+ 	  /* FIXME consider using a vector? */
+ 	  struct uploaded_string *last, *newlast;
+ 	  newlast = (struct uploaded_string *) xmalloc (sizeof (struct uploaded_string));
+ 	  newlast->str = xstrdup (buf);
+ 	  newlast->next = NULL;
+ 	  if (utp->cmd_strings)
+ 	    {
+ 	      for (last = utp->cmd_strings; last->next; last = last->next)
+ 		;
+ 	      last->next = newlast;
+ 	    }
+ 	  else
+ 	    utp->cmd_strings = newlast;
+ 	}
+     }
    else
      {
!       /* Don't error out, the target might be sending us optional
! 	 info that we don't care about.  */
!       warning ("Unrecognized tracepoint piece '%c', ignoring", piece);
      }
  }
  
Index: tracepoint.h
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.h,v
retrieving revision 1.29
diff -p -r1.29 tracepoint.h
*** tracepoint.h	23 Mar 2010 22:05:45 -0000	1.29
--- tracepoint.h	25 Mar 2010 21:31:44 -0000
*************** extern char *default_collect;
*** 116,122 ****
  
  /* Struct to collect random info about tracepoints on the target.  */
  
! struct uploaded_tp {
    int number;
    enum bptype type;
    ULONGEST addr;
--- 116,129 ----
  
  /* Struct to collect random info about tracepoints on the target.  */
  
! struct uploaded_string
! {
!   char *str;
!   struct uploaded_string *next;
! };
! 
! struct uploaded_tp
! {
    int number;
    enum bptype type;
    ULONGEST addr;
*************** struct uploaded_tp {
*** 129,140 ****
    char *actions[100];
    int num_step_actions;
    char *step_actions[100];
    struct uploaded_tp *next;
  };
  
  /* Struct recording info about trace state variables on the target.  */
  
! struct uploaded_tsv {
    const char *name;
    int number;
    LONGEST initial_value;
--- 136,158 ----
    char *actions[100];
    int num_step_actions;
    char *step_actions[100];
+ 
+   /* The original string defining the location of the tracepoint.  */
+   char *at_string;
+ 
+   /* The original string defining the tracepoint's condition.  */
+   char *cond_string;
+ 
+   /* List of original strings defining the tracepoint's actions.  */
+   struct uploaded_string *cmd_strings;
+ 
    struct uploaded_tp *next;
  };
  
  /* Struct recording info about trace state variables on the target.  */
  
! struct uploaded_tsv
! {
    const char *name;
    int number;
    LONGEST initial_value;
*************** extern void while_stepping_pseudocommand
*** 160,165 ****
--- 178,186 ----
  extern struct trace_state_variable *find_trace_state_variable (const char *name);
  extern struct trace_state_variable *create_trace_state_variable (const char *name);
  
+ extern void encode_source_string (int num, ULONGEST addr,
+ 				  char *srctype, char *src, char *buf);
+ 
  extern void parse_trace_status (char *line, struct trace_status *ts);
  
  extern void parse_tracepoint_definition (char *line, struct uploaded_tp **utpp);
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.683
diff -p -r1.683 gdb.texinfo
*** doc/gdb.texinfo	24 Mar 2010 21:24:08 -0000	1.683
--- doc/gdb.texinfo	25 Mar 2010 21:31:45 -0000
*************** These are the currently defined stub fea
*** 30713,30718 ****
--- 30713,30723 ----
  @tab @samp{-}
  @tab No
  
+ @item @samp{TracepointSource}
+ @tab No
+ @tab @samp{-}
+ @tab No
+ 
  @end multitable
  
  These are the currently defined stub features, in more detail:
*************** The remote stub accepts and implements t
*** 30806,30811 ****
--- 30811,30820 ----
  The remote stub accepts and implements the reverse step packet
  (@pxref{bs}).
  
+ @item TracepointSource
+ The remote stub understands the @samp{QTDPsrc} packet that supplies
+ the source form of tracepoint definitions.
+ 
  @end table
  
  @item qSymbol::
*************** encoded).  @value{GDBN} will continue to
*** 30849,30854 ****
--- 30858,30864 ----
  @item QTBuffer
  @item QTDisconnected
  @itemx QTDP
+ @itemx QTDPsrc
  @itemx QTDV
  @itemx qTfP
  @itemx qTfV
*************** The packet was understood and carried ou
*** 31250,31255 ****
--- 31260,31284 ----
  The packet was not recognized.
  @end table
  
+ @item QTDPsrc:@var{n}:@var{addr}:@var{type}:@var{start}:@var{slen}:@var{bytes}
+ @cindex @samp{QTDPsrc} packet
+ Specify a source string of tracepoint @var{n} at address @var{var}.
+ This is useful to get accurate reproduction of the tracepoints
+ originally downloaded at the beginning of the trace run.  The
+ @var{type} is a name of the part, such as @samp{cond} for the
+ conditional expression, 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.
+ 
+ The available string types are: @samp{at} for the location,
+ @samp{cond} for the conditional, and @samp{cmd} for an action command.
+ @value{GDBN} issues a separate packet, in order, for each command in a
+ 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.
+ 
  @item QTDV:@var{n}:@var{value}
  @cindex define trace state variable, remote request
  @cindex @samp{QTDV} packet

Re: [PATCH] Tracepoint source strings

[Eli Zaretskii]

> 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.

Re: [PATCH] Tracepoint source strings

[Stan Shebs]

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

Re: [PATCH] Tracepoint source strings

[Eli Zaretskii]

> Date: Fri, 26 Mar 2010 10:51:02 -0700
> From: Stan Shebs <stan@codesourcery.com>
> CC: Stan Shebs <stan@codesourcery.com>, gdb-patches@sourceware.org
> 
> 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. :-)

Yes, and then someone comes up and changes the code to put there
something in addition to the address (you already prepend an asterisk
to it).

But if I'm the only one who is bothered by this, I withdraw my
objections.

> >>     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".

So you are writing a string as if it were an 8-byte int?  Won't that
swap bytes on some architectures?  And why pretend that a string is a
number, anyway?

As for the rest, my questions were meant to signal that portions of
the description are not clear enough, and could use some more explicit
description and/or references to other parts of the manual.

Re: [PATCH] Tracepoint source strings

[Stan Shebs]

Eli Zaretskii wrote:
>> Date: Fri, 26 Mar 2010 10:51:02 -0700
>> From: Stan Shebs <stan@codesourcery.com>
>> CC: Stan Shebs <stan@codesourcery.com>, gdb-patches@sourceware.org
>>
>> 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. :-)
>>     
>
> Yes, and then someone comes up and changes the code to put there
> something in addition to the address (you already prepend an asterisk
> to it).
>
> But if I'm the only one who is bothered by this, I withdraw my
> objections.
>   

What's a better thing to do then?  Am I missing something in our current 
coding preferences for this kind of thing?  (Which is entirely possible, 
oldtimers often get out of date on best practices... :-) )

>>>>     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".
>>     
>
> So you are writing a string as if it were an 8-byte int?  Won't that
> swap bytes on some architectures?  And why pretend that a string is a
> number, anyway?
>   

fwrite doesn't know about ints and such; as the man page says, it's just 
doing repeated putc starting at the given address.  The only effect of 
the two arguments is the way the results are reported, so if you ask to 
write 2 elements of size 4, but low-level writing fails at byte 6, it's 
going to report that it wrote 1 element.  Kind of a weak API concept, I 
suspect there are few production uses where neither argument is 1...

> As for the rest, my questions were meant to signal that portions of
> the description are not clear enough, and could use some more explicit
> description and/or references to other parts of the manual.
>
>   

Indeed, and I will be working on that.  Having been so close to this 
stuff for a while, it's good to get the fresh perspective!

Stan

Re: [PATCH] Tracepoint source strings

[Eli Zaretskii]

> Date: Fri, 26 Mar 2010 11:38:01 -0700
> From: Stan Shebs <stan@codesourcery.com>
> CC: Stan Shebs <stan@codesourcery.com>, gdb-patches@sourceware.org
> 
> What's a better thing to do then?  Am I missing something in our current 
> coding preferences for this kind of thing?

I'd take a look at asprintf and xstrprintf.

Re: [PATCH] Tracepoint source strings

[Pedro Alves]

On Thursday 25 March 2010 22:01:25, Stan Shebs wrote:
>         * gdb.texinfo (Tracepoint Packets): Describe QTDPsrc.
>         (General Query Packets): Describe TracepointSource.

Could you add a NEWS entry for these new packets, please?

-- 
Pedro Alves