From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1085 invoked by alias); 19 May 2011 16:21:22 -0000 Received: (qmail 1072 invoked by uid 22791); 19 May 2011 16:21:20 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_BJ,TW_BP,TW_GJ,T_RP_MATCHES_RCVD 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; Thu, 19 May 2011 16:21:01 +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 p4JGL0VI005973 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 19 May 2011 12:21:00 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p4JGL0PB024133; Thu, 19 May 2011 12:21:00 -0400 Received: from opsy.redhat.com (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 p4JGKxRs027762; Thu, 19 May 2011 12:20:59 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id 27AA537918A; Thu, 19 May 2011 10:20:58 -0600 (MDT) From: Tom Tromey To: Kevin Pouget Cc: gdb-patches@sourceware.org Subject: Re: [RFC] Python Finish Breakpoints References: Date: Thu, 19 May 2011 16:21:00 -0000 In-Reply-To: (Kevin Pouget's message of "Wed, 18 May 2011 04:57:26 -0400") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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-05/txt/msg00444.txt.bz2 >>>>> "Kevin" == Kevin Pouget writes: Moving to gdb-patches. Kevin> I've included with this mail a complete patch build agains the Kevin> current HEAD, and checked that there was no regression in the Kevin> testsuite Thanks. I'm sorry about the delay in my reply. I'm going to try to prioritize patch review and email a bit higher in the future. I think this functionality is very good. I am not sure about some aspects of the implementation, though, and I have some style nits to pick. Kevin> -static void Kevin> +void Kevin> create_breakpoint_sal (struct gdbarch *gdbarch, Kevin> struct symtabs_and_lines sals, char *addr_string, Kevin> char *cond_string, What is the rationale for using this function in particular? It seems to me that it would probably be better to make an explicit_pc=1 SAL using the unwound PC. Then there would be no reason to avoid the currently exported API. Kevin> + struct value *value = get_return_value(func_type, value_type); Missing space before the "(". This appears multiple places in the patch. Kevin> - if (inferior_thread ()->control.proceed_to_finish) Kevin> + if (gdbpy_is_stopped_at_finish_bp (inferior_thread ()->control.stop_bpstat) Kevin> + || inferior_thread ()->control.proceed_to_finish) gdbpy_is_stopped_at_finish_bp is not safe to call here -- it assumes the GIL has been acquired, which it has not. I would rather it not be changed to acquire the GIL, however. I think one of two other approaches would be preferable. One way you could handle this is to add a new constant to enum bptype. This is likely to be pretty invasive. Another way would be to add a flag to the struct breakpoint itself. Yet another way would be a new breakpoint_ops method. This is related, in a way, to the out-of-scope handling. Right now the patch tries to reimplement the existing breakpoint re-setting logic in py-finishbreakpoint.c, via observers. I think it would be better to have this be done automatically by the existing code in breakpoint.c, perhaps adding some additional python-visible notification step. Kevin> +static char * const outofscope_func = "out_of_scope"; "const char *". Kevin> + /* The function finished by this breakpoint. */ Kevin> + struct symbol *function; If you want to store a pointer to a symbol, then you have to account for the case where the objfile is destroyed. Otherwise you can end up with a dangling pointer. Alternatively, perhaps you could have this refer to a Symbol object; but then you have to be careful to check it for validity before using it. Actually, it seems that the symbol is only used to get the function's return type. You might as well just compute that up front and store a Type. Kevin> +/* Python function to set the 'out_of_scope_notif' attribute of Kevin> + FinishBreakpoint. */ Kevin> + Kevin> +static int Kevin> +bpfinishpy_set_outofscope_notif (PyObject *self, PyObject *newvalue, Kevin> + void *closure) I don't understand the point of this function. I think documentation would help. Kevin> +/* Python function to get the 'return_value' attribute of Kevin> + FinishBreakpoint. */ Kevin> + Kevin> +static PyObject * Kevin> +bpfinishpy_get_returnvalue (PyObject *self, void *closure) Kevin> +{ Kevin> + struct finish_breakpoint_object *self_finishbp = Kevin> + (struct finish_breakpoint_object *) self; Kevin> + Kevin> + BPPY_REQUIRE_VALID (&self_finishbp->py_bp); Kevin> + Kevin> + if (self_finishbp->function == NULL) Kevin> + goto return_none; Kevin> + Kevin> + /* Ensure that GDB is stopped at this FinishBreakpoint. */ Kevin> + if (inferior_thread ()->control.stop_bpstat != NULL) Kevin> + { Kevin> + bpstat bs; Kevin> + Kevin> + for(bs = inferior_thread ()->control.stop_bpstat; Kevin> + bs; bs = bs->next) I am not an expert here, but I think it is probably no good to rely on this state remaining live. I think it would be better to simply have a stop at one of these breakpoints compute and cache the Value object immediately. Kevin> +static void Kevin> +gdbpy_out_of_scope (struct finish_breakpoint_object *bpfinish_obj) Kevin> +{ Kevin> + breakpoint_object *bp_obj = (breakpoint_object *) bpfinish_obj; Kevin> + PyObject *py_obj = (PyObject *) bp_obj ; Kevin> + Kevin> + bpfinish_obj->out_of_scope_notif = 0; Kevin> + Kevin> + if (PyObject_HasAttrString (py_obj, outofscope_func)) Kevin> + { Kevin> + struct gdbarch *garch = bp_obj->bp->gdbarch ? Kevin> + bp_obj->bp->gdbarch : get_current_arch (); Kevin> + struct cleanup *cleanup = ensure_python_env (garch, current_language); You can't call any Python functions without the GIL. This applies to PyObject_HasAttrString here. In this case, though, gdbpy_out_of_scope is called by one function that already has the GIL. So, I think the acquisition should be pushed into its other caller. Kevin> + pc = get_frame_pc (prev_frame); This can throw an exception. So, it needs to be wrapped in a TRY_CATCH. This may apply to some other GDB functions called by the "Python-facing" code, I did not check them all. Kevin> +static PyMethodDef finish_breakpoint_object_methods[] = { Kevin> + { "check_scope", bpfinishpy_check_scope, METH_NOARGS, Kevin> + "check_scope () -> Boolean.\n\ Kevin> +Return true if out_of_scope() has been triggered, false if not." }, How is this useful? And, why a method instead of an attribute? Kevin> +static PyGetSetDef finish_breakpoint_object_getset[] = { Kevin> + { "out_of_scope_notif", bpfinishpy_get_outofscope_notif, bpfinishpy_set_outofscope_notif, I don't like the name "out_of_scope_notif", particularly if this is an apt description of its meaning: Kevin> + "Boolean telling whether the breakpoint is still within the scope \ Kevin> +of the current callstack.", NULL }, Kevin> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-cc.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint-cc.exp Funny file name. Kevin> +gdb_test "python ExceptionBreakpoint()" "ExceptionBreakpoint init" "set BP before throwing the exception" Kevin> +gdb_test "python print len(gdb.breakpoints())" "4" "check number of BPs" Kevin> +gdb_test "continue" ".*stopped at ExceptionFinishBreakpoint.*" "check FinishBreakpoint in catch()" Kevin> +gdb_test "python print len(gdb.breakpoints())" "4" "check finish BP removal" I don't think this tests the most important case -- where an exception is thrown, invalidating the return breakpoint. That is, put a breakpoint in throw_exception_1, continue to there, then set the breakpoint, then continue again. I didn't examine the longjmp test but the same idea applies there. Other cases to consider are - * inferior exec * inferior exit * explicit inferior function call * "return" command Tom