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] |
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.
Attachment:
tp_print.txt
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |