Re: [RFC] Python Finish Breakpoints

[Kevin Pouget <kevin dot pouget at gmail dot com>]

Hello,

thanks for you comments, I replied inline:


> 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.

There isn't. I just followed the "finish" command implementation and
understood that this function looked like what I was looking for!
I re-wrote this part of the code to use the existing API

> 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.

you're right; I chose the second way,

breakpoint.h:
enum py_bp_type
  {
    py_bp_none,         /* No Python object.  */
    py_bp_standard,     /* Ordinary breakpoint object.  */
    py_bp_finish        /* FinishBreakpoint object.  */
  };

    /* Type of the Python Breakpoint object stored in `py_bp_object'.  */
    enum py_bp_type py_bp_type;

py-breakpoint.c:
gdbpy_is_stopped_at_bp_type (bpstat stop_bpstat, enum py_bp_type type)

so that it doesn't require GIL state handling

> 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.

I'm not sure what you're talking about; I re-explain and document this
aspect at the bottom of the mail, you'll tell me if this is still
relevant

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

I copied it from py-breakpoint.c:
> static char * const stop_func = "stop";

and I think thank we can't change it, otherwise it conflicts with
> PyObject* PyObject_CallMethod(PyObject *o, char *method, char *format, ...)

and creates a compiler warning


> 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 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.

as per your two comments, I now only store the `struct type'  of the
function and the return value,
and cache the `gdb.Value' object when GDB stops at a FinishBreakpoint.

`FinishBreakpoint.return_value' may have to populate the cache when
it's accessed from Breakpoint.stop() because there is no guarantee the
FinishBreakpoint observer will be notified first.

> 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.

fixed

> 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.

fixed

> 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.

funny but correct, or too funny? ;)

> 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

I'll work on the tests for the next version of the patch ("return"
should already be covered)


--

so, about the `out_of_scope' stuffs,

> 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.

here is a bit of documentation about this class:

@subsubsection Finish Breakpoints

@cindex python finish breakpoints
@tindex gdb.FinishBreakpoint

A @fdb{finish breakpoint} is a breakpoint set at the return address of
a frame, based on the "finish command. @code{gdb.FinishBreakpoint} extends
@code{gdb.Breakpoint}

@defmethod FinishBreakpoint __init__ frame @r{[}internal@r{]}
Create a FinishBreakpoint at the return of the @code{gdb.Frame} object
@var{frame}. The optional @var{internal} argument allows the breakpoint
to become invisible to the user. @xref{Breakpoints In Python}, for further
details about this argument.
@end defmethod

@defop Operation {gdb.Breakpoint} out_of_scope (self)
In some circonstances (eg, @code{longjmp}, C++ exceptions, @value{GDBN}
@code{return} command, ...), a function may not properly terminate, and thus
never hit a @{code} FinishBreakpoint. When @value{GDBN} notices such a
situation, the @code{out_of_scope} function will be triggered.

This method will only be called if the @code{out_of_scope_notif} attribute
is @code{True}.

Deleting (@code{gdb.Breakpoint.delete()}) the @code{FinishBreakpoint} is
allowed in this function.

You may want to sub-class the @code{gdb.FinishBreakpoint} and implement the
@code{out_of_scope} method:

@smallexample
class MyFinishBreakpoint (gdb.FinishBreakpoint)
	def stop (self):
		print "normal finish"
		return True
	
	def out_of_scope():
		print "abnormal finish"
@end smallexample
@end defop

@defmethod FinishBreakpoint check_scope
Because @value{GDBN} currently only checks if @code{FinishBreakpoint}s ran
out of scope at normal stops, this method allows to force the verification
and trigger the @code{out_of_scope} method if necessary. It will return
@code{True} if @code{out_of_scope()} was triggered, @code{False} otherwise.
@end defmethod


@defivar FinishBreakpoint out_of_scope_notif
This attribute will be @code{True} until the @code{out_of_scope} method has
been called and @code{False} afterwards. This attribute is writeable, so out
of scope notifications can be re-enabled.
@end defivar

@defivar FinishBreakpoint return_value
When @value{GDBN} is stopped at a @code{FinishBreakpoint}, and the frame
used to build the @code{FinishBreakpoint} had debug symbols, this attribute
will contain a @code{gdb.Value} object corresponding to the return value of
the function. The value will be @code{None} if the value was not computable.
This attribute is not writable.
@end defivar


> 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?

as described, this is useful especially in the case of a GDB-forced
"return": normal_stop observers are not notified, so `out_of_scope()'
is not triggered. `check_scope()' allows Python scripts to force this
verification when they need. The reason why it's a method is that it
may execute `out_of_scope()', which was, from my point of view,
inconsistent with the concept of 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 },

I've got some difficulties to find a correct name for this attribute,
which is used for two purposes:
- avoid calling `out_of_scope' every normal_stop when the breakpoint
is not anymore in the callstack
- allow the script to re-activate notification when it wants to
're-use' the FinishBreakpoint (instead of deleting it / creating a new
one)


cordially,

Kevin

Attachment: finish_bp.txt
Description: Text document