On Sat, Feb 12, 2011 at 02:45, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> ">" == 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
>

Thanks for your help Tom.

I make a new patch according to your comments.  And I have send the
patch for doc in prev mail.

Best,
Hui

2011-02-17  Hui Zhu  <teawater@gmail.com>

	* Makefile.in (HFILES_NO_SRCDIR): Add printcmd.h.
	* ax-gdb.c (gen_printf_expr_callback): New function.
	* ax-general.c (ax_memcpy): New function.
	(aop_map): Add new entry for "printf".
	(ax_print): Handle "printf".
	(ax_reqs): Ditto.
	* ax.h (agent_op): Add aop_printf.
	(ax_memcpy): Forward declare.
	* printcmd.c (printf_callback): New typedef.
	(string_printf): New function from ui_printf.
	(ui_printf): Call string_printf.
	(printf_command): Remove static.
	* printcmd.h: New file.
	* tracepoint.c (printf_command, gen_printf_expr_callback,
	printf_callback, string_printf): Forward declares.
	(validate_actionline, encode_actions_1): handle printf_command.