On Fri, Oct 28, 2011 at 10:37 PM, Tom Tromey wrote: Hello, thanks for you're answer, I reply inline > You need a ChangeLog. sorry, I forgot to include it in the e-mail, you'll find it at the bottom > 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. I need to do it this way because as far as I've seen, "breakpoint_auto_delete" is only triggered in infrun.c::normal_stop, so if Breakpoint.stop returns False, the breakpoint is not deleted immediately, and may be hit twice. So I disable the breakpoint to avoid it. > Kevin> +      newbp->is_finish_bp = bpfinishpy_is_finish_bp ((PyObject *) newbp); > > I think this flag could be set more cleanly in bpfinishpy_init. you're right, fixed > 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. fixed > 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. you're right, bpfinish_stopped_at_finish_bp is called in 2 situations, so it just sets the error flag, then - direct access to returnvalue member --> propage exception to Python - bpfinishpy_handle_stop --> bpfinishpy_handle_stop() > 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. I'm not totally convinced ... what would you think about throwing an AttributeError("return_value not available yet") when accessing the attribute before the breakpoint is hit, but keep the cached value afterwards? > 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. changed > 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. no, the GIL is not required. This function is triggered from infrun.c:normal_stop, to determine if we want to save the stop_registers, like it's done for `finish' > 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. I'm not sure what you meant, *self_bp* was redundant so I removed it > 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. I wrapped them all and propagate the error with gdbpy_convert_exception > 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. > PyArg_ParseTupleAndKeywords (args, kwargs, "O|O", keywords, &frame_obj, &internal) No, according to the documentation, > Note that any Python object references which are provided to the caller are borrowed references; do not decrement their reference count! > 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? I've updated the comments to make it clearer: /* gdb.Type object of the value return by the breakpointed function. May be NULL if no debug information was available or return type was VOID. */ PyObject *return_type; /* gdb.Type object of the function finished by this breakpoint. Will be NULL if return_type is NULL. */ PyObject *function_type; if return_type is NULL, "return_value" attribute will be None (the return value is not computed/computable) I've updated the doc as well: > The value will be @code{None} if the function return type is @code{VOID} or if the return value was not computable. > 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. I'm not sure about the best way to handle these exceptions, so for all the error thrown when trying to compute the return_type/function_type, I just silently swallow it, and set the return_type to None. Propagating the exception is not suitable, because I don't want not to prevent the F_BP creation just because of the return value; and I don't want to write a warning within Python code ... > 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. right, fixed (the same should apply to gdb.Breakpoint creation, I copied the code from there) > 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? no, not anymore, I even removed both of them, > newbp->is_finish_bp = bpfinishpy_is_finish_bp ((PyObject *) newbp); > I think this flag could be set more cleanly in bpfinishpy_init. but I've rewritten this bit > 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. fixed I'm not 100% confident with this "ensure_python_env" and the value of the "arch" parameter; I based my code on how py-inferior does, so: observer "normal stop" --> get_current_arch () observer "exit" --> target_gdbarch and I kept > garch = b->gdbarch ? b->gdbarch : get_current_arch (); in the callback, to ensure that the arch is actually the one from the BP, if any let me know if this is not the right way to do > Kevin> +struct breakpoint *gdbpy_is_stopped_at_finish_bp (bpstat stop_bpstat); > Kevin>  #endif /* GDB_PYTHON_H */ > > Newline between these two lines. fixed and finally, from the previous mail > You could subclass breakpoint and add a new field there. This would > segregate the changes in the python subdirectory. This may be best. > Alternatively you could make a bp_finish breakpoint. I'm not sure > whether this is sufficient or whether you would also need an additional > flag. If the latter you could make a bp_finish_python or something like > that -- I said earlier that this was probably invasive, but grepping for > bp_finish makes me think it wouldn't be too bad. I'm not sure about what you mean exactly, seems like half of it was already done ... anyway, I think that the original problem is almost gone: - FinishBreakpoint inherits from Breakpoint - there is a field "is_finish_bp" in gdb.Breakpoint C structure, - infrun.c can know without the GIL if a breakpoint object is a FinishBreakpoint using "gdbpy_is_stopped_at_finish_bp" Thanks for this thorough review, Kevin (tested with no regression against 7.3.50.20111028-cvs, Fedora-15 X86_64) 2011-11-02 Kevin Pouget Introduce gdb.FinishBreakpoints 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 (get_return_value): New function. (print_return_value): Split to create get_return_value. * inferior.h (get_return_value): New prototype. * infrun.c: Include python/python.h. (stop_registers): Mention FinishBreakpoint in description. (normal_stop): Set stop_registers if stopped at FinishBreakpoint. * python/py-breakpoint.c (bppy_pending_object): Make non-static. (gdbpy_should_stop): Disable temporary breakpoints. (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. * 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): Likewise. (BPPY_SET_REQUIRE_VALID): Likewise. (frame_object_to_frame_info): New prototype. (gdbpy_initialize_finishbreakpoints): New prototype. (bpfinishpy_is_finish_bp): 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.