From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6541 invoked by alias); 24 Nov 2011 08:27:44 -0000 Received: (qmail 6498 invoked by uid 22791); 24 Nov 2011 08:27:40 -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-vw0-f41.google.com (HELO mail-vw0-f41.google.com) (209.85.212.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 24 Nov 2011 08:27:17 +0000 Received: by vbbfn1 with SMTP id fn1so2482821vbb.0 for ; Thu, 24 Nov 2011 00:27:16 -0800 (PST) Received: by 10.52.21.148 with SMTP id v20mr26872110vde.89.1322123236099; Thu, 24 Nov 2011 00:27:16 -0800 (PST) MIME-Version: 1.0 Received: by 10.220.188.8 with HTTP; Thu, 24 Nov 2011 00:26:55 -0800 (PST) In-Reply-To: References: From: Kevin Pouget Date: Thu, 24 Nov 2011 08:27:00 -0000 Message-ID: Subject: Re: [RFC] Python Finish Breakpoints To: Tom Tromey Cc: gdb-patches@sourceware.org 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/msg00676.txt.bz2 On Thu, Nov 17, 2011 at 6:48 PM, Tom Tromey wrote: > > >>>>> "Kevin" =3D=3D Kevin Pouget writes: > > Kevin> py-breakpoint.c::gdbpy_should_stop is executed _before_ we have the > Kevin> chance to save the stop_registers, used to compute the return value > Kevin> (in infrun.c::normal_stop). > Kevin> (the problem existed since the beginning, but I never faced it bef= ore) > > Kevin> I've updated the function infcmd.c::get_return_value to take this > Kevin> situation into account, using the current registers if the > Kevin> 'stop_registers' are not set, based on what is done in > Kevin> infrun.c::normal_stop: > > This approach seems reasonable to me but I am not sure whether or not it > is really ok. =A0Most times I've tried to change infrun, I've been burned. I'm not sure what to do here, I don't change the value of the global variable "stop_registers" so if shouldn't affect more than my code, but it also depends of the side effects of > regcache_dup (get_current_regcache ()) which is now triggered 'slightly' before when it used to be ... > Kevin> 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->fun= ction_type) > > The problem is that Python errors are sticky. =A0So, if one occurs one > must either pass it upward (having the current function fail), or clear > it somehow. =A0It isn't ok to ignore them, call other Python functions, > and then later check. > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0/* Remember only non-VOID return type= s. =A0*/ > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0if (TYPE_CODE (ret_type) !=3D TYPE_CO= DE_VOID) > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0{ > 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_type = =3D > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0type_to_type_object (= SYMBOL_TYPE (function)); > > As discussed above, you need to check for errors immediately after each > call, and DTRT. =A0You can ignore them with PyErr_Clear. ok, I didn't know this point, so I've rewritten this to unconditionally clear the Py exception after these two calls, and then test against NULL as before. > Kevin> I think I wrote a word about that in the previous mail, anyway, my > Kevin> feeling was that I don't want to abort the FinishBreakpoint creati= on > Kevin> just because of a return value not computable, so I currently null= ify > Kevin> these fields and silently disable return value computation. I can't > Kevin> see any straightforward way to notify Python about that, > Kevin> warning/exceptions won't suite ... otherwise, I could expose the > Kevin> return_type to the Python interface, this way, one could check that > Kevin> it's not None and now if GDB will/might be able to compute the ret= urn > Kevin> value when the FinishBP is hit > > Ok, this makes sense to me. > > Tom> bpfinishpy_detect_out_scope_cb still acquires the GIL (via > Tom> ensure_python_env), but should not. > > Kevin> I'm not exactly sure what was you concern here, as far as I > Kevin> understand, the GIL was acquired in bpfinishpy_handle_stop, so it > Kevin> should have no effect in detect_out_scope_cb. So I've removed it f= rom > Kevin> detect_out_scope_cb as it was useless. > > I think it is inefficient to recursively acquire the GIL. right, the doc doesn't say anything about the efficiency of GIL acquisition, so let's assume you're right! > Kevin> +@defun FinishBreakpoint.__init__ (@r{[}frame@r{]} @r{[}, internal= @r{]}) > Kevin> +Create a finish breakpoint at the return address of the @code{gdb= .Frame} > Kevin> +object @var{frame} (or @code{gdb.selected_frame()} is not provide= d). > > I think it should read something like: > > =A0 =A0Create a finish breakpoint at the return address of the @code{gdb.= Frame} > =A0 =A0object @var{frame}. =A0If @var{frame} is not provided, this defaul= ts to > =A0 =A0the newest frame. ok for the sentence construction, > I think it is better to default to the newest frame, because the > selected frame is more of a user-interface thing, and I think code > wanting this should explicitly use it. My thought when I chose to use 'selected_frame' was to match the behavior of the CLI finish command. When you type it, you finish the 'selected' frame, and not the newest one. In my use-cases, I always have gdb.selected_frame =3D=3D gdb.newest_frame, so I don't have a strong preference. I've switch the code/doc to newest_frame. > Kevin> +type is @code{VOID} > > I think just @code{void} is better. sure > Kevin> + =A0/* If stop_registers where not saved, use the current registe= rs. =A0*/ > > s/where/were/ fixed > Kevin> + =A0if (cleanup) > Kevin> + =A0 =A0do_cleanups (cleanup); > > This is a gdb anti-pattern. =A0A call to make_cleanup can return NULL in > some scenarios. > > It is better to install a null cleanup and then unconditionally call > do_cleanups. ok, didn't know it either, fixed with the null cleanup > Kevin> + =A0/* Default to gdb.selected_frame if necessary. =A0*/ > Kevin> + =A0if (!frame_obj) > Kevin> + =A0 =A0frame_obj =3D gdbpy_selected_frame (NULL, NULL); > > gdbpy_newest_frame > > I don't think there is a decref for the frame_obj along this path. I've changed it to: =A0if (!frame_obj) =A0 =A0frame_obj =3D gdbpy_newest_frame (NULL, NULL); =A0else =A0 =A0Py_INCREF (frame_obj); =A0frame =3D frame_object_to_frame_info (frame_obj); =A0Py_DECREF (frame_obj); which looks clearer to me than setting a flag, or testing the kwargs for the "frame" keyword > Kevin> + =A0 =A0 =A0 =A0 =A0PyErr_SetString (PyExc_ValueError, > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0_("\"FinishBreakpoint\" not meaningful in= the outermost frame.")); > > Kevin> + =A0 =A0 =A0 =A0 =A0PyErr_SetString (PyExc_ValueError, > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 _("\"FinishBreakpoint\" cann= ot be set on a dummy frame.")); > > Indentation. > > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0PyErr_SetString (PyExc_ValueError, > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 _("Invalid ID for the `frame' object.")); > > Indentation. These 3 lines where right-justified to column 79, I've split them. > Kevin> +static void > Kevin> +bpfinishpy_out_of_scope (struct finish_breakpoint_object *bpfinis= h_obj) > Kevin> +{ > [...] > Kevin> + =A0TRY_CATCH (except, RETURN_MASK_ALL) > Kevin> + =A0 =A0{ > Kevin> + =A0 =A0 =A0delete_breakpoint (bpfinish_obj->py_bp.bp); > Kevin> + =A0 =A0} > Kevin> + =A0if (except.reason < 0) > Kevin> + =A0 =A0{ > Kevin> + =A0 =A0 =A0gdbpy_convert_exception (except); > Kevin> + =A0 =A0 =A0gdbpy_print_stack (); > Kevin> + =A0 =A0} > > I probably asked you to add this, but if bpfinishpy_out_of_scope can > only be called in one spot: > > Kevin> + =A0 =A0 =A0 =A0 =A0TRY_CATCH (except, RETURN_MASK_ALL) > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0{ > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0if (b->pspace =3D=3D current_inferior= ()->pspace > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 && (!target_has_registers > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 || frame_find_by_id (b->= frame_id) =3D=3D NULL)) > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0{ > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bpfinishpy_out_of_scope (finish_b= p); > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0} > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0} > Kevin> + =A0 =A0 =A0 =A0 =A0if (except.reason < 0) > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0{ > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0gdbpy_convert_exception (except); > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0 =A0gdbpy_print_stack (); > Kevin> + =A0 =A0 =A0 =A0 =A0 =A0} > > ... then the TRY_CATCH in bpfinishpy_out_of_scope is not needed. right > Kevin> + =A0struct cleanup *cleanup =3D ensure_python_env (target_gdbarch, > Kevin> + =A0 =A0 =A0current_language); > > Indentation. fixed There is no regression against the current tree (2011-11-18) Thanks, Kevin 2011-11-18 =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_return_= value. =A0 =A0 =A0 =A0(get_return_value): New function based on print_return_value= . 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-sta= tic. =A0 =A0 =A0 =A0(gdbpy_breakpoint_created): Set is_py_finish_bp is necessary. =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 call= ing stop =A0 =A0 =A0 =A0method. =A0 =A0 =A0 =A0* python/python-internal.h (breakpoint_object_type): Add as = 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-st= atic 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 noop = function. =A0 =A0 =A0 =A0(_initialize_python): Add gdbpy_initialize_finishbreakpoints. =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 (Finish Breakpoints in Python): New subsection. =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 v= ariable =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.