[RFC] Python Finish Breakpoints

[Kevin Pouget]

On Fri, Nov 4, 2011 at 10:03 PM, Tom Tromey <tromey@redhat.com> wrote:
>
> >>>>> "Kevin" == Kevin Pouget <kevin.pouget@gmail.com> writes:
>
> Tom> It seems like it should be an error to try to compute the return value
> Tom> when not stopped at this breakpoint.
>
> Kevin> I'm not totally convinced ...
> Kevin> what would you think about throwing an AttributeError("return_value
> Kevin> not available yet") when accessing the attribute before the breakpoint
> 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> +      for (bs = inferior_thread ()->control.stop_bpstat;
> Kevin> +          bs; bs = bs->next)
> Kevin> +        {
> Kevin> +          struct breakpoint *bp = bs->breakpoint_at;
> Kevin> +
> Kevin> +          if (bp != NULL && (PyObject *) bp->py_bp_object == self)
> Kevin> +            {
> Kevin> +              bpfinish_stopped_at_finish_bp (self_finishbp);
> Kevin> +              if (PyErr_Occurred ())
> Kevin> +                return 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)
> ...
>   /* If stop_registers where not saved, use the current registers.  */
>   if (!stop_regs)
>     {
>       stop_regs = regcache_dup (get_current_regcache ());
>       cleanup = make_cleanup_regcache_xfree (stop_regs);
>     }

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/fedora 15

> Kevin> +  TRY_CATCH (except, RETURN_MASK_ALL)
> Kevin> +    {
> Kevin> +      struct value *ret =
> Kevin> +          get_return_value (type_object_to_type (py_bp->function_type),
> Kevin> +                            type_object_to_type (py_bp->return_type));
> Kevin> +
> Kevin> +      if (ret)
> Kevin> +        py_bp->return_value = value_to_value_object (ret);
> Kevin> +      else
> Kevin> +        py_bp->return_value = Py_None;
> Kevin> +    }
> Kevin> +  if (except.reason < 0)
> Kevin> +    gdbpy_convert_exception(except);
>
> Missing a space.
>
> I think you need to Py_INCREF Py_None here.
>
>
> Kevin> +  if (except.reason < 0)
> Kevin> +    {
> Kevin> +      gdbpy_convert_exception(except);
>
> Missing space.
>
> Kevin> +  if (except.reason < 0
> Kevin> +      || !self_bpfinish->return_type || !self_bpfinish->function_type)
> Kevin> +    {
> Kevin> +      /* Won't be able to compute return value.  */
> Kevin> +      self_bpfinish->return_type = NULL;
> Kevin> +      self_bpfinish->function_type = NULL;
>
> Need decrefs here.

all fixed, thanks

> Kevin> +                  self_bpfinish->return_type = type_to_type_object (ret_type);
> Kevin> +                  self_bpfinish->function_type =
> Kevin> +                      type_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> +  if (except.reason < 0
> Kevin> +      || !self_bpfinish->return_type || !self_bpfinish->function_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> +  BPPY_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 valid

> Kevin> +static void
> Kevin> +bpfinishpy_out_of_scope (struct finish_breakpoint_object *bpfinish_obj)
> Kevin> +{
> [...]
> Kevin> +  delete_breakpoint (bpfinish_obj->py_bp.bp);
>
> I think it needs a TRY_CATCH.
>
> Kevin> +      else 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> +    }
>
> This too, I think.

right, done

> Kevin> +static void
> Kevin> +bpfinishpy_handle_stop (struct bpstats *bs, int print_frame)
> Kevin> +{
> Kevin> +  struct cleanup *cleanup = ensure_python_env (get_current_arch (),
> Kevin> +                                               current_language);
> Kevin> +
> Kevin> +  iterate_over_breakpoints (bpfinishpy_detect_out_scope_cb,
> Kevin> +                            bs == NULL ? NULL : bs->breakpoint_at);
>
> bpfinishpy_detect_out_scope_cb still acquires the GIL (via ensure_python_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  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 (Breakpoints In Python): New subsection: Finish
	Breakpoints.
	(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: 55589 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20111109/8f640754/attachment.bin>