From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31977 invoked by alias); 5 Feb 2013 12:08:56 -0000 Received: (qmail 31967 invoked by uid 22791); 5 Feb 2013 12:08:55 -0000 X-SWARE-Spam-Status: No, hits=-6.8 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_RG 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; Tue, 05 Feb 2013 12:08:46 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r15C8kL6022184 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 5 Feb 2013 07:08:46 -0500 Received: from localhost.localdomain (ovpn-116-47.ams2.redhat.com [10.36.116.47]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r15C8i6C008727; Tue, 5 Feb 2013 07:08:45 -0500 Message-ID: <5110F64C.2040907@redhat.com> Date: Tue, 05 Feb 2013 12:08:00 -0000 From: Phil Muldoon MIME-Version: 1.0 To: Tom Tromey CC: "gdb-patches@sourceware.org" Subject: Re: [patch][python] 2 of 5 - Frame filter MI code changes. References: <50B8C333.4070008@redhat.com> <87ip8gtoex.fsf@fleche.redhat.com> <50C1F56E.5060506@redhat.com> <87sj7dd3i7.fsf@fleche.redhat.com> In-Reply-To: <87sj7dd3i7.fsf@fleche.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: 2013-02/txt/msg00114.txt.bz2 On 12/10/2012 09:02 PM, Tom Tromey wrote: >>>>>> "Phil" == Phil Muldoon writes: > > Tom> I'd prefer it if the various callers were changed to use mi_getopt. > Tom> This provides uniformity and lets us add options later. > > Phil> If there was uniformity then I would agree, but as far as I looked > Phil> there wasn't. Some MI commands use mi_getopt, some parse their own > Phil> options, some allow long options ("--"), others do not, and mi_getopt > Phil> does not handle long options in any case (and huge amounts of other > Phil> useful getopt functions too). I wrote a patch for mi_getopts to > Phil> handle long options, but why do we even need another implementation of > Phil> getopt like functionality? > > Ok, I see the problem now. > > Right now, though. -stack-list-frames doesn't take any options. > I think it would be better if commands like this were converted to use > mi_getopt instead of doing the parsing by hand. > So I suggest having all the functions use a short option name. > > I think the changes like in -stack-list-locals are strange, too, in that > they make options order-dependent. This will be surprising to users. I spent the last couple of weeks trying to fix this, and I have reached a dead end with your review requirements. First, these commands are already option placement dependent. Lets look at an example before the frame filters patch: mi_cmd_stack_list_args: stack-list-arguments: Usage: PRINT_VALUES [FRAME_LOW FRAME_HIGH] You can see in that command PRINT_VALUES is mandatory and is an option. Its placement in the argument order is dependent on being the first argument. The existing MI code already expects this to be at argv[0]. Here is an example instantiation of this command for a slice of frames: -stack-list-arguments --all-values 0 24 Or this for the whole backtrace: -stack-list-arguments --all-values If these were the only cases this would be fine. However the commands highlighted in the review also accept integer representations for options (and also why order placement is important to this (and other) commands): -stack-list-arguments 1 0 24 Where the first "1" is an integer value replacement for the --all-values option. Some MI commands, for whatever reason, allow these integer replacements. (As an aside, some MI commands parse their own argv and opt-out of using mi_getopt for this reason). So this is all ok so far. I patched mi_getopt locally to process "--foo" style options. This would still work: -stack-list-arguments --no-frame-filters 1 0 24 But this would not: -stack-list-arguments 1 --no-frame-filters 0 24 mi_getopt (quite rightly) detects the "1" as not an option and returns -1 as an indication that we have reached the end of the options in the command line. At this point --no-frame-filters has not been processed). I followed the logic that if the review does not want option placement dependency, then all options should not be placement dependent. I thought about running a substitution for the command line that would substitute integer options with their long equivalent (IE, substitute 1 for --all-values). I am uncomfortable with this as it reallocates the size of argv for every command that allows integer option substitution. I also thought about just running mi_getopt manually on every index in argv and ignoring the return value, thereby forcing mi_getopt to process each element in the argv array. The problem with this is then I lose access to oind which indicates the end of the arguments, and the beginning of the rest of the command line (in the examples case, the slice). The next thing I thought about was just removing integer option replacement. That turned out to be a non-starter as we would lose MI compatibility with previous versions. So I'm at a loss. I do not know how to write this code with the review requirements that allows: -- options not being order dependent; -- not heaping more hacks (like -- argv pre-substitution) on top of existing bad behavior. This is all somewhat tangential to the frame filters patch. While I do not mind fixing issues as I go, this is becoming an issue far more contentious than the patch content. What do you think we should do? Cheers, Phil > > Phil> I wanted to mention something else about MI. I recently discovered in > Phil> the GDB manual that -stack-list-locals, -stack-list-arguments are > Phil> considered depreciated. Not even sure if we should add frame filter > Phil> logic to them. What do you think? > > I think we still ought to. It doesn't seem like much work and it is > friendlier in case some MI user hasn't updated. > > Tom> I don't think I follow the high/low logic here. > > Tom> How does this code strip off the first 'frame_low' frames? > > Phil> fi is unwound to the position of frame_low in a loop preceding this > Phil> call. This is existing code, and not in the patch context. > > Tom> Also, Do frame_low and frame_high refer to "raw" or "cooked" frames? > Tom> I tend to think they should refer to cooked ones, but I think at least > Tom> the answer should be explicit and documented. > > Phil> In the existing mi sense, they just refer to frames on the stack. I > Phil> followed this logic, but something I am still unsure of is if a frame > Phil> is elided between frame low, and frame high, if that should be > Phil> counted. I think it should. > > I see. I think this is wrong then. I think the arguments have to be > applied after filtering. Here's why: > > One use for these arguments to the stack commands is so that a GUI can > display just part of the stack, and then, if the user scrolls the > display, it can request more of the stack to show. This way the GUI > doesn't have to unwind the whole stack just to show the first 5 lines or > whatever. > > Suppose you have a frame filter that notices a sentinel frame and then > elides the following 3 frames. This isn't necessarily an obscure > situation, maybe some interpreter takes a few function calls to > implement a call in the interpreted language and the filter wants to > collapse those frames to just show what is happening in the interpreted > code. > > Suppose this sentinel frame occurs at stack position 3. > > Now suppose the MI client requests frames 0..4. Ok, the filter will > work (since it can request more frames than the MI client did) and will > present the right thing. > > But now the user scrolls the window and the MI client requests frames > starting at frame 5. > > Whoops -- the output will be inconsistent; the filter won't trigger > because the sentinel appears at frame 3, which the existing code already > iterated past. > > Tom >