From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27115 invoked by alias); 13 Mar 2012 23:51:38 -0000 Received: (qmail 27106 invoked by uid 22791); 13 Mar 2012 23:51:37 -0000 X-SWARE-Spam-Status: No, hits=-1.5 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_NONE,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from elasmtp-banded.atl.sa.earthlink.net (HELO elasmtp-banded.atl.sa.earthlink.net) (209.86.89.70) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 13 Mar 2012 23:51:24 +0000 Received: from [68.96.200.16] (helo=macbook2.local) by elasmtp-banded.atl.sa.earthlink.net with esmtpa (Exim 4.67) (envelope-from ) id 1S7bUh-0004xq-VV; Tue, 13 Mar 2012 19:51:08 -0400 Message-ID: <4F5FDD6B.3030507@earthlink.net> Date: Tue, 13 Mar 2012 23:51:00 -0000 From: Stan Shebs User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0.2) Gecko/20120216 Thunderbird/10.0.2 MIME-Version: 1.0 To: Tom Tromey CC: gdb-patches@sourceware.org Subject: Re: [PATCH] dynamic printf References: <4F4DCDD5.2040807@earthlink.net> <87ipien6me.fsf@fleche.redhat.com> In-Reply-To: <87ipien6me.fsf@fleche.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-ELNK-Trace: ae6f8838ff913eba0cc1426638a40ef67e972de0d01da94076cad54e5d45e3247df2c1012951aec6350badd9bab72f9c350badd9bab72f9c350badd9bab72f9c X-IsSubscribed: yes 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: 2012-03/txt/msg00461.txt.bz2 On 3/8/12 1:08 PM, Tom Tromey wrote: >>>>>> "Stan" == Stan Shebs writes: > Stan> This patch implements a "dynamic printf", which is basically a > Stan> breakpoint with a printf;continue as its command list - but with > Stan> additional features that make it more interesting. > > Very nice. I implemented something like this in Python once, but > without the cool remote agent features. > > Stan> dprintf ,,... > Stan> where the location is as for breakpoints, while the format and args > Stan> are as for the printf command. So you could have something like > > I think you have to have a comma after the location. That is the only > reliable linespec terminator. Hmmm. The format is a string though; would linespec parsing attempt to proceed into that? How is "break if foo" working these days? > > Stan> The patch itself is somewhat of a hack-n-slash through the middle of > Stan> GDB, and there is much to critique. :-) > > I have a few things, but nothing really major. Some of this is outside > areas I know much about, but I did at least skim it all and I think it > generally looks good. > > Stan> +/* Parse the given expression, compile it into an agent expression > Stan> + that does a printf, and display the resulting expression. */ > Stan> + > Stan> +extern char *parse_format_string (char **arg); > > There must be a header that this could go in. Yeah, I should make a separate patch to break out the format string parsing. > Stan> +/* Temporary hack to smuggle remainder of command line through. */ > Stan> +char *glob_extra_string = NULL; > > I'd much prefer something cleaner. > There's been a lot of work in recent times to clean up breakpoints in > various ways -- adding methods, refactorings, etc -- and there is more > to come. This goes against the trend. I think the right thing is to make up a package for post-location modifiers to break-type commands. We're up to condition, thread, task, and now command, and maybe process and itset soon, all being passed as separate arguments everywhere. > > Stan> + struct agent_expr *cmd_bytecode; > > Needs a comment. > Perhaps subclassing bp_location is also doable? I'm not sure what you mean by this? > > Stan> + case gdb_agent_op_printf: > [...] > Stan> + /* (should re-check format before calling?) */ > Stan> + printf (format, > Stan> + args[0], args[1], args[2], args[3], args[4], > Stan> + args[5], args[6], args[7], args[8], args[9]); > > Yeah, this seems overly optimistic to me. > gdb used to have bugs where bad printf commands would crash it (IIRC > there are some closed PRs about this). > Maybe common-izing some code from ui_printf is the right thing? Or some > kind of format checking. Also this leaves you at the mercy of the host > printf for the '"%s", 0' case. > Yeah, literal printf is just having compounding problems. Among other things, the args are all 64-bit values on the stack, so something like a 4-byte-getting "%d" can mess up vararg fetching bigtime. The ui_printf code would make sense (and for the same reason), in that we are looking at each format char so as to know how to cast the data value, but ultimately relying on the library function to turn the value into characters. Stan