From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13262 invoked by alias); 16 Nov 2011 10:53:35 -0000 Received: (qmail 13246 invoked by uid 22791); 16 Nov 2011 10:53:33 -0000 X-SWARE-Spam-Status: No, hits=-1.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KAM_STOCKGEN,RCVD_IN_DNSWL_LOW,TW_BP,TW_EG X-Spam-Check-By: sourceware.org Received: from mail-vx0-f169.google.com (HELO mail-vx0-f169.google.com) (209.85.220.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 16 Nov 2011 10:53:18 +0000 Received: by vcbfo11 with SMTP id fo11so378590vcb.0 for ; Wed, 16 Nov 2011 02:53:17 -0800 (PST) Received: by 10.220.107.136 with SMTP id b8mr2087446vcp.40.1321440796152; Wed, 16 Nov 2011 02:53:16 -0800 (PST) MIME-Version: 1.0 Received: by 10.220.187.140 with HTTP; Wed, 16 Nov 2011 02:52:55 -0800 (PST) In-Reply-To: References: From: Kevin Pouget Date: Wed, 16 Nov 2011 10:53:00 -0000 Message-ID: Subject: Re: [RFC] Python Finish Breakpoints To: Tom Tromey Cc: gdb-patches@sourceware.org, pmuldoon@redhat.com Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes 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/msg00422.txt.bz2 On Wed, Nov 9, 2011 at 3:09 PM, Kevin Pouget wrote: > > On Fri, Nov 4, 2011 at 10:03 PM, Tom Tromey wrote: > > > > >>>>> "Kevin" =3D=3D Kevin Pouget writes: > > > > Tom> It seems like it should be an error to try to compute the return v= alue > > Tom> when not stopped at this breakpoint. > > > > Kevin> I'm not totally convinced ... > > Kevin> what would you think about throwing an AttributeError("return_va= lue > > Kevin> not available yet") when accessing the attribute before the brea= kpoint > > 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> +static PyObject * > > Kevin> +bpfinishpy_get_returnvalue (PyObject *self, void *closure) > > Kevin> +{ > > [...] > > Kevin> + =A0 =A0 =A0for (bs =3D inferior_thread ()->control.stop_bpstat; > > Kevin> + =A0 =A0 =A0 =A0 =A0bs; bs =3D bs->next) > > Kevin> + =A0 =A0 =A0 =A0{ > > Kevin> + =A0 =A0 =A0 =A0 =A0struct breakpoint *bp =3D bs->breakpoint_at; > > Kevin> + > > Kevin> + =A0 =A0 =A0 =A0 =A0if (bp !=3D NULL && (PyObject *) bp->py_bp_= object =3D=3D self) > > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0{ > > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0bpfinish_stopped_at_finish_bp (self= _finishbp); > > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0if (PyErr_Occurred ()) > > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 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. > > This part is a bit tricky, and your suggestion has highlighted a > problem I didn't consider/test: > > py-breakpoint.c::gdbpy_should_stop is executed _before_ we have the > chance to save the stop_registers, used to compute the return value > (in infrun.c::normal_stop). > (the problem existed since the beginning, but I never faced it before) > > I've updated the function infcmd.c::get_return_value to take this > situation into account, using the current registers if the > 'stop_registers' are not set, based on what is done in > infrun.c::normal_stop: > > > struct value * > > get_return_value (struct type *func_type, struct type *value_type) > > ... > > =A0 /* If stop_registers where not saved, use the current registers. = =A0*/ > > =A0 if (!stop_regs) > > =A0 =A0 { > > =A0 =A0 =A0 stop_regs =3D regcache_dup (get_current_regcache ()); > > =A0 =A0 =A0 cleanup =3D make_cleanup_regcache_xfree (stop_regs); > > =A0 =A0 } > > but I can't say that I'm confident with regcache handling, and I don't > know if these lines would have unexpected side effects ... > > > The patch enclosed passes the testsuite with no regression on x86_64/fedo= ra 15 > > > Kevin> + =A0TRY_CATCH (except, RETURN_MASK_ALL) > > Kevin> + =A0 =A0{ > > Kevin> + =A0 =A0 =A0struct value *ret =3D > > Kevin> + =A0 =A0 =A0 =A0 =A0get_return_value (type_object_to_type (py_b= p->function_type), > > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0type_ob= ject_to_type (py_bp->return_type)); > > Kevin> + > > Kevin> + =A0 =A0 =A0if (ret) > > Kevin> + =A0 =A0 =A0 =A0py_bp->return_value =3D value_to_value_object (= ret); > > Kevin> + =A0 =A0 =A0else > > Kevin> + =A0 =A0 =A0 =A0py_bp->return_value =3D Py_None; > > Kevin> + =A0 =A0} > > Kevin> + =A0if (except.reason < 0) > > Kevin> + =A0 =A0gdbpy_convert_exception(except); > > > > Missing a space. > > > > I think you need to Py_INCREF Py_None here. > > > > > > Kevin> + =A0if (except.reason < 0) > > Kevin> + =A0 =A0{ > > Kevin> + =A0 =A0 =A0gdbpy_convert_exception(except); > > > > Missing space. > > > > Kevin> + =A0if (except.reason < 0 > > Kevin> + =A0 =A0 =A0|| !self_bpfinish->return_type || !self_bpfinish->f= unction_type) > > Kevin> + =A0 =A0{ > > Kevin> + =A0 =A0 =A0/* Won't be able to compute return value. =A0*/ > > Kevin> + =A0 =A0 =A0self_bpfinish->return_type =3D NULL; > > Kevin> + =A0 =A0 =A0self_bpfinish->function_type =3D NULL; > > > > Need decrefs here. > > all fixed, thanks > > > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0self_bpfinish->return_type = =3D type_to_type_object (ret_type); > > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0self_bpfinish->function_typ= e =3D > > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0type_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. > > I think that I handle that in the following lines: > > > Kevin> + =A0if (except.reason < 0 > > Kevin> + =A0 =A0 =A0|| !self_bpfinish->return_type || !self_bpfinish->f= unction_type) > > I think I wrote a word about that in the previous mail, anyway, my > feeling was that I don't want to abort the FinishBreakpoint creation > just because of a return value not computable, so I currently nullify > these fields and silently disable return value computation. I can't > see any straightforward way to notify Python about that, > warning/exceptions won't suite ... otherwise, I could expose the > return_type to the Python interface, this way, one could check that > it's not None and now if GDB will/might be able to compute the return > value when the FinishBP is hit > > > Kevin> + =A0BPPY_SET_REQUIRE_VALID (&self_bpfinish->py_bp); > > > > Hm, why is this here? > > no reason apparently, the try/catch above shall ensure that the BP is val= id > > > Kevin> +static void > > Kevin> +bpfinishpy_out_of_scope (struct finish_breakpoint_object *bpfin= ish_obj) > > Kevin> +{ > > [...] > > Kevin> + =A0delete_breakpoint (bpfinish_obj->py_bp.bp); > > > > I think it needs a TRY_CATCH. > > > > Kevin> + =A0 =A0 =A0else if (b->pspace =3D=3D current_inferior ()->pspa= ce > > Kevin> + =A0 =A0 =A0 =A0 =A0 && (!target_has_registers > > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 || frame_find_by_id (b->frame_id) = =3D=3D NULL)) > > Kevin> + =A0 =A0 =A0 =A0{ > > Kevin> + =A0 =A0 =A0 =A0 =A0bpfinishpy_out_of_scope (finish_bp); > > Kevin> + =A0 =A0 =A0 =A0} > > Kevin> + =A0 =A0} > > > > This too, I think. > > right, done > > > Kevin> +static void > > Kevin> +bpfinishpy_handle_stop (struct bpstats *bs, int print_frame) > > Kevin> +{ > > Kevin> + =A0struct cleanup *cleanup =3D ensure_python_env (get_current_= arch (), > > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 current_language); > > Kevin> + > > Kevin> + =A0iterate_over_breakpoints (bpfinishpy_detect_out_scope_cb, > > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bs =3D= =3D NULL ? NULL : bs->breakpoint_at); > > > > bpfinishpy_detect_out_scope_cb still acquires the GIL (via ensure_pytho= n_env), > > but should not. > > I'm not exactly sure what was you concern here, as far as I > understand, the GIL was acquired in bpfinishpy_handle_stop, so it > should have no effect in detect_out_scope_cb. So I've removed it from > detect_out_scope_cb as it was useless. > > I've also made a litlle modification in the FinishBreakpoint __init__, > which now default it's frame argument to gdb.current_frame(), instead > of making it mandatory. I've updated the documentation and testsuite > accordingly. > > > Thanks for the time you spend reviewing my patches, > > Kevin > > -- > > 2011-11-09 =A0Kevin Pouget =A0 > > =A0 =A0 =A0 =A0Introduce gdb.FinishBreakpoint in Python > > =A0 =A0 =A0 =A0* Makefile.in (SUBDIR_PYTHON_OBS): Add py-finishbreakpoint= .o. > =A0 =A0 =A0 =A0(SUBDIR_PYTHON_SRCS): Add python/py-finishbreakpoint.c. > =A0 =A0 =A0 =A0Add build rule for this file. > =A0 =A0 =A0 =A0* infcmd.c (print_return_value): Split to create get_retur= n_value. > =A0 =A0 =A0 =A0(get_return_value): New function based on print_return_val= ue. Handle > =A0 =A0 =A0 =A0case where stop_registers are not set. > =A0 =A0 =A0 =A0* inferior.h (get_return_value): New prototype. > =A0 =A0 =A0 =A0* python/py-breakpoint.c (bppy_pending_object): Make non-s= tatic. > =A0 =A0 =A0 =A0(gdbpy_breakpoint_created): Set is_py_finish_bp is necessa= ry. > =A0 =A0 =A0 =A0(struct breakpoint_object): Move to python-internal.h > =A0 =A0 =A0 =A0(BPPY_REQUIRE_VALID): Likewise. > =A0 =A0 =A0 =A0(BPPY_SET_REQUIRE_VALID): Likewise. > =A0 =A0 =A0 =A0(gdbpy_breakpoint_created): Initialize is_finish_bp. > =A0 =A0 =A0 =A0(gdbpy_should_stop): Add =A0pre/post hooks before/after ca= lling stop > =A0 =A0 =A0 =A0method. > =A0 =A0 =A0 =A0* python/python-internal.h (breakpoint_object_type): Add a= s extern. > =A0 =A0 =A0 =A0(bppy_pending_object): Likewise. > =A0 =A0 =A0 =A0(typedef struct breakpoint_object) Removed. > =A0 =A0 =A0 =A0(struct breakpoint_object): Moved from py-breakpoint.c. > =A0 =A0 =A0 =A0Add field is_finish_bp. > =A0 =A0 =A0 =A0(BPPY_REQUIRE_VALID): Moved from py-breakpoint.c. > =A0 =A0 =A0 =A0(BPPY_SET_REQUIRE_VALID): Likewise. > =A0 =A0 =A0 =A0(frame_object_to_frame_info): New prototype. > =A0 =A0 =A0 =A0(gdbpy_initialize_finishbreakpoints): New prototype. > =A0 =A0 =A0 =A0(bpfinishpy_is_finish_bp): Likewise. > =A0 =A0 =A0 =A0(bpfinishpy_pre_stop_hook): Likewise. > =A0 =A0 =A0 =A0(bpfinishpy_post_stop_hook): Likewise. > =A0 =A0 =A0 =A0* python/py-finishbreakpoint.c: New file. > =A0 =A0 =A0 =A0* python/py-frame.c(frame_object_to_frame_info): Make non-= static and > =A0 =A0 =A0 =A0accept PyObject instead of frame_object. > =A0 =A0 =A0 =A0(frapy_is_valid): Don't cast to frame_object. > =A0 =A0 =A0 =A0(frapy_name): Likewise. > =A0 =A0 =A0 =A0(frapy_type): Likewise. > =A0 =A0 =A0 =A0(frapy_unwind_stop_reason): Likewise. > =A0 =A0 =A0 =A0(frapy_pc): Likewise. > =A0 =A0 =A0 =A0(frapy_block): Likewise. > =A0 =A0 =A0 =A0(frapy_function): Likewise. > =A0 =A0 =A0 =A0(frapy_older): Likewise. > =A0 =A0 =A0 =A0(frapy_newer): Likewise. > =A0 =A0 =A0 =A0(frapy_find_sal): Likewise. > =A0 =A0 =A0 =A0(frapy_read_var): Likewise. > =A0 =A0 =A0 =A0(frapy_select): Likewise. > =A0 =A0 =A0 =A0* python/python.c (gdbpy_is_stopped_at_finish_bp): New noo= p function. > =A0 =A0 =A0 =A0(_initialize_python): Add gdbpy_initialize_finishbreakpoin= ts. > =A0 =A0 =A0 =A0* python/python.h: Include breakpoint.h > =A0 =A0 =A0 =A0(gdbpy_is_stopped_at_finish_bp): New prototype. > > doc/ > =A0 =A0 =A0 =A0* gdb.texinfo (Breakpoints In Python): New subsection: Fin= ish > =A0 =A0 =A0 =A0Breakpoints. > =A0 =A0 =A0 =A0(Python API): Add menu entry for Finish Breakpoints > > testsuite/ > =A0 =A0 =A0 =A0* gdb.python/py-breakpoint.exp (mult_line): Define and use= variable > =A0 =A0 =A0 =A0instead of line number. > =A0 =A0 =A0 =A0* gdb.python/py-finish-breakpoint.c: New file. > =A0 =A0 =A0 =A0* gdb.python/py-finish-breakpoint.exp: New file. > =A0 =A0 =A0 =A0* gdb.python/py-finish-breakpoint.py: New file. > =A0 =A0 =A0 =A0* gdb.python/py-finish-breakpoint2.cc: New file. > =A0 =A0 =A0 =A0* gdb.python/py-finish-breakpoint2.exp: New file. > =A0 =A0 =A0 =A0* gdb.python/py-finish-breakpoint2.py: New file. ping after one week, if you have a bit of time Thanks, Kevin