From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7966 invoked by alias); 17 Oct 2012 16:28:03 -0000 Received: (qmail 7950 invoked by uid 22791); 17 Oct 2012 16:28:01 -0000 X-SWARE-Spam-Status: No, hits=-6.4 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_BP,TW_GJ,TW_HP X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 17 Oct 2012 16:27:55 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q9HGRsKw008979 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 17 Oct 2012 12:27:54 -0400 Received: from host2.jankratochvil.net (ovpn-116-24.ams2.redhat.com [10.36.116.24]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q9HGRm2X005866 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 17 Oct 2012 12:27:51 -0400 Date: Wed, 17 Oct 2012 16:28:00 -0000 From: Jan Kratochvil To: Andrew Burgess Cc: "gdb-patches@sourceware.org" Subject: Re: [PATCH] improve python finish breakpoint for exceptions/longjmp case. Message-ID: <20121017162748.GA6546@host2.jankratochvil.net> References: <505C805A.1050400@broadcom.com> <20121011163241.GA9620@host2.jankratochvil.net> <507C7498.7040001@broadcom.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <507C7498.7040001@broadcom.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-10/txt/msg00279.txt.bz2 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 > > * python/py-finishbreakpoint.c (struct finish_breakpoint_object) > : 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 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 > > 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 . > +*/ > + > +#include > +#include > +#include > + > +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