Re: [PATCH] dynamic printf

[Stan Shebs <stanshebs@earthlink.net>]

On 3/8/12 1:08 PM, Tom Tromey wrote:
>>>>>> "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.

Hmmm.  The format is a string though; would linespec parsing attempt to 
proceed into that?  How is "break <location> if foo" working these days?
>
> 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.

Yeah, I should make a separate patch to break out the format string parsing.

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

I think the right thing is to make up a package for post-location 
modifiers to break-type commands.  We're up to condition, thread, task, 
and now command, and maybe process and itset soon, all being passed as 
separate arguments everywhere.

>
> Stan>  +  struct agent_expr *cmd_bytecode;
>
> Needs a comment.
> Perhaps subclassing bp_location is also doable?

I'm not sure what you mean by this?

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

Yeah, literal printf is just having compounding problems.  Among other 
things, the args are all 64-bit values on the stack, so something like a 
4-byte-getting "%d" can mess up vararg fetching bigtime.  The ui_printf 
code would make sense (and for the same reason), in that we are looking 
at each format char so as to know how to cast the data value, but 
ultimately relying on the library function to turn the value into 
characters.

Stan