Re: [PATCH] tracepoint: add new trace command "printf"[0] gdb

Tom Tromey
2011-02-11T18:45:48Z

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