Re: [RFC] Python Finish Breakpoints

[Tom Tromey <tromey@redhat.com>]

>>>>> "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.

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> 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.

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).  The 

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.

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.

Kevin> +type is @code{VOID}

I think just @code{void} is better.

Kevin> +  /* If stop_registers where not saved, use the current registers.  */

s/where/were/

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.

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.

Kevin> +          PyErr_SetString (PyExc_ValueError,
Kevin> +            _("\"FinishBreakpoint\" not meaningful in the outermost frame."));
       
Indentation.

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.

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.

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.

Kevin> +  struct cleanup *cleanup = ensure_python_env (target_gdbarch,
Kevin> +      current_language);

Indentation.

Tom