On Wed, Nov 2, 2011 at 3:43 PM, Kevin Pouget wrote: > 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 Hello, there was a little confusion in the previous patch, ensure_python_env/cleanup commented out, here is a cleaner version. Thanks, Kevin 2011-11-04 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.