From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5039 invoked by alias); 4 Nov 2011 21:04:16 -0000 Received: (qmail 5031 invoked by uid 22791); 4 Nov 2011 21:04:15 -0000 X-SWARE-Spam-Status: No, hits=-5.4 required=5.0 tests=AWL,BAYES_50,KAM_STOCKGEN,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SINGLE_HEADER_1K,SPF_HELO_PASS,TW_BP 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, 04 Nov 2011 21:03:54 +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 pA4L3rtq030089 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 4 Nov 2011 17:03:53 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id pA4L3qYi005486; Fri, 4 Nov 2011 17:03:53 -0400 Received: from barimba (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 pA4L3pDU000396; Fri, 4 Nov 2011 17:03:51 -0400 From: Tom Tromey To: Kevin Pouget Cc: gdb-patches@sourceware.org, pmuldoon@redhat.com Subject: Re: [RFC] Python Finish Breakpoints References: Date: Fri, 04 Nov 2011 21:04:00 -0000 In-Reply-To: (Kevin Pouget's message of "Fri, 4 Nov 2011 15:24:27 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.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: 2011-11/txt/msg00123.txt.bz2 >>>>> "Kevin" == Kevin Pouget writes: Tom> It seems like it should be an error to try to compute the return value Tom> when not stopped at this breakpoint. Kevin> I'm not totally convinced ... Kevin> what would you think about throwing an AttributeError("return_value Kevin> not available yet") when accessing the attribute before the breakpoint Kevin> is hit, but keep the cached value afterwards? What I meant was that accessing the cached value any time is fine -- just that attempting to compute the value for the first time at any point other than the breakpoint location was wrong, just because whatever we compute then will be unrelated to what the user might want. It is hard to be sure that the current code handles this properly. See below. Kevin> + TRY_CATCH (except, RETURN_MASK_ALL) Kevin> + { Kevin> + struct value *ret = Kevin> + get_return_value (type_object_to_type (py_bp->function_type), Kevin> + type_object_to_type (py_bp->return_type)); Kevin> + Kevin> + if (ret) Kevin> + py_bp->return_value = value_to_value_object (ret); Kevin> + else Kevin> + py_bp->return_value = Py_None; Kevin> + } Kevin> + if (except.reason < 0) Kevin> + gdbpy_convert_exception(except); Missing a space. I think you need to Py_INCREF Py_None here. Kevin> +static PyObject * Kevin> +bpfinishpy_get_returnvalue (PyObject *self, void *closure) Kevin> +{ [...] Kevin> + for (bs = inferior_thread ()->control.stop_bpstat; Kevin> + bs; bs = bs->next) Kevin> + { Kevin> + struct breakpoint *bp = bs->breakpoint_at; Kevin> + Kevin> + if (bp != NULL && (PyObject *) bp->py_bp_object == self) Kevin> + { Kevin> + bpfinish_stopped_at_finish_bp (self_finishbp); Kevin> + if (PyErr_Occurred ()) Kevin> + return NULL; This seems to try to do the right thing -- but is inferior_thread()->control even valid at all points that can reach this? What about just computing the value before calling the 'stop' method? As long as it computes a lazy value this won't be very expensive. Kevin> + if (except.reason < 0) Kevin> + { Kevin> + gdbpy_convert_exception(except); Missing space. Kevin> + self_bpfinish->return_type = type_to_type_object (ret_type); Kevin> + self_bpfinish->function_type = Kevin> + type_to_type_object (SYMBOL_TYPE (function)); These can fail with NULL and must be handled, probably by returning. But you can't return out of a TRY_CATCH. Kevin> + if (except.reason < 0 Kevin> + || !self_bpfinish->return_type || !self_bpfinish->function_type) Kevin> + { Kevin> + /* Won't be able to compute return value. */ Kevin> + self_bpfinish->return_type = NULL; Kevin> + self_bpfinish->function_type = NULL; Need decrefs here. Kevin> + BPPY_SET_REQUIRE_VALID (&self_bpfinish->py_bp); Hm, why is this here? Kevin> +static void Kevin> +bpfinishpy_out_of_scope (struct finish_breakpoint_object *bpfinish_obj) Kevin> +{ [...] Kevin> + delete_breakpoint (bpfinish_obj->py_bp.bp); I think it needs a TRY_CATCH. Kevin> + else if (b->pspace == current_inferior ()->pspace Kevin> + && (!target_has_registers Kevin> + || frame_find_by_id (b->frame_id) == NULL)) Kevin> + { Kevin> + bpfinishpy_out_of_scope (finish_bp); Kevin> + } Kevin> + } This too, I think. Kevin> +static void Kevin> +bpfinishpy_handle_stop (struct bpstats *bs, int print_frame) Kevin> +{ Kevin> + struct cleanup *cleanup = ensure_python_env (get_current_arch (), Kevin> + current_language); Kevin> + Kevin> + iterate_over_breakpoints (bpfinishpy_detect_out_scope_cb, Kevin> + bs == NULL ? NULL : bs->breakpoint_at); bpfinishpy_detect_out_scope_cb still acquires the GIL (via ensure_python_env), but should not. Tom