From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18104 invoked by alias); 7 Dec 2012 19:03:21 -0000 Received: (qmail 18017 invoked by uid 22791); 7 Dec 2012 19:03:18 -0000 X-SWARE-Spam-Status: No, hits=-6.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS 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, 07 Dec 2012 19:03:06 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qB7J350N011007 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 7 Dec 2012 14:03:05 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id qB7J34YP014199 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 7 Dec 2012 14:03:04 -0500 From: Tom Tromey To: Phil Muldoon Cc: "gdb-patches\@sourceware.org" Subject: Re: [patch][python] 1 of 5 - Frame filter Python C code changes. References: <50B8C322.8050602@redhat.com> <87mwxstos6.fsf@fleche.redhat.com> <50C1F210.4090509@redhat.com> Date: Fri, 07 Dec 2012 19:03:00 -0000 In-Reply-To: <50C1F210.4090509@redhat.com> (Phil Muldoon's message of "Fri, 07 Dec 2012 13:41:36 +0000") Message-ID: <87ip8dvg5z.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2.90 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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-12/txt/msg00202.txt.bz2 >>>>> "Phil" == Phil Muldoon writes: Phil> I was just trying to show a case where you have frame wrappers, Phil> wrapping other frame wrappers, etc, until you get to BaseFrameWrapper Phil> which wraps a gdb.Frame. What notation should I use to illustrate Phil> this? Yeah, I don't know. Maybe spell it out more. Phil> + if len(lvars) == 0: Phil> + return None Tom> Tom> It is curious that this is needed. Tom> Will whatever code eventually handles display of this do something Tom> different for an empty list? Phil> If this function returns None, then GDB will not print any frame Phil> locals. It can probably be written differently in the Python code. Phil> But fetch_frame_locals has to return a None, or an iterator Yeah, but what happens if the iterator has no elements? It seems to me that this should work the same as the None case. But then you don't need the special check above. Tom> It seems strange to have both _get_sort_priority and _get_priority. Tom> Similarly elsewhere. Phil> This was because the Python sort/filter functions (in invoke) stop if Phil> it encounters an error. So if there is a badly implemented filter Phil> (say one that does not have a "priority" attribute) then all frame Phil> filters will not run. This was to try and make the system more Phil> robust. It seems even more robust to just use the safe variant everywhere. Phil> Sure, but I think if a class does not have an attribute which is Phil> mandatory, we should not just set it blindly. I think it is fine. It is mandatory and documented as such. Phil> + if hasattr(filter_item, "enabled"): Phil> + return filter_item.enabled Phil> + else: Phil> + raise gdb.GdbError("Cannot find class attribute 'enabled'") Tom> Tom> What is wrong with just fetching it and letting Python throw the Tom> exception? Phil> This was a case again, of trying to make the exception more relevant Phil> to the user where a frame filter is badly implemented. I have no Phil> strong feelings here either. I think the standard message is just as clear: AttributeError: 'str' object has no attribute 'method' Tom> If the 'invoke' function is meant to be used from outside the new Tom> commands, it should probably not be in gdb.command. Phil> It was easier to write "invoke" in Python than in C, and yes it is Phil> only meant for GDB to call. Good point on the gdb.Command issue Phil> though. Yeah, I don't mind it being in Python. I think it is better that way too. Phil> + DICTIONARY is the name of the frame filter dictionary on which to Phil> + operate. Named dictionaries are: "global" for the global frame Phil> + filter dictionary, "progspace" for the program space's framefilter Phil> + dictionary. If either of these two are not specified, the Phil> + dictionary name is assumed to be the name of the object-file name. Tom> Tom> It would be nice if these commands had completion methods that did the Tom> right thing. Phil> Completion of the printer name? Completion of all the arguments - the printer name but also the dictionary. Phil> +extract_sym (PyObject *obj, char **name, struct symbol **sym, Phil> + const struct language_defn **language) Tom> [...] Phil> + *language = current_language; Tom> Tom> Why current_language and not python_language? Phil> I guess I am not clear on the difference between the two? There may not be a difference here, but it is weird to read the Python code and see a reference to current_language when python_language exists. Tom> Phil> + if (ui_out_is_mi_like_p (out)) Phil> + { Phil> + struct type *type; Phil> + Phil> + type = check_typedef (value_type (val)); Phil> + if (mi_print_type == PRINT_ALL_VALUES) Phil> + should_print = 1; Phil> + else if (mi_print_type == PRINT_SIMPLE_VALUES Phil> + && TYPE_CODE (type) != TYPE_CODE_ARRAY Phil> + && TYPE_CODE (type) != TYPE_CODE_STRUCT Phil> + && TYPE_CODE (type) != TYPE_CODE_UNION) Phil> + should_print = 1; Phil> + } Tom> Tom> Rather than making this all conditional, you could make the API simpler Tom> to understand by having the CLI always pass PRINT_ALL_VALUES. Phil> I am not sure what you mean here with CLI, as this is an MI case? Sorry, what I mean is, don't check ui_out_is_mi_like_p at all, just always respect the "print type", and have the CLI code pass in the appropriate value to get the behavior it wants. Phil> + int mi_print_type, Phil> + int print_mi_args_flag, Phil> + const char *cli_print_frame_args_type, Tom> Tom> cli_print_frame_args_type isn't documented in the function comment. Tom> The comment refers to print_mi_args, not print_mi_args_flag. Tom> This seems like a lot of duplication to me. Tom> Why not use a single enum for all cases? That would be a lot simpler. Tom> It is fine to introduce a new enum just for this code. Phil> Because CLI and MI use those enums internally, and each print values Phil> differently depending on type. So I get those values from MI/CLI. Phil> They provide them, I did not create them. But I still have to account Phil> for their instructions to match existing CLI/MI expectations of output. I think it is better to present a simpler API to the callers. It is fine to introduce new enums and to make the CLI and MI code convert their requests. Or, better, unify the enums across the entire code base. Phil> + module = PyImport_ImportModule ("gdb.command.frame_filters"); Tom> Tom> Yeah, definitely should be a different module. Tom> Tom> I think 'invoke' should be renamed, too; and documented. Tom> It is nice for things like this if there is an obvious way from Python Tom> to accomplish the same thing that gdb does internally. We didn't think Tom> of this for value pretty printers (though you can still do it), but I'd Tom> like us to consider it for new additions. Phil> It was very much simpler to write "invoke" in Python, than in C. In Phil> fact where I could write something in Python, I did. Most of the C Phil> code is just printing the frames. Yes, my comment isn't about C, but rather which module the function lives in. I think something like 'gdb.frames' would be better, analogous to gdb.types. And, rename the function and document it so that user code can call it to easily iterate over filtered frames. Tom> You're creating cleanups for everything except this. Tom> It doesn't really matter, since this code is not exposed to gdb Tom> exceptions, but I wonder why. Simpler to make it all uniform. Phil> This was a late addition to track frames printed and is simply a Phil> blimp. I think I did not create a cleanup for this as it will always Phil> fall through to the free. The question is just about uniformity. All code in that function will fall through. So I think it should either do explicit frees everywhere, or use cleanups everywhere. Tom