Re: [PATCH] Collecting strings at tracepoints

[Stan Shebs <stanshebs@earthlink.net>]

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

On 10/4/11 12:10 PM, Tom Tromey wrote:
>>>>>> "Stan" == Stan Shebs<stan_shebs@mentor.com>  writes:
> Stan>  While conceptually simple, we need a new agent bytecode to make this
> Stan>  work (writing a loop using existing bytecodes doesn't let us check
> Stan>  that we're running off the edge of valid memory), and a support flag
> Stan>  so that users get informed if the target doesn't support string
> Stan>  collection.
>
> Stan>  + /* Inspired by trace_kludge, this indicates that pointers to chars
> Stan>  +    should get an added tracenz bytecode to record nonzero bytes, up to
> Stan>  +    a length that is the value of string_kludge.  */
> Stan>  + int string_kludge;
>
> I'd prefer a name that makes it obvious that this is a tracing thing.
>
> Stan>  +   { "tracenz", PACKET_DISABLE,
> Stan>  +     remote_string_tracing_feature, -1 },
>
> I think this requires documentation in the remote protocol section.
>
> Stan>  +   while (*exp == ' ' || *exp == '\t')
> Stan>  +     exp++;
>
> We have skip_spaces now.
>
>

Thanks for the feedback!  I took all into account, and committed this:

2011-11-02  Stan Shebs <stan@codesourcery.com>

     String collection for tracepoints.
     * NEWS: Mention string collection.
     * common/ax.def (tracenz): New bytecode.
     * ax-gdb.h (trace_string_kludge): Declare.
     * ax-gdb.c: Include valprint.h and c-lang.h.
     (trace_string_kludge): New global.
     (gen_traced_pop): Add string case.
     (agent_command): Add string case.
     * tracepoint.h (decode_agent_options): Declare.
     * tracepoint.c: Include cli-utils.h.
     (decode_agent_options): New function.
     (validate_actionline): Call it.
     (encode_actions_1): Ditto.
     * target.h (struct target_ops): New method to_supports_string_tracing.
     (target_supports_string_tracing): New macro.
     * target.c (update_current_target): Add to_supports_string_tracing.
     * remote.c (struct remote_state): New field string_tracing.
     (remote_string_tracing_feature): New function.
     (remote_protocol_features): New feature tracenz.
     (remote_supports_string_tracing): New function.
     (init_remote_ops): Set to_supports_string_tracing.

     * tracepoint.c (agent_mem_read_string): New function.
     (eval_agent_expr): Call it for tracenz.
     * server.c (handle_query): Report support for tracenz.

     * gdb.texinfo (Tracepoint Action Lists): Document collect/s.
     (General Query Packets): Describe tracenz feature.
     * agentexpr.texi (Bytecode Descriptions): Describe tracenz.

     * gdb.trace/collection.c: Add code using strings.
     * gdb.trace/collection.exp: Add tests of string collection.



[-- Attachment #2: tstr-patch-2 --]
[-- Type: text/plain, Size: 13148 bytes --]

Index: ax-gdb.c
===================================================================
RCS file: /cvs/src/src/gdb/ax-gdb.c,v
retrieving revision 1.89
diff -r1.89 ax-gdb.c
44a45,47
> #include "valprint.h"
> #include "c-lang.h"
> 
337a341,345
> /* Inspired by trace_kludge, this indicates that pointers to chars
>    should get an added tracenz bytecode to record nonzero bytes, up to
>    a length that is the value of trace_string_kludge.  */
> int trace_string_kludge;
> 
395a404,410
>   int string_trace = 0;
>   if (trace_string_kludge
>       && TYPE_CODE (value->type) == TYPE_CODE_PTR
>       && c_textual_element_type (check_typedef (TYPE_TARGET_TYPE (value->type)),
> 				 's'))
>     string_trace = 1;
> 
400,402c415,423
< 	/* We don't trace rvalues, just the lvalues necessary to
< 	   produce them.  So just dispose of this value.  */
< 	ax_simple (ax, aop_pop);
---
> 	if (string_trace)
> 	  {
> 	    ax_const_l (ax, trace_string_kludge);
> 	    ax_simple (ax, aop_tracenz);
> 	  }
> 	else
> 	  /* We don't trace rvalues, just the lvalues necessary to
> 	     produce them.  So just dispose of this value.  */
> 	  ax_simple (ax, aop_pop);
408a430,432
> 	  if (string_trace)
> 	    ax_simple (ax, aop_dup);
> 
415a440,446
> 
> 	  if (string_trace)
> 	    {
> 	      ax_simple (ax, aop_ref32);
> 	      ax_const_l (ax, trace_string_kludge);
> 	      ax_simple (ax, aop_tracenz);
> 	    }
424a456,464
>        
> 	/* But if the register points to a string, assume the value
> 	   will fit on the stack and push it anyway.  */
> 	if (string_trace)
> 	  {
> 	    ax_reg (ax, value->u.reg);
> 	    ax_const_l (ax, trace_string_kludge);
> 	    ax_simple (ax, aop_tracenz);
> 	  }
2491a2532,2535
>   trace_string_kludge = 0;
>   if (*exp == '/')
>     exp = decode_agent_options (exp);
> 
Index: ax-gdb.h
===================================================================
RCS file: /cvs/src/src/gdb/ax-gdb.h,v
retrieving revision 1.21
diff -r1.21 ax-gdb.h
114a115
> extern int trace_string_kludge;
Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.465
diff -r1.465 remote.c
333a334,336
>   /* True if the stub can collect strings using tracenz bytecode.  */
>   int string_tracing;
> 
3714a3718,3727
> static void
> remote_string_tracing_feature (const struct protocol_feature *feature,
> 			       enum packet_support support,
> 			       const char *value)
> {
>   struct remote_state *rs = get_remote_state ();
> 
>   rs->string_tracing = (support == PACKET_ENABLE);
> }
> 
3766a3780,3781
>   { "tracenz", PACKET_DISABLE,
>     remote_string_tracing_feature, -1 },
9742a9758,9765
> static int
> remote_supports_string_tracing (void)
> {
>   struct remote_state *rs = get_remote_state ();
> 
>   return rs->string_tracing;
> }
> 
10461a10485
>   remote_ops.to_supports_string_tracing = remote_supports_string_tracing;
Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.286
diff -r1.286 target.c
674a675
>       INHERIT (to_supports_string_tracing, t);
842a844,846
>   de_fault (to_supports_string_tracing,
> 	    (int (*) (void))
> 	    return_zero);
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.214
diff -r1.214 target.h
661a662,664
>     /* Does this target support the tracenz bytecode for string collection?  */
>     int (*to_supports_string_tracing) (void);
> 
906a910,912
> #define target_supports_string_tracing() \
>   (*current_target.to_supports_string_tracing) ()
> 
Index: tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.c,v
retrieving revision 1.233
diff -r1.233 tracepoint.c
54a55
> #include "cli/cli-utils.h"
576a578,617
> /* Parse any collection options, such as /s for strings.  */
> 
> char *
> decode_agent_options (char *exp)
> {
>   struct value_print_options opts;
> 
>   if (*exp != '/')
>     return exp;
> 
>   /* Call this to borrow the print elements default for collection
>      size.  */
>   get_user_print_options (&opts);
> 
>   exp++;
>   if (*exp == 's')
>     {
>       if (target_supports_string_tracing ())
> 	{
> 	  /* Allow an optional decimal number giving an explicit maximum
> 	     string length, defaulting it to the "print elements" value;
> 	     so "collect/s80 mystr" gets at most 80 bytes of string.  */
> 	  trace_string_kludge = opts.print_max;
> 	  exp++;
> 	  if (*exp >= '0' && *exp <= '9')
> 	    trace_string_kludge = atoi (exp);
> 	  while (*exp >= '0' && *exp <= '9')
> 	    exp++;
> 	}
>       else
> 	error (_("Target does not support \"/s\" option for string tracing."));
>     }
>   else
>     error (_("Undefined collection format \"%c\"."), *exp);
> 
>   exp = skip_spaces (exp);
> 
>   return exp;
> }
> 
658a700,703
>       trace_string_kludge = 0;
>       if (*p == '/')
> 	p = decode_agent_options (p);
> 
1315a1361,1364
> 	  trace_string_kludge = 0;
> 	  if (*action_exp == '/')
> 	    action_exp = decode_agent_options (action_exp);
> 
2583a2633,2635
> 	      if (*action_exp == '/')
> 		action_exp = decode_agent_options (action_exp);
> 
Index: tracepoint.h
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.h,v
retrieving revision 1.43
diff -r1.43 tracepoint.h
214a215,217
> 
> extern char *decode_agent_options (char *exp);
> 
Index: NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.463
diff -r1.463 NEWS
116a117,124
> collect[/s] EXPRESSIONS
>   The tracepoint collect command now takes an optional modifier "/s"
>   that directs it to dereference pointer-to-character types and
>   collect the bytes of memory up to a zero byte.  The behavior is
>   similar to what you see when you use the regular print command on a
>   string.  An optional integer following the "/s" sets a bound on the
>   number of bytes that will be collected.
> 
Index: doc/agentexpr.texi
===================================================================
RCS file: /cvs/src/src/gdb/doc/agentexpr.texi,v
retrieving revision 1.15
diff -r1.15 agentexpr.texi
491a492,496
> @item @code{tracenz} (0x2f)  @var{addr} @var{size} @result{}
> Record the bytes at @var{addr} in a trace buffer, for later retrieval
> by GDB.  Stop at either the first zero byte, or when @var{size} bytes
> have been recorded, whichever occurs first.
> 
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.885
diff -r1.885 gdb.texinfo
10669c10669
< @item collect @var{expr1}, @var{expr2}, @dots{}
---
> @item collect@r{[}/@var{mods}@r{]} @var{expr1}, @var{expr2}, @dots{}
10714a10715,10723
> The optional @var{mods} changes the usual handling of the arguments.
> @code{s} requests that pointers to chars be handled as strings, in
> particular collecting the contents of the memory being pointed at, up
> to the first zero.  The upper bound is by default the value of the
> @code{print elements} variable; if @code{s} is followed by a decimal
> number, that is the upper bound instead.  So for instance
> @samp{collect/s25 mystr} collects as many as 25 characters at
> @samp{mystr}.
> 
34709a34719,34723
> @item @samp{tracenz}
> @tab No
> @tab @samp{-}
> @tab No
> 
34833a34848,34852
> @item tracenz
> @cindex string tracing, in remote protocol
> The remote stub supports the @samp{tracenz} bytecode for collecting strings.
> See @ref{Bytecode Descriptions} for details about the bytecode.
> 
Index: gdbserver/server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.149
diff -r1.149 server.c
1589a1590
> 	  strcat (own_buf, ";tracenz+");
Index: gdbserver/tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/tracepoint.c,v
retrieving revision 1.28
diff -r1.28 tracepoint.c
1211a1212,1214
> static int agent_mem_read_string (struct traceframe *tframe,
> 				  unsigned char *to, CORE_ADDR from,
> 				  ULONGEST len);
4646a4650,4656
> 	case gdb_agent_op_tracenz:
> 	  agent_mem_read_string (tframe, NULL, (CORE_ADDR) stack[--sp],
> 				 (ULONGEST) top);
> 	  if (--sp >= 0)
> 	    top = stack[sp];
> 	  break;
> 
4729a4740,4799
> static int
> agent_mem_read_string (struct traceframe *tframe,
> 		       unsigned char *to, CORE_ADDR from, ULONGEST len)
> {
>   unsigned char *buf, *mspace;
>   ULONGEST remaining = len;
>   unsigned short blocklen, i;
> 
>   /* To save a bit of space, block lengths are 16-bit, so break large
>      requests into multiple blocks.  Bordering on overkill for strings,
>      but it could happen that someone specifies a large max length.  */
>   while (remaining > 0)
>     {
>       size_t sp;
> 
>       blocklen = (remaining > 65535 ? 65535 : remaining);
>       /* We want working space to accumulate nonzero bytes, since
> 	 traceframes must have a predecided size (otherwise it gets
> 	 harder to wrap correctly for the circular case, etc).  */
>       buf = (unsigned char *) xmalloc (blocklen + 1);
>       for (i = 0; i < blocklen; ++i)
> 	{
> 	  /* Read the string one byte at a time, in case the string is
> 	     at the end of a valid memory area - we don't want a
> 	     correctly-terminated string to engender segvio
> 	     complaints.  */
> 	  read_inferior_memory (from + i, buf + i, 1);
> 
> 	  if (buf[i] == '\0')
> 	    {
> 	      blocklen = i + 1;
> 	      /* Make sure outer loop stops now too.  */
> 	      remaining = blocklen;
> 	      break;
> 	    }
> 	}
>       sp = 1 + sizeof (from) + sizeof (blocklen) + blocklen;
>       mspace = add_traceframe_block (tframe, sp);
>       if (mspace == NULL)
> 	{
> 	  xfree (buf);
> 	  return 1;
> 	}
>       /* Identify block as a memory block.  */
>       *mspace = 'M';
>       ++mspace;
>       /* Record address and size.  */
>       memcpy ((void *) mspace, (void *) &from, sizeof (from));
>       mspace += sizeof (from);
>       memcpy ((void *) mspace, (void *) &blocklen, sizeof (blocklen));
>       mspace += sizeof (blocklen);
>       /* Copy the string contents.  */
>       memcpy ((void *) mspace, (void *) buf, blocklen);
>       remaining -= blocklen;
>       from += blocklen;
>       xfree (buf);
>     }
>   return 0;
> }
> 
Index: common/ax.def
===================================================================
RCS file: /cvs/src/src/gdb/common/ax.def,v
retrieving revision 1.4
diff -r1.4 ax.def
89,90c89
< /* We need something here just to make the tables come out ok.  */
< DEFOP (invalid, 0, 0, 0, 0, 0x2f)
---
> DEFOP (tracenz, 0, 0, 2, 0, 0x2f)
Index: testsuite/gdb.trace/collection.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.trace/collection.c,v
retrieving revision 1.5
diff -r1.5 collection.c
203a204,218
> int strings_test_func ()
> {
>   int i = 0;
>   char *locstr, *longloc;
> 
>   locstr = "abcdef";
>   longloc = malloc(500);
>   strcpy(longloc, "how now brown cow spam spam spam wonderful wonderful spam");
> 
>   i += strlen (locstr);
>   i += strlen (longloc);
> 
>   return i;	/* Set_Tracepoint_Here */
> }
> 
265a281
>   i += strings_test_func ();
Index: testsuite/gdb.trace/collection.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.trace/collection.exp,v
retrieving revision 1.27
diff -r1.27 collection.exp
621a622,664
> proc gdb_collect_strings_test { func mystr myrslt mylim msg } {
>     global hex
>     global cr
>     global gdb_prompt
> 
>     prepare_for_trace_test
> 
>     # Find the comment-identified line for setting this tracepoint.
>     set testline 0
>     gdb_test_multiple "list $func, +30" "collect $msg: find tracepoint line" {
> 	-re "\[\r\n\](\[0-9\]+)\[^\r\n\]+ Set_Tracepoint_Here .*$gdb_prompt" {
> 	    set testline $expect_out(1,string)
> 	    pass "collect $msg: find tracepoint line"
> 	}
> 	-re ".*$gdb_prompt " {
> 	    fail "collect $msg: find tracepoint line (skipping strings test)"
> 	    return
> 	}
> 	timeout {
> 	    fail "collect $msg: find tracepoint line (skipping strings test)"
> 	    return
> 	}
>     }
> 
>     gdb_test "trace $testline" \
> 	    "Tracepoint \[0-9\]+ at .*" \
> 	    "collect $msg: set tracepoint"
>     gdb_trace_setactions "collect $msg: define actions" \
> 	    "" \
> 	    "collect/s$mylim $mystr" "^$"
> 
>     # Begin the test.
>     run_trace_experiment $msg $func
> 
>     gdb_test "print $mystr" \
> 	"\\$\[0-9\]+ = $hex \"$myrslt\".*$cr" \
> 	    "collect $msg: collected local string"
> 
>     gdb_test "tfind none" \
> 	    "#0  end .*" \
> 	    "collect $msg: cease trace debugging"
> }
> 
730a774,780
> 
>     gdb_collect_strings_test strings_test_func "locstr" "abcdef" "" \
> 	    "local string"
> 
>     gdb_collect_strings_test strings_test_func "longloc" "how now brown c" 15 \
> 	    "long local string"
>