Re: [PATCH] Dynamic printf for a target agent

[Tom Tromey <tromey@redhat.com>]

>>>>> "Stan" == Stan Shebs <stanshebs@earthlink.net> writes:

Stan> +static void
Stan> +maint_agent_printf_command (char *exp, int from_tty)
Stan> +{
[...]
Stan> +      expr = parse_exp_1 (&cmd1, (struct block *) 0, 1);

Now that Hui's "-at" patch is going in, perhaps this function should
have the same treatment.

Stan> +void
Stan> +ax_string (struct agent_expr *x, char *str, int slen)
Stan> +{
Stan> +  int i;
Stan> +
Stan> +  grow_expr (x, slen + 3);
Stan> +  x->buf[x->len++] = ((slen + 1) >> 8) & 0xff;
Stan> +  x->buf[x->len++] = (slen + 1) & 0xff;

I think this should check that the length fits in 2 bytes.

Stan> +@item @code{printf} (0x34)  @var{numargs} @var{string} @result{}
Stan> +Do a formatted print, in the style of the C function @code{printf}).
Stan> +The value of @var{numargs} is the number of arguments to expect on the
Stan> +stack, while @var{string} is the format string, prefixed with a
Stan> +two-byte length, and suffixed with a zero byte.  The format string

I think the docs should whether the length includes the zero byte.

Stan> +	case string_arg:
Stan> +	  {
Stan> +	    gdb_byte *str;
Stan> +	    CORE_ADDR tem;
Stan> +	    int j;
Stan> +
Stan> +	    tem = args[i];
Stan> +
Stan> +	    /* This is a %s argument.  Find the length of the string.  */
Stan> +	    for (j = 0;; j++)
Stan> +	      {
Stan> +		gdb_byte c;
Stan> +
Stan> +		read_inferior_memory (tem + j, &c, 1);
Stan> +		if (c == 0)
Stan> +		  break;
Stan> +	      }
Stan> +
Stan> +	      /* Copy the string contents into a string inside GDB.  */
Stan> +	      str = (gdb_byte *) alloca (j + 1);
Stan> +	      if (j != 0)
Stan> +		read_inferior_memory (tem, str, j);
Stan> +	      str[j] = 0;
Stan> +
Stan> +              printf (current_substring, (char *) str);

Is it ever possible for the argument to "%s" to be NULL?  It seems like
it should be; but then the length-finding code seems wrong, and the
printing of a NULL should be handled directly, not left to the host
printf.

Stan> +	case gdb_agent_op_printf:
Stan> +	  {
Stan> +	    int nargs, slen, i;
Stan> +	    CORE_ADDR fn = 0, chan = 0;
Stan> +	    /* Can't have more args than the entire size of the stack.  */
Stan> +	    ULONGEST args[STACK_MAX];
Stan> +	    char *format;
Stan> +
Stan> +	    nargs = aexpr->bytes[pc++];
Stan> +	    slen = aexpr->bytes[pc++];
Stan> +	    slen = (slen << 8) + aexpr->bytes[pc++];
Stan> +	    format = (char *) &(aexpr->bytes[pc]);

Perhaps double-check that the terminating \0 byte is in fact present.

Stan> +if $target_can_dprintf {
Stan> +
Stan> +    gdb_run_cmd
Stan> +
Stan> +    gdb_test "" "Breakpoint"

This seems weird.

FWIW I rather liked the format-parsing refactoring.

Tom