From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11043 invoked by alias); 11 Feb 2011 18:45:48 -0000 Received: (qmail 11033 invoked by uid 22791); 11 Feb 2011 18:45:47 -0000 X-SWARE-Spam-Status: No, hits=-6.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 11 Feb 2011 18:45:31 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p1BIjMG3020630 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 11 Feb 2011 13:45:22 -0500 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p1BIjMng015701; Fri, 11 Feb 2011 13:45:22 -0500 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id p1BIjLX9006419; Fri, 11 Feb 2011 13:45:21 -0500 Received: by opsy.redhat.com (Postfix, from userid 500) id DD2CC378338; Fri, 11 Feb 2011 11:45:20 -0700 (MST) From: Tom Tromey To: Hui Zhu Cc: gdb-patches ml , Stan Shebs Subject: Re: [PATCH] tracepoint: add new trace command "printf"[0] gdb References: <4D24D9DD.8090104@codesourcery.com> Date: Fri, 11 Feb 2011 18:45:00 -0000 In-Reply-To: (Hui Zhu's message of "Fri, 4 Feb 2011 23:59:02 +0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-02/txt/msg00214.txt.bz2 >>>>> ">" == Hui Zhu 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