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]

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


On Thu, Feb 17, 2011 at 16:15, Hui Zhu <teawater@gmail.com> wrote:
> 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.
>

Hi guys,

Patch in attachment checked in.

Thanks,
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-gdb.h (gen_printf_expr_callback): Forward declare.
	* ax-general.c (ax_memcpy): New function.
	(aop_map): Add new entry for "printf".
	(ax_print): Handle "printf".
	(ax_reqs): Ditto.
	* ax.h (ax_memcpy): Forward declare.
	* common/ax.def (invalid2): Removed.
	(printf): New entry.
	* printcmd.c (printcmd.h): New include.
	(string_printf): New function.
	(ui_printf): Removed.
	(printf_command): Remove static.  Call string_printf.
	(eval_command): Call string_printf.
	* 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]