This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC] Python Finish Breakpoints
- From: Kevin Pouget <kevin dot pouget at gmail dot com>
- To: Tom Tromey <tromey at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 24 Nov 2011 09:26:55 +0100
- Subject: Re: [RFC] Python Finish Breakpoints
- References: <BANLkTim+YH64zkWvdz2_kVUUj=AJ7v4LKw@mail.gmail.com> <BANLkTi=Eu-5B4YyhP2rGdQXgXbBN-EmLKA@mail.gmail.com> <BANLkTikt2hEUcXkGVH44NaUcwiF1SGdMaw@mail.gmail.com> <BANLkTi=UpgogckTD5MZsW+PC5d2F8X-+jA@mail.gmail.com> <BANLkTi==bj50mZgFKq_qA-+3-2CQA3tBVw@mail.gmail.com> <BANLkTimmYEmvKT_984jYEVZnA5RGFpEgNw@mail.gmail.com> <m34o4qd5lh.fsf@fleche.redhat.com> <BANLkTinZ3DSVPRvSiFydCuYs71-DM5-aOg@mail.gmail.com> <m3vcwvubry.fsf@fleche.redhat.com> <BANLkTinBg76_-9i5n=VA2NXp+ZF978J=ig@mail.gmail.com> <m3y5wfl7ky.fsf@fleche.redhat.com> <CAPftXUKHxpww_pEcsMruR6rqo40xw9q9Jzn65-dXys_COt6JbQ@mail.gmail.com> <CAPftXUKsSSb=36CQ+ptGenkt4Q167TtJXuDD2fkU7KFYDu_tPQ@mail.gmail.com> <CAPftXUL0x7MG8NJtGxqVoEs4Z_D+ipQSnxCMEirQDa1ZPWC8Lw@mail.gmail.com> <m3hb2y5q8i.fsf@redhat.com> <CAPftXU+othLQZ=jLUap1vvvaKfcW5e+CyuGeuKDkm=0xpCx-vw@mail.gmail.com> <m3y5w47szo.fsf@fleche.redhat.com> <CAPftXULH7STUB5-oSou7xb_Ww36bC1pDoKfzGvujSCM8PJyG=Q@mail.gmail.com> <CAPftXUKRH9fJVF9UyiHT4tngzT+59HDQGiMg+zti7ZMZDdwy1A@mail.gmail.com> <m3obwrppl4.fsf@fleche.redhat.com> <CAPftXU+y1YoqT7H6YwFKiqgmxhVSdpPTWQGek899utvPO2bmfw@mail.gmail.com> <m3ehx6fxnf.fsf@fleche.redhat.com> <CAPftXUJwtGJhqFyfX6LVK87QH2xeLkvv5kx=yaEnYJMv0YR_rw@mail.gmail.com>
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
2011-11-18 ?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.