From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14310 invoked by alias); 25 Apr 2013 19:24:42 -0000 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 Received: (qmail 14298 invoked by uid 89); 25 Apr 2013 19:24:42 -0000 X-Spam-SWARE-Status: No, score=-6.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Thu, 25 Apr 2013 19:24:40 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3PJOd54003262 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 25 Apr 2013 15:24:39 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r3PJObfh009957 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 25 Apr 2013 15:24:38 -0400 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: <51753B9D.4050502@redhat.com> Date: Fri, 26 Apr 2013 06:47:00 -0000 In-Reply-To: <51753B9D.4050502@redhat.com> (Phil Muldoon's message of "Mon, 22 Apr 2013 14:31:09 +0100") Message-ID: <87k3nqh0qy.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2013-04/txt/msg00793.txt.bz2 >>>>> "Phil" == Phil Muldoon writes: Phil> This email and patch covers the python/ changes for Python Frame Filters. Thanks. Phil> diff --git a/gdb/python/lib/gdb/FrameIterator.py b/gdb/python/lib/gdb/FrameIterator.py ... Phil> +class FrameIterator(object): ... Phil> + def __getitem__(self, index): Phil> + """__getitem__ implementation. Phil> + Phil> + Arguments: Phil> + index: A specific index to fetch.""" Phil> + Phil> + return next(itertools.islice(self.frame, index, index+1)) I don't think this will actually work. self.frame isn't an iterator, so I don't think islice will do the right thing here. I don't think this method is needed. Iterators only really need 'next' (though supplying __iter__ is also nice). Phil> + if hasattr(filter_item, "priority"): Phil> + return filter_item.priority Phil> + else: Phil> + return 0 You can use getattr(filter_item, "priority", 0) here Phil> + if hasattr(filter_item, "enabled"): Phil> + return filter_item.enabled Phil> + else: Phil> + return False getattr here too. Phil> +static enum py_bt_status Phil> +extract_sym (PyObject *obj, char **name, struct symbol **sym, Phil> + const struct language_defn **language) It occurs to me now that this return value convention is going to make this code less amenable to analysis with the CPython checker. Hmm, maybe it would work ok if we set PY_BT_ERROR = -1. Then we could use the "negative means failure" attribute. Phil> +static enum py_bt_status Phil> +extract_value (PyObject *obj, struct value **value) ... Phil> + return 1; Should be one of the enum values, I guess PY_BT_OK. Phil> +static enum py_bt_status Phil> +py_print_value (struct ui_out *out, struct value *val, Many of the callers of this make assumptions about the return value, e.g.: + if (! py_print_type (out, val)) + goto error; These have to be checks against enum constants. The same goes for py_print_value and py_print_single_arg and more. I feel like this has come up in every review. Phil> + /* Never set an indent level for common_val_print if MI. */ Phil> + if (ui_out_is_mi_like_p (out)) Phil> + local_indent = 0; The second line here is intended too much. Phil> + /* MI does not print certain values, differentiated by type, Phil> + depending on what ARGS_TYPE indicates. Test type against option. Phil> + For CLI print all values. */ Phil> + if (args_type == MI_PRINT_SIMPLE_VALUES Phil> + || args_type == MI_PRINT_ALL_VALUES) Phil> + Phil> + { Extra blank line here. Phil> +static enum py_bt_status Phil> +py_print_single_arg (struct ui_out *out, ... Phil> + /* MI has varying rules for tuples, but generally if there is only Phil> + one element in each item in the list, do not start a tuple. The Phil> + exception is -stack-list-variables which emits an ARGS="1" field Phil> + if the value is a frame argument. This is denoted in this Phil> + function with PRINT_ARGS_FIELD which is flag from the caller to Phil> + emit the ARGS field. */ Phil> + if (ui_out_is_mi_like_p (out)) Phil> + { Phil> + if (print_args_field || args_type != NO_VALUES) Phil> + make_cleanup_ui_out_tuple_begin_end (out, NULL); Phil> + } Phil> + Phil> + TRY_CATCH (except, RETURN_MASK_ALL) Phil> + { I think the make_cleanup_ui_out_tuple_begin_end call should probably go inside this TRY_CATCH. Phil> +static enum py_bt_status Phil> +enumerate_locals (PyObject *iter, ... Phil> + make_cleanup (null_cleanup, NULL); I don't think this is needed. Phil> + do_cleanups (cleanups); Phil> + } I think this call is wrong. In particular if the loop runs more than once, it will try to re-do a cleanup that was already run. Maybe you meant to introduce a new inner cleanup in the loop, and that was what the null_cleanup was for. Phil> + if (item == NULL && PyErr_Occurred ()) Phil> + goto error; Phil> + Phil> + return PY_BT_OK; Phil> + Phil> + error: Phil> + do_cleanups (cleanups); Phil> + return PY_BT_ERROR; Similarly, this should do the cleanups along both branches, or neither branch. Phil> +static enum py_bt_status Phil> +py_print_frame (PyObject *filter, int flags, enum py_frame_args args_type, ... Phil> + /* stack-list-variables. */ Phil> + if (print_locals && print_args && ! print_frame_info) Phil> + { Phil> + if (! py_mi_print_variables (filter, out, &opts, Phil> + args_type, frame)) Phil> + goto error; Phil> + else Phil> + return PY_BT_COMPLETED; Phil> + } This early return has to run the cleanups. Phil> + int success = py_print_frame (item, flags, args_type, out, indent, Phil> + levels_printed); Phil> + Phil> + if (success == PY_BT_ERROR && PyErr_Occurred ()) It seems like it should be impossible to see PY_BT_ERROR and not have PyErr_Occurred be true. If it can happen then it seems like that is an API contract violation. Phil> +int Phil> +apply_frame_filter (struct frame_info *frame, int flags, Seems like this should return enum py_bt_status. It needs to be updated in a few spots. Tom