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

[Tom Tromey <tromey@redhat.com>]

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