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

Jan Kratochvil
2012-10-17T16:28:03Z

Hello Andrew,

I have found a patch with some goals similar has been already posted here:
http://sourceware.org/ml/gdb-patches/2012-01/msg00267.html
(Never replied.)

But I find the patch below as a more appropriate approach.

On Mon, 15 Oct 2012 22:39:52 +0200, Andrew Burgess wrote:
> On 11/10/2012 5:32 PM, Jan Kratochvil wrote:
> > FYI I did not review original python/py-finishbreakpoint.c but I do not think
> > the whole finish_command logic should have been duplicated. But the original
> > review was very long which I skipped as a Python one so I may miss something.
>
> I'm not sure what to make of this.

Nothing; I have read now the former thread and I understand now that the
intention was to catch function finish even after very arbitrary GDB commands
happen in the meantime. Hooking to finish_command would no longer track the
finish situation in such case.

> 2012-10-15 Andrew Burgess <aburgess@broadcom.com>
>
> * python/py-finishbreakpoint.c (struct finish_breakpoint_object)
> <initiating_frame>: New field.
> (bpfinishpy_post_stop_hook): Disable the longjmp breakpoints when
> we stop at a finish breakpoint. Have the finish breakpoint
> deleted at the next stop, wherever that might be.
> (bpfinishpy_init): Set longjmp breakpoints. Remember frame we're
> in when creating the finish breakpoint.
> (struct bpfinishpy_out_of_scope_data): New structure for passing
> parameters to out of scope callback.
> (bpfinishpy_detect_out_scope_cb): Look for frame we are hoping to
> finish when deciding if we're out of scope, not frame of parent.
> Check we're stopped in correct thread, or that the breakpoint
> thread has exited before we declare a breakpoint out of scope.
> (bpfinishpy_handle_stop): Pass parameter struct to callback.
> (bpfinishpy_handle_exit): Pass parameter struct to callback.
>
> diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
> index 56ab775..02f14c2 100644
> --- a/gdb/python/py-finishbreakpoint.c
> +++ b/gdb/python/py-finishbreakpoint.c
> @@ -53,6 +53,9 @@ struct finish_breakpoint_object
> the function; Py_None if the value is not computable; NULL if GDB is
> not stopped at a FinishBreakpoint. */
> PyObject *return_value;
> + /* The initiating frame for this operation, used to decide when we have
> + left this frame. */
> + struct frame_id initiating_frame;
> };
>
> /* Python function to get the 'return_value' attribute of
> @@ -141,6 +144,10 @@ bpfinishpy_post_stop_hook (struct breakpoint_object *bp_obj)
> /* Can't delete it here, but it will be removed at the next stop. */
> disable_breakpoint (bp_obj->bp);
> gdb_assert (bp_obj->bp->disposition == disp_del);
> + bp_obj->bp->disposition = disp_del_at_next_stop;
> +
> + /* Disable all the longjmp breakpoints too. */
> + delete_longjmp_breakpoint_at_next_stop (bp_obj->bp->thread);
> }
> if (except.reason < 0)
> {
> @@ -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 (), null_frame_id);

I find too intrusive to call set_longjmp_breakpoint here.

A countercase - I did not try to reproduce it in real:

* You have breakpoint installed at TRACEDFUNC and you automatically use
Python finish breakpoint to trace return values of TRACEDFUNC.
* User at CALLERFUNC will type in GDB CLI "finish".
* CALLERFUNC does a lot of processing and it also calls TRACEDFUNC.
* Now you overwide tp->INITIATING_FRAME of the user "finish" command by
null_frame_id which breaks the behavior in some way.

I believe you just want non-stopping notification when the breakpoint went out
of scope. This is exactly what was needed in a recently checked in patch:
commit 95689df3ff480400019264b3e031de0967f3c8f8
Author: Jan Kratochvil <jan.kratochvil@redhat.com>
Date: Mon Jun 18 17:28:33 2012 +0000
Remove stale dummy frames.

You want to install the "longjmp breakpoint" there by
set_longjmp_breakpoint_for_call_dummy. You want to hook there
check_longjmp_breakpoint_for_call_dummy to call bpfinishpy_detect_out_scope_cb
in some way. Currently you do it on stop but that is too late, breakpoint may
may have been for example placed at stack trampoline function (code at the
stack) and the breakpoint instruction now corrupts live stack data there.

Please correct me if I have missed something in your patch.

You may want to rename some GDB symbols from "call_dummy" to something more
generic. If you do so (I do not require it) please make it a separate
rename-only patch.

I will have to do a second review then as it will change the patch a bit.

> +
> + /* 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;
> gdb/testsuite/ChangeLog
>
> 2012-10-15 Andrew Burgess <aburgess@broadcom.com>
>
> Additional testing for FinishBreakpoint.
> * gdb.python/py-finish-breakpoint2.cc: Add extra levels of nesting
> to allow more testing opportunities.
> * gdb.python/py-finish-breakpoint2.exp: Additional test cases.
> * gdb.python/py-finish-breakpoint3.c: New file.
> * gdb.python/py-finish-breakpoint3.exp: New file.
> * gdb.python/py-finish-breakpoint3.py: New file.

Please use some descriptive (I do not mind which specific) short suffix/name
but not numbers (2, 3). Numbers are more difficult to later refer to
/ remember during regressions.

[...]
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint3.c
> @@ -0,0 +1,102 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2012 Free Software Foundation, Inc.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#include <pthread.h>
> +#include <assert.h>
> +#include <unistd.h>
> +
> +static volatile int blocks[2];
> +
> +void
> +breakpt (int is_first_thread)
> +{
> + if (is_first_thread)
> + {
> + /* Main thread. */
> +
> + blocks[0] = 1; /* Set thread 2 going. */
> + while (!blocks[1]); /* Wait for thread 2. */

I used this kind of synchronization to get quickly done a demonstation of the
GDB bug. But such hack is not acceptable for a real testcase code.

Please use proper pthread_* operations (pthread_barrier_wait in this case
I think).

Also testcases should not remain hanging indefinitely if something breaks, in
this case 'alarm (60);' at the start of 'main' should ensure that I think.

> + }
> + else
> + {
> + /* Other thread - Nothing. */
> + }
> +}
> +
[...]
> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint3.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint3.exp
> new file mode 100644
> index 0000000..5a1417a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint3.exp
[...]
> +# This file is part of the GDB testsuite.
> +
> +load_lib gdb-python.exp
> +
> +standard_testfile
> +set pyfile ${srcdir}/${subdir}/${testfile}.py

Two spaces -> one space.

Thanks,
Jan