This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] dynamic printf


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]