[PATCH] tracepoint: add new trace command "printf"[0] gdb
Tom Tromey
tromey@redhat.com
Fri Feb 11 18:45:00 GMT 2011
>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:
>> + gen_expr (expr, &pc, aexpr, &value);
>> +
>> +
>> + if (value.optimized_out)
There's no need to have 2 blank lines here.
This function and some other new ones have no documentation.
>> + {"printf", 0, 0, 0, 0}, /* 0x31 */
[...]
>> + aop_printf = 0x31,
If you add a new AX expression, you must update agentexpr.texi.
I think any AX addition should also require a corresponding addition to
gdbserver.
>> +typedef void (printf_callback) (char *fbuf, char **expp,
>> + struct bp_location *loc,
>> + struct agent_expr *aexpr);
>From what I can see, the `loc' and `aexpr' arguments are passed through
string_printf without interpretation.
In a case like this it is customary to just add a single `void *data'
argument and have the caller and callback agree on the type. Here, that
type would be an instance of a struct wrapping the two values.
This definition should not be here.
>> static void
>> +ui_printf (char *arg, struct ui_file *stream)
>> +{
>> + string_printf (arg, stream, NULL, NULL, NULL);
>> +}
There's no reason to keep ui_printf around, just inline this into the 2
callers.
>> +extern void printf_command (char *arg, int from_tty);
>> +typedef void (printf_callback) (char **expp, struct bp_location *loc,
>> + struct agent_expr *aexpr);
>> +extern void string_printf (char *arg, struct ui_file *stream,
>> + printf_callback callback, struct bp_location *loc,
>> + struct agent_expr *aexpr);
These should be in some appropriate header, not tracepoint.c.
>> + /* The agent expr include expr for arguments, format string, 1 byte
>> + * for aop_printf, 1 byte for the number of arguments, 1 byte for
>> + * size of format string, 1 byte for blank after format string
>> + * and 1 byte for aop_end. */
Wrong comment format, no leading "*"s.
This new feature needs a documentation patch and probably also a patch
to NEWS.
>From what I can tell from the patch, the idea here is to add a printf to
a tracepoint's actions, with the printf evaluated on the remote side. I
guess that is an ok idea, though I don't use this stuff enough to really
have an opinion.
I think it would be good for other maintainers to speak up now. If it
is left to me, I will allow this if you fix up the problems and write
the documentation.
Tom
More information about the Gdb-patches
mailing list