Re: [PATCH] dynamic printf

[Tom Tromey <tromey@redhat.com>]

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