Re: [RFC] Python Finish Breakpoints

[Kevin Pouget <kevin.pouget@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.