This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Dynamic printf for a target agent
- From: Tom Tromey <tromey at redhat dot com>
- To: Stan Shebs <stanshebs at earthlink dot net>
- Cc: Yao Qi <yao at codesourcery dot com>, gdb-patches at sourceware dot org
- Date: Wed, 27 Jun 2012 14:44:59 -0600
- Subject: Re: [PATCH] Dynamic printf for a target agent
- References: <4FC57340.6070306@earthlink.net> <4FC70975.7020604@codesourcery.com> <4FE8841A.7090100@earthlink.net>
>>>>> "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