Re: [PATCH] dynamic printf

Tom Tromey
2012-03-08T21:08:31Z

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