From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4243 invoked by alias); 28 Oct 2011 20:37:42 -0000 Received: (qmail 4224 invoked by uid 22791); 28 Oct 2011 20:37:41 -0000 X-SWARE-Spam-Status: No, hits=-5.0 required=5.0 tests=AWL,BAYES_50,KAM_STOCKGEN,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_BP,TW_WB 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, 28 Oct 2011 20:37:18 +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 p9SKbHgJ019595 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 28 Oct 2011 16:37:18 -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 p9SKbHLG009973; Fri, 28 Oct 2011 16:37:17 -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 p9SKbG6S003368; Fri, 28 Oct 2011 16:37:16 -0400 From: Tom Tromey To: Kevin Pouget Cc: gdb-patches@sourceware.org, pmuldoon@redhat.com Subject: Re: [RFC] Python Finish Breakpoints References: Date: Fri, 28 Oct 2011 20:51:00 -0000 In-Reply-To: (Kevin Pouget's message of "Fri, 28 Oct 2011 09:54:13 +0200") 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-10/txt/msg00782.txt.bz2 >>>>> "Kevin" == Kevin Pouget writes: You need a ChangeLog. Kevin> -static PyTypeObject breakpoint_object_type; Kevin> +PyTypeObject breakpoint_object_type; The old line here was a forward declaration. I think you should just entirely remove it. Kevin> + /* If it's a temporary breakpoint. */ Kevin> + if (bpfinishpy_is_finish_bp (py_bp)) Kevin> + { Kevin> + /* Can't delete it here. */ Kevin> + gdb_assert (b->disposition == disp_del); Kevin> + disable_breakpoint (b); Kevin> + } I don't understand this code. It seems to me that disp_del is a better setting here. Kevin> + newbp->is_finish_bp = bpfinishpy_is_finish_bp ((PyObject *) newbp); I think this flag could be set more cleanly in bpfinishpy_init. Kevin> + /* gdb.Type object of the function finished by this breakpoint. */ Kevin> + PyObject *function_type; Kevin> + /* gdb.Type object of the value return by the breakpointed function. */ Kevin> + PyObject *return_type; Kevin> + /* When stopped at this FinishBreakpoint, value returned by the function; Kevin> + Py_None if the value is not computable; NULL if GDB is not stopped at Kevin> + a FinishBreakpoint. */ Kevin> + PyObject *return_value; I think the new class needs a destructor which decrefs these. Kevin> +static void Kevin> +bpfinish_stopped_at_finish_bp (struct finish_breakpoint_object *py_bp) Kevin> +{ Kevin> + if (py_bp->return_type) 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)); Calls into gdb have to be wrapped in TRY_CATCH. Then errors have to be propagated to Python somehow, or printed and ignored via gdbpy_print_stack. Given the context I would say propagate. Kevin> + /* Check if we have a cached value. */ Kevin> + if (!self_finishbp->return_value) Kevin> + { Kevin> + bpstat bs; Kevin> + Kevin> + BPPY_REQUIRE_VALID (&self_finishbp->py_bp); 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> + bpfinish_stopped_at_finish_bp (self_finishbp); Kevin> + } It seems like it should be an error to try to compute the return value when not stopped at this breakpoint. Kevin> +struct breakpoint * Kevin> +gdbpy_is_stopped_at_finish_bp (bpstat stop_bpstat) Kevin> +{ Since the name implies that this is a predicate and since the result is only ever used as a boolean, I think this should return int. Kevin> + bpstat bs; Kevin> + Kevin> + for (bs = stop_bpstat; bs; bs = bs->next) Kevin> + { Kevin> + if (bs->breakpoint_at Kevin> + && bpfinishpy_bp_is_finish_bp ((breakpoint_object *) Kevin> + bs->breakpoint_at->py_bp_object)) I am not really sure about this. It seems like it may be pedantically incorrect, though it is hard to see when it could possibly fail. That is, is the GIL required or not? It doesn't call a function and the breakpoint owns a ref to the breakpoint object, so it seems like it could not be deleted out from under us. I'm inclined to say it is ok. Kevin> + breakpoint_object *self_bp = (breakpoint_object *) self; Kevin> + struct finish_breakpoint_object *self_bpfinish = Kevin> + (struct finish_breakpoint_object *) self; I think this is the only use of self in this function. Just drop it and cast directly to the most specific subclass. Kevin> + prev_frame = get_prev_frame (frame); Kevin> + if (prev_frame == 0) Kevin> + { Kevin> + PyErr_SetString (PyExc_ValueError, Kevin> + _("\"FinishBreakpoint\" not meaningful in the outermost frame.")); Kevin> + return -1; Kevin> + } Kevin> + else if (get_frame_type (prev_frame) == DUMMY_FRAME) Kevin> + { Kevin> + PyErr_SetString (PyExc_ValueError, Kevin> + _("\"FinishBreakpoint\" cannot be set on a dummy frame.")); Kevin> + return -1; Kevin> + } I think the calls to get_prev_frame and get_frame_type need to be wrapped in a TRY_CATCH. Kevin> + frame_id = get_frame_id (prev_frame); Kevin> + if (frame_id_eq (frame_id, null_frame_id)) Likewise. I'd try to put all the gdb-facing logic into a single big TRY_CATCH. Kevin> + if (internal) Kevin> + { Kevin> + internal_bp = PyObject_IsTrue (internal); Kevin> + if (internal_bp == -1) Kevin> + { Kevin> + PyErr_SetString (PyExc_ValueError, Kevin> + _("The value of `internal' must be a boolean.")); Kevin> + return -1; Do you need to decref 'frame_obj' here? I suspect so. There are other early returns that probably need this. A typical solution is a label where all the locals are xdecref'd then return -1. Kevin> + /* Find the function we will return from. */ Kevin> + self_bpfinish->return_type = NULL; Kevin> + self_bpfinish->function_type = NULL; These can be left NULL in the object. What happens if you try to fetch the return value in that case? Kevin> + if (get_frame_pc_if_available (frame, &pc)) Kevin> + { Kevin> + function = find_pc_function (pc); Kevin> + if (function != NULL) Kevin> + { Kevin> + struct type *ret_type = TYPE_TARGET_TYPE (SYMBOL_TYPE (function)); Kevin> + More TRY_CATCH. Kevin> + /* Remember only non-VOID return types. */ Kevin> + if (TYPE_CODE (ret_type) != TYPE_CODE_VOID) Kevin> + { Kevin> + self_bpfinish->return_type = type_to_type_object (ret_type); Error check. Kevin> + self_bpfinish->function_type = Kevin> + type_to_type_object (SYMBOL_TYPE (function)); Likewise. Kevin> + if (except.reason < 0) Kevin> + { Kevin> + PyErr_Format (except.reason == RETURN_QUIT Kevin> + ? PyExc_KeyboardInterrupt : PyExc_RuntimeError, Kevin> + "%s", except.message); Kevin> + return -1; Just use GDB_PY_SET_HANDLE_EXCEPTION. Kevin> +int Kevin> +bpfinishpy_is_finish_bp (PyObject *obj) Kevin> +{ Kevin> + return obj != NULL Kevin> + && PyObject_TypeCheck (obj, &finish_breakpoint_object_type); Kevin> +} Kevin> +int Kevin> +bpfinishpy_bp_is_finish_bp (breakpoint_object *bp_obj) Kevin> +{ Kevin> + return bp_obj != NULL Kevin> + && bp_obj->is_finish_bp; Kevin> +} Are both of these needed? Kevin> +static void Kevin> +bpfinishpy_handle_stop (struct bpstats *bs, int print_frame) Kevin> +{ Kevin> + iterate_over_breakpoints (bpfinishpy_detect_out_scope_cb, Kevin> + bs == NULL ? NULL : bs->breakpoint_at); Kevin> +} The way this is written, it will acquire and release the GIL for each breakpoint. I think it would be better to acquire + release just once. Kevin> +static void Kevin> +bpfinishpy_handle_exit (struct inferior *inf) Kevin> +{ Kevin> + iterate_over_breakpoints (bpfinishpy_detect_out_scope_cb, NULL); Kevin> +} Likewise. Kevin> +struct breakpoint *gdbpy_is_stopped_at_finish_bp (bpstat stop_bpstat); Kevin> #endif /* GDB_PYTHON_H */ Newline between these two lines. Tom