[RFC] Python Finish Breakpoints
Kevin Pouget
kevin.pouget@gmail.com
Wed Nov 30 16:02:00 GMT 2011
On Thu, Nov 24, 2011 at 9:26 AM, Kevin Pouget <kevin.pouget@gmail.com> wrote:
>
> On Thu, Nov 17, 2011 at 6:48 PM, Tom Tromey <tromey@redhat.com> wrote:
> >
> > >>>>> "Kevin" == Kevin Pouget <kevin.pouget@gmail.com> 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 before)
> >
> > 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. Most 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> + if (except.reason < 0
> > Kevin> + || !self_bpfinish->return_type || !self_bpfinish->function_type)
> >
> > The problem is that Python errors are sticky. So, if one occurs one
> > must either pass it upward (having the current function fail), or clear
> > it somehow. It isn't ok to ignore them, call other Python functions,
> > and then later check.
>
> > 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);
> > Kevin> + self_bpfinish->function_type =
> > Kevin> + type_to_type_object (SYMBOL_TYPE (function));
> >
> > As discussed above, you need to check for errors immediately after each
> > call, and DTRT. You 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 creation
> > Kevin> just because of a return value not computable, so I currently nullify
> > 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 return
> > 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 from
> > 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 provided).
> >
> > I think it should read something like:
> >
> > Create a finish breakpoint at the return address of the @code{gdb.Frame}
> > object @var{frame}. If @var{frame} is not provided, this defaults to
> > the 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 == 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> + /* If stop_registers where not saved, use the current registers. */
> >
> > s/where/were/
>
> fixed
>
> > Kevin> + if (cleanup)
> > Kevin> + do_cleanups (cleanup);
> >
> > This is a gdb anti-pattern. A 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> + /* Default to gdb.selected_frame if necessary. */
> > Kevin> + if (!frame_obj)
> > Kevin> + frame_obj = 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:
>
> if (!frame_obj)
> frame_obj = gdbpy_newest_frame (NULL, NULL);
> else
> Py_INCREF (frame_obj);
>
> frame = frame_object_to_frame_info (frame_obj);
> Py_DECREF (frame_obj);
>
> which looks clearer to me than setting a flag, or testing the kwargs
> for the "frame" keyword
>
> > Kevin> + PyErr_SetString (PyExc_ValueError,
> > Kevin> + _("\"FinishBreakpoint\" not meaningful in the outermost frame."));
> >
> > Kevin> + PyErr_SetString (PyExc_ValueError,
> > Kevin> + _("\"FinishBreakpoint\" cannot be set on a dummy frame."));
> >
> > Indentation.
> >
> > Kevin> + PyErr_SetString (PyExc_ValueError,
> > Kevin> + _("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 *bpfinish_obj)
> > Kevin> +{
> > [...]
> > Kevin> + TRY_CATCH (except, RETURN_MASK_ALL)
> > Kevin> + {
> > Kevin> + delete_breakpoint (bpfinish_obj->py_bp.bp);
> > Kevin> + }
> > Kevin> + if (except.reason < 0)
> > Kevin> + {
> > Kevin> + gdbpy_convert_exception (except);
> > Kevin> + gdbpy_print_stack ();
> > Kevin> + }
> >
> > I probably asked you to add this, but if bpfinishpy_out_of_scope can
> > only be called in one spot:
> >
> > Kevin> + TRY_CATCH (except, RETURN_MASK_ALL)
> > Kevin> + {
> > Kevin> + 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> + }
> > Kevin> + if (except.reason < 0)
> > Kevin> + {
> > Kevin> + gdbpy_convert_exception (except);
> > Kevin> + gdbpy_print_stack ();
> > Kevin> + }
> >
> > ... then the TRY_CATCH in bpfinishpy_out_of_scope is not needed.
>
> right
>
> > Kevin> + struct cleanup *cleanup = ensure_python_env (target_gdbarch,
> > Kevin> + current_language);
> >
> > Indentation.
>
> fixed
>
>
> There is no regression against the current tree (2011-11-18)
>
>
> Thanks,
>
> Kevin
Hello,
I noticed a bug in the previous version of the patch, which led to
`out_of_scope' callback being triggered when it was not supposed to be
(namely, when the breakpoint was hit, the execution continued, and
then the callback was triggered before the temporary breakpoint could
be deleted.
I fixed it by ensuring that the breakpoint is active before triggering
the notification:
> if (bpfinish_obj->py_bp.bp->enable_state == bp_enabled
> && PyObject_HasAttrString (py_obj, outofscope_func))
> ...
Cordially,
Kevin
--
2011-11-30 Kevin Pouget <kevin.pouget@st.com>
Introduce gdb.FinishBreakpoint in Python
* Makefile.in (SUBDIR_PYTHON_OBS): Add py-finishbreakpoint.o.
(SUBDIR_PYTHON_SRCS): Add python/py-finishbreakpoint.c.
Add build rule for this file.
* infcmd.c (print_return_value): Split to create get_return_value.
(get_return_value): New function based on print_return_value. Handle
case where stop_registers are not set.
* inferior.h (get_return_value): New prototype.
* python/py-breakpoint.c (bppy_pending_object): Make non-static.
(gdbpy_breakpoint_created): Set is_py_finish_bp is necessary.
(struct breakpoint_object): Move to python-internal.h
(BPPY_REQUIRE_VALID): Likewise.
(BPPY_SET_REQUIRE_VALID): Likewise.
(gdbpy_breakpoint_created): Initialize is_finish_bp.
(gdbpy_should_stop): Add pre/post hooks before/after calling stop
method.
* python/python-internal.h (breakpoint_object_type): Add as extern.
(bppy_pending_object): Likewise.
(typedef struct breakpoint_object) Removed.
(struct breakpoint_object): Moved from py-breakpoint.c.
Add field is_finish_bp.
(BPPY_REQUIRE_VALID): Moved from py-breakpoint.c.
(BPPY_SET_REQUIRE_VALID): Likewise.
(frame_object_to_frame_info): New prototype.
(gdbpy_initialize_finishbreakpoints): New prototype.
(bpfinishpy_is_finish_bp): Likewise.
(bpfinishpy_pre_stop_hook): Likewise.
(bpfinishpy_post_stop_hook): Likewise.
* python/py-finishbreakpoint.c: New file.
* python/py-frame.c(frame_object_to_frame_info): Make non-static and
accept PyObject instead of frame_object.
(frapy_is_valid): Don't cast to frame_object.
(frapy_name): Likewise.
(frapy_type): Likewise.
(frapy_unwind_stop_reason): Likewise.
(frapy_pc): Likewise.
(frapy_block): Likewise.
(frapy_function): Likewise.
(frapy_older): Likewise.
(frapy_newer): Likewise.
(frapy_find_sal): Likewise.
(frapy_read_var): Likewise.
(frapy_select): Likewise.
* python/python.c (gdbpy_is_stopped_at_finish_bp): New noop function.
(_initialize_python): Add gdbpy_initialize_finishbreakpoints.
* python/python.h: Include breakpoint.h
(gdbpy_is_stopped_at_finish_bp): New prototype.
doc/
* gdb.texinfo (Finish Breakpoints in Python): New subsection.
(Python API): Add menu entry for Finish Breakpoints.
testsuite/
* gdb.python/py-breakpoint.exp (mult_line): Define and use variable
instead of line number.
* gdb.python/py-finish-breakpoint.c: New file.
* gdb.python/py-finish-breakpoint.exp: New file.
* gdb.python/py-finish-breakpoint.py: New file.
* gdb.python/py-finish-breakpoint2.cc: New file.
* gdb.python/py-finish-breakpoint2.exp: New file.
* gdb.python/py-finish-breakpoint2.py: New file.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Introduce-gdb.FinishBreakpoint.patch
Type: text/x-patch
Size: 55855 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20111130/4d1aeaa6/attachment.bin>
More information about the Gdb-patches
mailing list