Re: [RFC] Python Finish Breakpoints

[Tom Tromey <tromey@redhat.com>]

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

Kevin> +  if (except.reason < 0)
Kevin> +    {
Kevin> +      gdbpy_convert_exception(except);

Missing space.

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.

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.

Kevin> +  BPPY_SET_REQUIRE_VALID (&self_bpfinish->py_bp);

Hm, why is this here?

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.

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.

Tom