This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC] Python Finish Breakpoints


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]