Re: [RFC] Python Finish Breakpoints

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Kevin" == Kevin Pouget <kevin.pouget@gmail.com> writes:

You need a ChangeLog.

Kevin> -static PyTypeObject breakpoint_object_type;
Kevin> +PyTypeObject breakpoint_object_type;

The old line here was a forward declaration.
I think you should just entirely remove it.

Kevin> +  /* If it's a temporary breakpoint.  */
Kevin> +  if (bpfinishpy_is_finish_bp (py_bp))
Kevin> +    {
Kevin> +      /* Can't delete it here.  */
Kevin> +      gdb_assert (b->disposition == disp_del);
Kevin> +      disable_breakpoint (b);
Kevin> +    }

I don't understand this code.
It seems to me that disp_del is a better setting here.

Kevin> +      newbp->is_finish_bp = bpfinishpy_is_finish_bp ((PyObject *) newbp);

I think this flag could be set more cleanly in bpfinishpy_init.

Kevin> +  /* gdb.Type object of the function finished by this breakpoint.  */
Kevin> +  PyObject *function_type;
Kevin> +  /* gdb.Type object of the value return by the breakpointed function.  */
Kevin> +  PyObject *return_type;
Kevin> +  /* When stopped at this FinishBreakpoint, value returned by the function;
Kevin> +     Py_None if the value is not computable; NULL if GDB is not stopped at
Kevin> +     a FinishBreakpoint.  */
Kevin> +  PyObject *return_value;

I think the new class needs a destructor which decrefs these.

Kevin> +static void
Kevin> +bpfinish_stopped_at_finish_bp (struct finish_breakpoint_object *py_bp)
Kevin> +{
Kevin> +  if (py_bp->return_type)
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));

Calls into gdb have to be wrapped in TRY_CATCH.
Then errors have to be propagated to Python somehow, or printed and
ignored via gdbpy_print_stack.  Given the context I would say propagate.

Kevin> +  /* Check if we have a cached value.  */
Kevin> +  if (!self_finishbp->return_value)
Kevin> +    {
Kevin> +      bpstat bs;
Kevin> +
Kevin> +      BPPY_REQUIRE_VALID (&self_finishbp->py_bp);
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> +              bpfinish_stopped_at_finish_bp (self_finishbp);
Kevin> +        }

It seems like it should be an error to try to compute the return value
when not stopped at this breakpoint.

