This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] tracepoint: add new trace command "printf"[0] gdb
- From: Tom Tromey <tromey at redhat dot com>
- To: Hui Zhu <teawater at gmail dot com>
- Cc: gdb-patches ml <gdb-patches at sourceware dot org>, Stan Shebs <stan at codesourcery dot com>
- Date: Fri, 11 Feb 2011 11:45:20 -0700
- Subject: Re: [PATCH] tracepoint: add new trace command "printf"[0] gdb
- References: <AANLkTimhMH+qUE1NmQ+f6+L8BcEU+k07eU+M9B-gLsrk@mail.gmail.com> <AANLkTik=K4Dk-Mkc5P7DDofzqwThhE6LvCM4rT-U_LNt@mail.gmail.com> <AANLkTiktJB=V0KS0u31rWMA6CyoV6J3+9Yufcgr=efr-@mail.gmail.com> <AANLkTinOj2JE8U=cu1ntVfh7nmTwnKDTox22da09vfyF@mail.gmail.com> <4D24D9DD.8090104@codesourcery.com> <AANLkTi=K3tw6_-uYDL8W4mSaL6nFPx-N541ALMC9-JZX@mail.gmail.com> <AANLkTik3mLyEUb4wRCbuG4X10=+Dp0_myYSC+kSjkzbL@mail.gmail.com> <AANLkTinOejC4S1cwA_uxdjWbpveLvR7C==mJMSSrw-aH@mail.gmail.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