Re: [RFC] Python Finish Breakpoints

Kevin Pouget
2011-10-13T14:34:12Z

On Mon, May 30, 2011 at 11:28 AM, Kevin Pouget <kevin.pouget@gmail.com> wrote:
>> Kevin> breakpoint.h:
>> Kevin> enum py_bp_type
>> Kevin>   {
>> Kevin>     py_bp_none,         /* No Python object.  */
>>
>> I don't think this one is needed.
>>
>> Kevin>     py_bp_standard,     /* Ordinary breakpoint object.  */
>> Kevin>     py_bp_finish        /* FinishBreakpoint object.  */
>>
>> These should be uppercase, but it seems to me that if there are just 2
>> states you might as well use an ordinary boolean(-ish) flag.
>
> OK, I wanted to let a room free for further Python-specific breakpoint
> handling, but if you feel like it's not necessary ...
> I changed it to "int is_py_finish_bp"
>
>> Kevin> as per your two comments, I now only store the `struct type'  of the
>> Kevin> function and the return value,
>>
>> You need to store a gdb.Type wrapper.
>> A 'struct type' can also be invalidated when an objfile is destroyed.
>
> I wouldn't mind, but I can't see how gdb.Type ensures validity, as far
> as I've seen, there is no "is_valid" method and I can't and no further
> verification during the Python -> C translation:
>
> struct type *
> type_object_to_type (PyObject *obj)
> {
>  if (! PyObject_TypeCheck (obj, &type_object_type))
>    return NULL;
>  return ((type_object *) obj)->type;
> }
>
>
>> Kevin> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint-cc.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint-cc.exp
>>
>> Tom> Funny file name.
>>
>> Kevin> funny but correct, or too funny? ;)
>>
>> It is more usual in the gdb test suite to give the .cc and .exp files
>> the same base name.
>
> so py-finish-breakpoint2.{exp,cc,py} should look more serious!
>
>>> Other cases to consider are -
>>> * inferior exec
>>> * inferior exit
>>> * explicit inferior function call
>>> * "return" command
>> Kevin> I'll work on the tests for the next version of the patch ("return"
>> Kevin> should already be covered)
>>
>> I will wait for this to do more review.
>
> these situations should now be covered in `'py-finish-breakpoint.exp"
>
>> Kevin> @defivar FinishBreakpoint out_of_scope_notif
>> Kevin> This attribute will be @code{True} until the @code{out_of_scope} method has
>> Kevin> been called and @code{False} afterwards. This attribute is writeable, so out
>> Kevin> of scope notifications can be re-enabled.
>> Kevin> @end defivar
>>
>> I still don't really understand under what circumstances it is useful
>> for a program to set this attribute.
>>
>> Kevin> - avoid calling `out_of_scope' every normal_stop when the breakpoint
>> Kevin> is not anymore in the callstack
>>
>> I think it would be ok to just leave this up to the subclass to handle.
>
> I thought about just getting rid of this part of the patch, but it
> really seem important to me.
> Here is a (pseudo-code) example to try to demonstrate that:
>
> function inner (param) {
>  if (param)
>    return 1
>  else
>    throw exception
> }
>
> function outter (param) {
>  return innter(param)
> }
>
> function main () {
>  try:
>    outter (TRUE)
>  except:
>    pass
>
>  try:
>    outter (FALSE)
>  except:
>    pass
>
>  try:
>    outter (TRUE)
>  except:
>    pass
> }
>
> in a Python script, you want to know the return value of OUTTER (in a
> script --> you don't know the actual shape of the Main function -- for
> instance think about the 'write(2)` function which returns the size of
> the data actually written)
>
> you'll need one gdb.Breakpoint and several gdb.FinishBreakpoint:
>
> class OutterBreakpoint(gdb.Breakpoint):
>  def __init__(self):
>    gdb.Breakpoint.__init__(self, "outter", internal=True)
>    self.silent = True
>
>  def stop():
>    OutterFinishBreakpoint(gdb.newest_frame())
>    return False
>
> class OutterFinishBreakpoint(gdb.FinishBreakpoint):
>  def stop():
>    print "outter finished with :", self.return_value
>    self.enable = False
>    gdb.post_event(self.delete)
>
>  def out_of_scope()
>    self.enable = False
>    gdb.post_event(self.delete)
>
> This `out_of_scope' function is the easiest way to cleanup the
> FinishBreakpoint when the finished function didn't return correctly
> (and is only useful in this situation, I admit)
>
>> Kevin> - allow the script to re-activate notification when it wants to
>> Kevin> 're-use' the FinishBreakpoint (instead of deleting it / creating a new
>> Kevin> one)
>>
>> I am not sure when this makes sense.
>
> that's the opposite situation, you know know the function will only be
> triggered from one (limitted set of) place, so you don't want to
> create/delete BPs each time:
>
>
> class OutterBreakpoint(gdb.Breakpoint):
>  ...
>  def stop():
>    if self.finish is None:
>        self.finish = OutterFinishBreakpoint(gdb.newest_frame())
>    return False
>
> class OutterFinishBreakpoint(gdb.FinishBreakpoint):
>  ...
>  def stop():
>     #don't delete
>     #treat normal termination
>
>  def out_of_scope():
>     #treat exception termination
>     self.out_of_scope_notif = True # allows to catch further
> exception termination
>
>
> I also added a verification which checks that `out_of_scope'  is only
> trigged for the inferior for the inferior which owns the breakpoint:
>
> +bpfinishpy_detect_out_scope_cb (struct breakpoint *b, void *args)
> ...
> +      else if (finish_bp->out_of_scope_notif
> +          && b->pspace == current_inferior()->pspace
> +          && (!target_has_registers || frame_find_by_id(b->frame_id) == NULL))
> +        {
> +          bpfinishpy_out_of_scope (finish_bp);
> +        }

Hello,

I would like to come back on this patch discussion, I know it has been
a while since the last post (May 30th), but I was waiting for my
copyright assignment to be ready, and then had to empty the patches
already in the pipe.

it appeared that the most stormy part was related to the
FinishBreakpoint.out_of_scope() callback:

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

I'll try a last time to convince you about its necessity, then I'll
consider dropping it off !

The main rational behind this callback is housekeeping: I'm building
new functionalities on top of the Python interface, and I would like
it to be invisible to the user (eg, when GDB performs a 'next/finish',
you don't want to let the internal breakpoints in the inferior when
the commands returns, in any situation.

In the case of FinishBreakpoints, I would like to easily know if when
the BP became meaningless (that is, the exec ran out of the scope
where the FBP can be trigged) and time has come to remove it.
Does it make sense?

Thanks,

Kevin