Re: [RFC] Python Finish Breakpoints

[Tom Tromey <tromey@redhat.com>]

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

Moving to gdb-patches.

Kevin> I've included with this mail a complete patch build agains the
Kevin> current HEAD, and checked that there was no regression in the
Kevin> testsuite

Thanks.

I'm sorry about the delay in my reply.  I'm going to try to prioritize
patch review and email a bit higher in the future.

I think this functionality is very good.  I am not sure about some
aspects of the implementation, though, and I have some style nits to
pick.

Kevin> -static void
Kevin> +void
Kevin>  create_breakpoint_sal (struct gdbarch *gdbarch,
Kevin>  		       struct symtabs_and_lines sals, char *addr_string,
Kevin>  		       char *cond_string,

What is the rationale for using this function in particular?

It seems to me that it would probably be better to make an explicit_pc=1
SAL using the unwound PC.  Then there would be no reason to avoid the
currently exported API.

Kevin> +  struct value *value = get_return_value(func_type, value_type);

Missing space before the "(".  This appears multiple places in the
patch.

Kevin> -  if (inferior_thread ()->control.proceed_to_finish)
Kevin> +  if (gdbpy_is_stopped_at_finish_bp (inferior_thread ()->control.stop_bpstat)
Kevin> +      || inferior_thread ()->control.proceed_to_finish)

gdbpy_is_stopped_at_finish_bp is not safe to call here -- it assumes the 
GIL has been acquired, which it has not.  I would rather it not be changed
to acquire the GIL, however.  I think one of two other approaches would
be preferable.

One way you could handle this is to add a new constant to enum bptype.
This is likely to be pretty invasive.

Another way would be to add a flag to the struct breakpoint itself.

Yet another way would be a new breakpoint_ops method.

This is related, in a way, to the out-of-scope handling.  Right now the
patch tries to reimplement the existing breakpoint re-setting logic in
py-finishbreakpoint.c, via observers.  I think it would be better to
have this be done automatically by the existing code in breakpoint.c,
perhaps adding some additional python-visible notification step.

Kevin> +static char * const outofscope_func = "out_of_scope";

"const char *".

Kevin> +  /* The function finished by this breakpoint.  */
Kevin> +  struct symbol *function;

If you want to store a pointer to a symbol, then you have to account for
the case where the objfile is destroyed.  Otherwise you can end up with
a dangling pointer.

Alternatively, perhaps you could have this refer to a Symbol object; but
then you have to be careful to check it for validity before using it.

Actually, it seems that the symbol is only used to get the function's
return type.  You might as well just compute that up front and store a
Type.

Kevin> +/* Python function to set the 'out_of_scope_notif' attribute of
Kevin> +   FinishBreakpoint.  */
Kevin> +
Kevin> +static int
Kevin> +bpfinishpy_set_outofscope_notif (PyObject *self, PyObject *newvalue,
Kevin> +                                 void *closure)

I don't understand the point of this function.

I think documentation would help.

Kevin> +/* Python function to get the 'return_value' attribute of
Kevin> +   FinishBreakpoint.  */
Kevin> +
Kevin> +static PyObject *
Kevin> +bpfinishpy_get_returnvalue (PyObject *self, void *closure)
Kevin> +{
Kevin> +  struct finish_breakpoint_object *self_finishbp =
Kevin> +      (struct finish_breakpoint_object *) self;
Kevin> +
Kevin> +  BPPY_REQUIRE_VALID (&self_finishbp->py_bp);
Kevin> +
Kevin> +  if (self_finishbp->function == NULL)
Kevin> +    goto return_none;
Kevin> +
Kevin> +  /* Ensure that GDB is stopped at this FinishBreakpoint.  */
Kevin> +  if (inferior_thread ()->control.stop_bpstat != NULL)
Kevin> +    {
Kevin> +      bpstat bs;
Kevin> +
Kevin> +      for(bs = inferior_thread ()->control.stop_bpstat;
Kevin> +          bs; bs = bs->next)

I am not an expert here, but I think it is probably no good to rely on
this state remaining live.

I think it would be better to simply have a stop at one of these
breakpoints compute and cache the Value object immediately.

Kevin> +static void
Kevin> +gdbpy_out_of_scope (struct finish_breakpoint_object *bpfinish_obj)
Kevin> +{
Kevin> +  breakpoint_object *bp_obj = (breakpoint_object *) bpfinish_obj;
Kevin> +  PyObject *py_obj = (PyObject *) bp_obj ;
Kevin> +  
Kevin> +  bpfinish_obj->out_of_scope_notif = 0;
Kevin> +
Kevin> +  if (PyObject_HasAttrString (py_obj, outofscope_func))
Kevin> +    {
Kevin> +      struct gdbarch *garch =  bp_obj->bp->gdbarch ?  
Kevin> +          bp_obj->bp->gdbarch : get_current_arch ();
Kevin> +      struct cleanup *cleanup = ensure_python_env (garch, current_language);

You can't call any Python functions without the GIL.
This applies to PyObject_HasAttrString here.

In this case, though, gdbpy_out_of_scope is called by one function that
already has the GIL.  So, I think the acquisition should be pushed into
its other caller.

Kevin> +  pc = get_frame_pc (prev_frame);

This can throw an exception.  So, it needs to be wrapped in a TRY_CATCH.
This may apply to some other GDB functions called by the "Python-facing"
code, I did not check them all.

Kevin> +static PyMethodDef finish_breakpoint_object_methods[] = {
Kevin> +  { "check_scope", bpfinishpy_check_scope, METH_NOARGS,
Kevin> +    "check_scope () -> Boolean.\n\
Kevin> +Return true if out_of_scope() has been triggered, false if not." },

How is this useful?
And, why a method instead of an attribute?

Kevin> +static PyGetSetDef finish_breakpoint_object_getset[] = {
Kevin> +  { "out_of_scope_notif", bpfinishpy_get_outofscope_notif, bpfinishpy_set_outofscope_notif,

I don't like the name "out_of_scope_notif", particularly if this is an
apt description of its meaning:

Kevin> +    "Boolean telling whether the breakpoint is still within the scope \
Kevin> +of the current callstack.", NULL },


Kevin> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-cc.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint-cc.exp

Funny file name.

Kevin> +gdb_test "python ExceptionBreakpoint()" "ExceptionBreakpoint init" "set BP before throwing the exception"
Kevin> +gdb_test "python print len(gdb.breakpoints())" "4" "check number of BPs"
Kevin> +gdb_test "continue" ".*stopped at ExceptionFinishBreakpoint.*" "check FinishBreakpoint in catch()"
Kevin> +gdb_test "python print len(gdb.breakpoints())" "4" "check finish BP removal"

I don't think this tests the most important case -- where an exception
is thrown, invalidating the return breakpoint.

That is, put a breakpoint in throw_exception_1, continue to there, then
set the breakpoint, then continue again.

I didn't examine the longjmp test but the same idea applies there.

Other cases to consider are -

* inferior exec
* inferior exit
* explicit inferior function call
* "return" command

Tom