This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] dynamic printf
- From: Tom Tromey <tromey at redhat dot com>
- To: Stan Shebs <stanshebs at earthlink dot net>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 08 Mar 2012 14:08:09 -0700
- Subject: Re: [PATCH] dynamic printf
- References: <4F4DCDD5.2040807@earthlink.net>
>>>>> "Stan" == Stan Shebs <stanshebs@earthlink.net> writes:
Stan> This patch implements a "dynamic printf", which is basically a
Stan> breakpoint with a printf;continue as its command list - but with
Stan> additional features that make it more interesting.
Very nice. I implemented something like this in Python once, but
without the cool remote agent features.
Stan> dprintf <location> <format>,<arg>,<arg>...
Stan> where the location is as for breakpoints, while the format and args
Stan> are as for the printf command. So you could have something like
I think you have to have a comma after the location. That is the only
reliable linespec terminator.
Stan> The patch itself is somewhat of a hack-n-slash through the middle of
Stan> GDB, and there is much to critique. :-)
I have a few things, but nothing really major. Some of this is outside
areas I know much about, but I did at least skim it all and I think it
generally looks good.
Stan> +/* Parse the given expression, compile it into an agent expression
Stan> + that does a printf, and display the resulting expression. */
Stan> +
Stan> +extern char *parse_format_string (char **arg);
There must be a header that this could go in.
Stan> +#if 0
:)
Stan> +char *
Stan> +parse_format_string (char **arg)
Need intro comment.
This looks like bits were copied from printcmd.c:ui_printf.
Could the code be shared instead?
Stan> +/* Temporary hack to smuggle remainder of command line through. */
Stan> +char *glob_extra_string = NULL;
I'd much prefer something cleaner.
There's been a lot of work in recent times to clean up breakpoints in
various ways -- adding methods, refactorings, etc -- and there is more
to come. This goes against the trend.
Stan> + struct agent_expr *cmd_bytecode;
Needs a comment.
Perhaps subclassing bp_location is also doable?
Stan> + case gdb_agent_op_printf:
[...]
Stan> + /* (should re-check format before calling?) */
Stan> + printf (format,
Stan> + args[0], args[1], args[2], args[3], args[4],
Stan> + args[5], args[6], args[7], args[8], args[9]);
Yeah, this seems overly optimistic to me.
gdb used to have bugs where bad printf commands would crash it (IIRC
there are some closed PRs about this).
Maybe common-izing some code from ui_printf is the right thing? Or some
kind of format checking. Also this leaves you at the mercy of the host
printf for the '"%s", 0' case.
Tom