On Fri, Nov 4, 2011 at 10:03 PM, Tom Tromey wrote: > > >>>>> "Kevin" == Kevin Pouget 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> +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. This part is a bit tricky, and your suggestion has highlighted a problem I didn't consider/test: py-breakpoint.c::gdbpy_should_stop is executed _before_ we have the chance to save the stop_registers, used to compute the return value (in infrun.c::normal_stop). (the problem existed since the beginning, but I never faced it before) I've updated the function infcmd.c::get_return_value to take this situation into account, using the current registers if the 'stop_registers' are not set, based on what is done in infrun.c::normal_stop: > struct value * > get_return_value (struct type *func_type, struct type *value_type) > ... > /* If stop_registers where not saved, use the current registers. */ > if (!stop_regs) > { > stop_regs = regcache_dup (get_current_regcache ()); > cleanup = make_cleanup_regcache_xfree (stop_regs); > } but I can't say that I'm confident with regcache handling, and I don't know if these lines would have unexpected side effects ... The patch enclosed passes the testsuite with no regression on x86_64/fedora 15 > 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> +  if (except.reason < 0) > Kevin> +    { > Kevin> +      gdbpy_convert_exception(except); > > Missing space. > > 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. all fixed, thanks > 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. I think that I handle that in the following lines: > Kevin> + if (except.reason < 0 > Kevin> + || !self_bpfinish->return_type || !self_bpfinish->function_type) I think I wrote a word about that in the previous mail, anyway, my feeling was that I don't want to abort the FinishBreakpoint creation just because of a return value not computable, so I currently nullify these fields and silently disable return value computation. I can't see any straightforward way to notify Python about that, warning/exceptions won't suite ... otherwise, I could expose the return_type to the Python interface, this way, one could check that it's not None and now if GDB will/might be able to compute the return value when the FinishBP is hit > Kevin> +  BPPY_SET_REQUIRE_VALID (&self_bpfinish->py_bp); > > Hm, why is this here? no reason apparently, the try/catch above shall ensure that the BP is valid > 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. right, done > 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. I'm not exactly sure what was you concern here, as far as I understand, the GIL was acquired in bpfinishpy_handle_stop, so it should have no effect in detect_out_scope_cb. So I've removed it from detect_out_scope_cb as it was useless. I've also made a litlle modification in the FinishBreakpoint __init__, which now default it's frame argument to gdb.current_frame(), instead of making it mandatory. I've updated the documentation and testsuite accordingly. Thanks for the time you spend reviewing my patches, Kevin -- 2011-11-09 Kevin Pouget Introduce gdb.FinishBreakpoint 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 (print_return_value): Split to create get_return_value. (get_return_value): New function based on print_return_value. Handle case where stop_registers are not set. * inferior.h (get_return_value): New prototype. * python/py-breakpoint.c (bppy_pending_object): Make non-static. (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. (gdbpy_should_stop): Add pre/post hooks before/after calling stop method. * 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): Moved from py-breakpoint.c. (BPPY_SET_REQUIRE_VALID): Likewise. (frame_object_to_frame_info): New prototype. (gdbpy_initialize_finishbreakpoints): New prototype. (bpfinishpy_is_finish_bp): Likewise. (bpfinishpy_pre_stop_hook): Likewise. (bpfinishpy_post_stop_hook): 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.