Kevin> +struct breakpoint *
Kevin> +gdbpy_is_stopped_at_finish_bp (bpstat stop_bpstat)
Kevin> +{

Since the name implies that this is a predicate and since the result is
only ever used as a boolean, I think this should return int.

Kevin> +  bpstat bs;
Kevin> +
Kevin> +  for (bs = stop_bpstat; bs; bs = bs->next)
Kevin> +    {
Kevin> +      if (bs->breakpoint_at
Kevin> +          && bpfinishpy_bp_is_finish_bp ((breakpoint_object *)
Kevin> +                                         bs->breakpoint_at->py_bp_object))

I am not really sure about this.  It seems like it may be pedantically
incorrect, though it is hard to see when it could possibly fail.  That
is, is the GIL required or not?  It doesn't call a function and the
breakpoint owns a ref to the breakpoint object, so it seems like it
could not be deleted out from under us.

I'm inclined to say it is ok.

Kevin> +  breakpoint_object *self_bp = (breakpoint_object *) self;
Kevin> +  struct finish_breakpoint_object *self_bpfinish =
Kevin> +      (struct finish_breakpoint_object *) self;

I think this is the only use of self in this function.
Just drop it and cast directly to the most specific subclass.

Kevin> +  prev_frame = get_prev_frame (frame);
Kevin> +  if (prev_frame == 0)
Kevin> +    {
Kevin> +      PyErr_SetString (PyExc_ValueError, 
Kevin> +           _("\"FinishBreakpoint\" not meaningful in the outermost frame."));
Kevin> +      return -1;
Kevin> +    }
Kevin> +  else if (get_frame_type (prev_frame) == DUMMY_FRAME)
Kevin> +      {
Kevin> +        PyErr_SetString (PyExc_ValueError,
Kevin> +                  _("\"FinishBreakpoint\" cannot be set on a dummy frame."));
Kevin> +        return -1;
Kevin> +      }

I think the calls to get_prev_frame and get_frame_type need to be
wrapped in a TRY_CATCH.

Kevin> +  frame_id = get_frame_id (prev_frame);
Kevin> +  if (frame_id_eq (frame_id, null_frame_id))

Likewise.

I'd try to put all the gdb-facing logic into a single big TRY_CATCH.

Kevin> +  if (internal)
Kevin> +    {
Kevin> +      internal_bp = PyObject_IsTrue (internal);
Kevin> +      if (internal_bp == -1) 
Kevin> +        {
Kevin> +          PyErr_SetString (PyExc_ValueError, 
Kevin> +                           _("The value of `internal' must be a boolean."));
Kevin> +          return -1;

Do you need to decref 'frame_obj' here?  I suspect so.
There are other early returns that probably need this.
A typical solution is a label where all the locals are xdecref'd then
return -1.

Kevin> +  /* Find the function we will return from.  */
Kevin> +  self_bpfinish->return_type = NULL;
Kevin> +  self_bpfinish->function_type = NULL;

These can be left NULL in the object.  What happens if you try to fetch
the return value in that case?

Kevin> +  if (get_frame_pc_if_available (frame, &pc))
Kevin> +    {
Kevin> +      function = find_pc_function (pc);
Kevin> +      if (function != NULL)
Kevin> +        {
Kevin> +          struct type *ret_type = TYPE_TARGET_TYPE (SYMBOL_TYPE (function));
Kevin> +

More TRY_CATCH.

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);

Error check.

Kevin> +              self_bpfinish->function_type =
Kevin> +                  type_to_type_object (SYMBOL_TYPE (function));

Likewise.

Kevin> +  if (except.reason < 0)
Kevin> +    {
Kevin> +      PyErr_Format (except.reason == RETURN_QUIT
Kevin> +                    ? PyExc_KeyboardInterrupt : PyExc_RuntimeError,
Kevin> +                    "%s", except.message);
Kevin> +      return -1;

Just use GDB_PY_SET_HANDLE_EXCEPTION.

Kevin> +int
Kevin> +bpfinishpy_is_finish_bp (PyObject *obj)
Kevin> +{
Kevin> +  return obj != NULL
Kevin> +         && PyObject_TypeCheck (obj, &finish_breakpoint_object_type);
Kevin> +}

Kevin> +int
Kevin> +bpfinishpy_bp_is_finish_bp (breakpoint_object *bp_obj)
Kevin> +{
Kevin> +  return bp_obj != NULL
Kevin> +         && bp_obj->is_finish_bp;
Kevin> +}

Are both of these needed?

Kevin> +static void
Kevin> +bpfinishpy_handle_stop (struct bpstats *bs, int print_frame)
Kevin> +{
Kevin> +  iterate_over_breakpoints (bpfinishpy_detect_out_scope_cb,
Kevin> +                            bs == NULL ? NULL : bs->breakpoint_at);
Kevin> +}

The way this is written, it will acquire and release the GIL for each
breakpoint.

I think it would be better to acquire + release just once.

Kevin> +static void
Kevin> +bpfinishpy_handle_exit (struct inferior *inf)
Kevin> +{
Kevin> +  iterate_over_breakpoints (bpfinishpy_detect_out_scope_cb, NULL);
Kevin> +}

Likewise.

Kevin> +struct breakpoint *gdbpy_is_stopped_at_finish_bp (bpstat stop_bpstat);
Kevin>  #endif /* GDB_PYTHON_H */

Newline between these two lines.

Tom