Re: [PATCH] improve python finish breakpoint for exceptions/longjmp case.

Jan Kratochvil
2012-11-22T18:15:31Z

On Tue, 06 Nov 2012 15:23:49 +0100, Andrew Burgess wrote:
> On 30/10/2012 5:40 PM, Jan Kratochvil wrote:
> > On Thu, 25 Oct 2012 21:23:19 +0200, Andrew Burgess wrote:
> >
> > Anyway we agree it is not transparent to "finish" anyway but this is more
> > a problem there are no observer-like breakpoints:
> >
> > ==> finish.c <==
> > void g (void) {}
> > void f (void) {
> > g ();
> > }
> > int main (void) {
> > f ();
> > return 0;
> > }
> >
> > ==> finish.cmd <==
> > file ./finish
> > start
> > step
> > break g
> > commands
> > echo hook-g\n
> > continue
> > end
> > finish
> >
> > -------------------
> >
> > hook-g
> > Actual:
> > [Inferior 1 (process 13204) exited normally]
> > (gdb) _
> > Expected:
> > main () at finish.c:7
> > 7 return 0;
> > (gdb) _
> >
>
> So currently the finish command stops whenever gdb stops, even though in
> this case once we stop (at g) we then automatically continue again.

I agree.

> In the example is it that you'd like to consider the "g" breakpoint
> observer-like (just because it contains a continue),

Right, but I see it was only my wish, GDB does not behave that way.

> or are you suggesting that the "finish" command should not auto-terminate
> once gdb stops?

I was suggesting "finish" should continue its "finish" work after the "g"
breakpoint commands were run. But I see it does not work that way.

Let's forget about this part.

> I can't really see how the above example relates to the issue with my patch, however,

I agree.

> I have a new patch in which I believe the level of risk of code corruption should be less.

The problem is it is not as safe as set_longjmp_breakpoint_for_call_dummy
which I was suggest to hook into. You are hooked by
observer_attach_normal_stop (bpfinishpy_handle_stop);
but that does not catch for example a "return" command.

==> 65.c <==
static int h (void) { return 1; }
static int f (void) { return h (); }
static int g (void) { return h (); }
int main (void) {
f ();
g ();
return 0;
}

==> 65.cmd <==
set confirm off
set height 0
set width 0
shell rm -f 65
shell gcc -o 65 65.c -Wall -g
file ./65
start
source 65.py
break h
continue
info break
python finishbp = MyFinishBreakpoint (gdb.newest_frame ())
info break
return
return
bt
continue
info break
python print finishbp.return_value

==> 65.py <==
class MyFinishBreakpoint (gdb.FinishBreakpoint):
def __init__(self, frame):
gdb.FinishBreakpoint.__init__ (self, frame)
print "MyFinishBreakpoint init"

def stop(self):
print "MyFinishBreakpoint stop"
print "return_value is: %d" % int (self.return_value)
return True

def out_of_scope(self):
print "MyFinishBreakpoint out of scope"

=============

and its output:

Temporary breakpoint 1 at 0x400511: file 65.c, line 5.

Temporary breakpoint 1, main () at 65.c:5
5 f ();
Breakpoint 2 at 0x4004f0: file 65.c, line 1.

Breakpoint 2, h () at 65.c:1
1 static int h (void) { return 1; }
Num Type Disp Enb Address What
2 breakpoint keep y 0x00000000004004f0 in h at 65.c:1
breakpoint already hit 1 time
Temporary breakpoint 3 at 0x400500: file 65.c, line 2.
MyFinishBreakpoint init
Num Type Disp Enb Address What
2 breakpoint keep y 0x00000000004004f0 in h at 65.c:1
breakpoint already hit 1 time
3 breakpoint del y 0x0000000000400500 in f at 65.c:2 thread 1
stop only in thread 1
#0 main () at 65.c:6

Breakpoint 2, h () at 65.c:1
1 static int h (void) { return 1; }
Num Type Disp Enb Address What
2 breakpoint keep y 0x00000000004004f0 in h at 65.c:1
breakpoint already hit 2 times
3 breakpoint del y 0x0000000000400500 in f at 65.c:2 thread 1
stop only in thread 1
None
(gdb) _

You can see in the end we already exited f() and entered it again but the
finish breakpoint still exists, out-of-scope detection did not catch it.

Unfortunately set_longjmp_breakpoint_for_call_dummy is a bit dummy frame
specific so it would need some extensions to be suitable for this purpose.

I believe it would be good to hook also into frame_pop().

> As far as I can see the only remaining issue is that using other commands
> that set/clear the longjmp breakpoints once a FinishBreakpoint is active
> (finish/step/etc) will remove the longjmp breakpoint, we'll then fallback to
> only calling the out-of-scope callback at the next stop. To solve this I'd
> like to change the longjmp breakpoint mechanism to support having multiple
> longjmp breakpoint sets active at once;

You would need a new type besides bp_longjmp and bp_exception. This is why
I created that bp_longjmp_call_dummy which I was suggesting to extend also for
your purposes. But I am OK with new bp_longjmp_py_finish_bpt or something.

I do not like much that you share tp->initiating_frame but I haven't found
a countercase reproducer.

> but I'd rather do this in a follow-up patch, and as the new behaviour is no
> worse than the original behaviour this feels like a reasonable compromise to
> me (but you might disagree).

I am OK with that, in fact I leave further Python API extensions on the other
maintainers.

[...]
> @@ -161,7 +168,7 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
> PyObject *frame_obj = NULL;
> int thread;
> struct frame_info *frame, *prev_frame = NULL;
> - struct frame_id frame_id;
> + struct frame_id prev_frame_id, init_frame_id;
> PyObject *internal = NULL;
> int internal_bp = 0;
> CORE_ADDR finish_pc, pc;
> @@ -184,6 +191,8 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>
> if (frame == NULL)
> goto invalid_frame;
> +
> + init_frame_id = get_frame_id (frame);
>
> TRY_CATCH (except, RETURN_MASK_ALL)
> {
> @@ -201,8 +210,8 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
> }
> else
> {
> - frame_id = get_frame_id (prev_frame);
> - if (frame_id_eq (frame_id, null_frame_id))
> + prev_frame_id = get_frame_id (prev_frame);
> + if (frame_id_eq (prev_frame_id, null_frame_id))
> PyErr_SetString (PyExc_ValueError,
> _("Invalid ID for the `frame' object."));
> }
> @@ -295,11 +304,18 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
> AUTO_BOOLEAN_TRUE,
> &bkpt_breakpoint_ops,
> 0, 1, internal_bp, 0);
> + set_longjmp_breakpoint (inferior_thread (), init_frame_id);

Here should be prev_frame_id instead. I find it maybe a bit confusing but it
is so, check 'case 2' in infrun.c 'case BPSTAT_WHAT_CLEAR_LONGJMP_RESUME'.
finish_command puts there the previous frame of where you execute the "finish"
command.

Countercase:

==> 64.c <==
#include <setjmp.h>
static jmp_buf env;
static int h (void) {
if (setjmp (env))
return 1;
longjmp (env, 1);
return 2;
}
int main (void) { return h (); }

==> 64.cmd <==
set confirm off
set height 0
set width 0
shell rm -f 64
shell gcc -o 64 64.c -Wall -g
file ./64
start
source 64.py
break h
continue
info break
python finishbp = MyFinishBreakpoint (gdb.newest_frame ())
info break
continue
python print finishbp.return_value

==> 64.py <==
class MyFinishBreakpoint (gdb.FinishBreakpoint):
def __init__(self, frame):
gdb.FinishBreakpoint.__init__ (self, frame)
print "MyFinishBreakpoint init"

def stop(self):
print "MyFinishBreakpoint stop"
print "return_value is: %d" % int (self.return_value)
return True

def out_of_scope(self):
print "MyFinishBreakpoint out of scope"

=============

and its output:

Temporary breakpoint 1 at 0x4005aa: file 64.c, line 9.

Temporary breakpoint 1, main () at 64.c:9
9 int main (void) { return h (); }
Breakpoint 2 at 0x400580: file 64.c, line 4.

Breakpoint 2, h () at 64.c:4
4 if (setjmp (env))
Num Type Disp Enb Address What
2 breakpoint keep y 0x0000000000400580 in h at 64.c:4
breakpoint already hit 1 time
Temporary breakpoint 3 at 0x4005af: file 64.c, line 9.
MyFinishBreakpoint init
Num Type Disp Enb Address What
2 breakpoint keep y 0x0000000000400580 in h at 64.c:4
breakpoint already hit 1 time
3 breakpoint del y 0x00000000004005af in main at 64.c:9 thread 1
stop only in thread 1
0x000000000040058a in h () at 64.c:4
4 if (setjmp (env))
None
(gdb) _

Existence of the Python FinishBreakpoint causes a stop during longjmp inside h
itself but that should not be caught, the function still has not returned.

> +
> + /* Set frame to NULL for sanity, creating the breakpoint could cause
> + us to switch threads, thus blowing away the frame cache, rendering
> + the frame pointer invalid. */
> + frame = NULL;
> }
> GDB_PY_SET_HANDLE_EXCEPTION (except);
>
> - self_bpfinish->py_bp.bp->frame_id = frame_id;
> + self_bpfinish->py_bp.bp->frame_id = prev_frame_id;
> self_bpfinish->py_bp.is_finish_bp = 1;
> + self_bpfinish->initiating_frame = init_frame_id;
>
> /* Bind the breakpoint with the current program space. */
> self_bpfinish->py_bp.bp->pspace = current_program_space;

Thanks,
Jan