From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2465 invoked by alias); 29 Feb 2012 19:58:39 -0000 Received: (qmail 2448 invoked by uid 22791); 29 Feb 2012 19:58:37 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_GJ,T_RP_MATCHES_RCVD 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, 29 Feb 2012 19:58:19 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q1TJwJJS018347 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 29 Feb 2012 14:58:19 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q1TJwIR9011457; Wed, 29 Feb 2012 14:58:18 -0500 Message-ID: <4F4E835A.5070102@redhat.com> Date: Wed, 29 Feb 2012 20:07:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.1) Gecko/20120216 Thunderbird/10.0.1 MIME-Version: 1.0 To: Tom Tromey CC: gdb-patches@sourceware.org Subject: Re: RFA: fix PR breakpoints/13776 References: <878vjmswbr.fsf@fleche.redhat.com> In-Reply-To: <878vjmswbr.fsf@fleche.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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-02/txt/msg00738.txt.bz2 On 02/28/2012 05:26 PM, Tom Tromey wrote: > I'd appreciate comments on this patch. > I have no idea whether it is the best way to fix the problem. > > Bug 13776 concerns 'next'ing over an exit. For the trivial: > > #include > int > main (void) > { > exit (0); > } > > We get this behavior: > > (gdb) start > Temporary breakpoint 1, main () at exit0.c:5 > 5 exit (0); > (gdb) next > [Inferior 1 (process 2428) exited normally] > warning: Error removing breakpoint 0 > warning: Error removing breakpoint 0 > warning: Error removing breakpoint 0 > > The bug is that exit_inferior ends up calling delete_longjmp_breakpoint, > which tries to delete the longjmp breakpoints -- but as the inferior is > dead, this fails. > > This patch fixes this problem by moving the breakpoint_init_inferior > call earlier in generic_mourn_inferior. This causes the breakpoints to > be marked as uninserted before they are deleted. > > While doing this I noticed that after the inferior exits, we are left > with a step-resume breakpoint: > > (gdb) maint info b > Num Type Disp Enb Address What > [...] > 0 step resume dstp y 0x00000000004004d2 inf 1 thread 1 > stop only in thread 1 > > The breakpoint.c patch causes this to be removed as well. > > Built and regtested on x86-64 Fedora 16. > > Tom > > 2012-02-28 Tom Tromey > > PR breakpoints/13776: > * target.c (generic_mourn_inferior): Call breakpoint_init_inferior > earlier. > * breakpoint.c (breakpoint_init_inferior): Delete step-resume > breakpoints. > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index db05b97..048cc63 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -3341,6 +3341,10 @@ breakpoint_init_inferior (enum inf_context context) > (gdb) tar rem :9999 # remote Windows gdbserver. > */ > > + case bp_step_resume: > + > + /* Also remove step-resume breakpoints. */ > + > delete_breakpoint (b); > break; > > diff --git a/gdb/target.c b/gdb/target.c > index 1f408f6..65a6c23 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -3583,13 +3583,14 @@ generic_mourn_inferior (void) > ptid = inferior_ptid; > inferior_ptid = null_ptid; > > + breakpoint_init_inferior (inf_exited); > + If you do this before exit_inferior, then the breakpoint_init_inferior change is wrong, because the clear_thread_inferior_resources will touch already released memory. Note with valgrind: main () at stepexit.c:6 6 exit (0); (gdb) n [Inferior 1 (process 11456) exited normally] ==11452== Invalid write of size 4 ==11452== at 0x5B38C2: clear_thread_inferior_resources (thread.c:105) ==11452== by 0x5B3E62: delete_thread_1 (thread.c:294) ==11452== by 0x5B3EC7: delete_thread (thread.c:311) ==11452== by 0x6CDFC0: delete_thread_of_inferior (inferior.c:175) ==11452== by 0x5B3FE8: iterate_over_threads (thread.c:368) ==11452== by 0x6CE1C4: exit_inferior_1 (inferior.c:272) ==11452== by 0x6CE25B: exit_inferior (inferior.c:295) ==11452== by 0x5E542E: generic_mourn_inferior (target.c:3591) ==11452== by 0x484C1B: inf_ptrace_mourn_inferior (inf-ptrace.c:181) ==11452== by 0x491F19: linux_nat_mourn_inferior (linux-nat.c:4227) ==11452== by 0x5E3F89: target_mourn_inferior (target.c:2755) ==11452== by 0x5A3353: handle_inferior_event (infrun.c:3405) ==11452== Address 0x4cba398 is 24 bytes inside a block of size 184 free'd ==11452== at 0x4A0662E: free (vg_replace_malloc.c:366) ==11452== by 0x6DB4F0: xfree (common-utils.c:107) ==11452== by 0x5407EB: delete_breakpoint (breakpoint.c:12666) ==11452== by 0x52FE08: breakpoint_init_inferior (breakpoint.c:3348) ==11452== by 0x5E53BB: generic_mourn_inferior (target.c:3586) ==11452== by 0x484C1B: inf_ptrace_mourn_inferior (inf-ptrace.c:181) ==11452== by 0x491F19: linux_nat_mourn_inferior (linux-nat.c:4227) ==11452== by 0x5E3F89: target_mourn_inferior (target.c:2755) ==11452== by 0x5A3353: handle_inferior_event (infrun.c:3405) ==11452== by 0x5A1CB5: wait_for_inferior (infrun.c:2709) ==11452== by 0x5A0E9B: proceed (infrun.c:2278) ==11452== by 0x599EFB: step_once (infcmd.c:1067) ==11452== (gdb) step-resume breakpoint lifetime isn't the prettiest thing... > if (!ptid_equal (ptid, null_ptid)) > { > int pid = ptid_get_pid (ptid); > exit_inferior (pid); > } > > - breakpoint_init_inferior (inf_exited); > registers_changed (); > > reopen_exec_file (); We can call mark_breakpoints_out before that exit_inferior to only mark the breakpoints uninserted, while leaving the rest of the breakpoint_init_inferior work do be done where it is. Also note how we're careful to not delete step-resume and exception resume breakpoints directly in clear_thread_inferior_resources. static void clear_thread_inferior_resources (struct thread_info *tp) { /* NOTE: this will take care of any left-over step_resume breakpoints, but not any user-specified thread-specific breakpoints. We can not delete the breakpoint straight-off, because the inferior might not be stopped at the moment. */ if (tp->control.step_resume_breakpoint) { tp->control.step_resume_breakpoint->disposition = disp_del_at_next_stop; tp->control.step_resume_breakpoint = NULL; } if (tp->control.exception_resume_breakpoint) { tp->control.exception_resume_breakpoint->disposition = disp_del_at_next_stop; tp->control.exception_resume_breakpoint = NULL; } bpstat_clear (&tp->control.stop_bpstat); do_all_intermediate_continuations_thread (tp, 1); do_all_continuations_thread (tp, 1); delete_longjmp_breakpoint (tp->num); } I think longjmp breakpoints should get the same treatment, though this isn't strictly necessary for the exit case (though it is for pthread_exit on targets without thread exit notifications, like remote). With this, the problem is gone, in both sync and async modes, and we have no step-resume breakpoint left behind. Maybe we should also delete longjmp and exception-resume breakpoints similarly in breakpoint_init_inferior, but since they're always deleted either through a cleanup or a continuation I couldn't find a way to leave them stale. There's probably a way with some path that throws an error before the continuation that deletes the breakpoint is reached. I'm leaving that out for now. WDTY? 2012-02-29 Tom Tromey Pedro Alves PR breakpoints/13776: * breakpoint.c (breakpoint_init_inferior): Delete step-resume breakpoints. (delete_longjmp_breakpoint_at_next_stop): New. * breakpoint.h (delete_longjmp_breakpoint_at_next_stop): Declare. * target.c (generic_mourn_inferior): Call mark_breakpoints_out before deleting the inferior. Add comments. * thread.c (clear_thread_inferior_resources): Don't delete lonjmp breakpoints immediately, but only on next stop. Move that code next to where we mark other breakpoints for deletion. --- gdb/breakpoint.c | 17 +++++++++++++++++ gdb/breakpoint.h | 3 +++ gdb/target.c | 9 +++++++++ gdb/thread.c | 4 ++-- 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index db05b97..f634d97 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -3341,6 +3341,10 @@ breakpoint_init_inferior (enum inf_context context) (gdb) tar rem :9999 # remote Windows gdbserver. */ + case bp_step_resume: + + /* Also remove step-resume breakpoints. */ + delete_breakpoint (b); break; @@ -6625,6 +6629,19 @@ delete_longjmp_breakpoint (int thread) } void +delete_longjmp_breakpoint_at_next_stop (int thread) +{ + struct breakpoint *b, *b_tmp; + + ALL_BREAKPOINTS_SAFE (b, b_tmp) + if (b->type == bp_longjmp || b->type == bp_exception) + { + if (b->thread == thread) + b->disposition = disp_del_at_next_stop; + } +} + +void enable_overlay_breakpoints (void) { struct breakpoint *b; diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h index 7e8c597..5b5172e 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -1254,6 +1254,9 @@ extern void set_longjmp_breakpoint (struct thread_info *tp, struct frame_id frame); extern void delete_longjmp_breakpoint (int thread); +/* Mark all longjmp breakpoints from THREAD for later deletion. */ +extern void delete_longjmp_breakpoint_at_next_stop (int thread); + extern void enable_overlay_breakpoints (void); extern void disable_overlay_breakpoints (void); diff --git a/gdb/target.c b/gdb/target.c index 1f408f6..d29d37a 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -3583,13 +3583,22 @@ generic_mourn_inferior (void) ptid = inferior_ptid; inferior_ptid = null_ptid; + /* Mark breakpoints uninserted in case something tries to delete a + breakpoint while we delete the inferior's threads (which would + fail, since the inferior is long gone). */ + mark_breakpoints_out (); + if (!ptid_equal (ptid, null_ptid)) { int pid = ptid_get_pid (ptid); exit_inferior (pid); } + /* Note this wipes step-resume breakpoints, so needs to be done + after exit_inferior, which ends up referencing the step-resume + breakpoints through clear_thread_inferior_resources. */ breakpoint_init_inferior (inf_exited); + registers_changed (); reopen_exec_file (); diff --git a/gdb/thread.c b/gdb/thread.c index 22d8b23..97f283c 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -113,12 +113,12 @@ clear_thread_inferior_resources (struct thread_info *tp) tp->control.exception_resume_breakpoint = NULL; } + delete_longjmp_breakpoint_at_next_stop (tp->num); + bpstat_clear (&tp->control.stop_bpstat); do_all_intermediate_continuations_thread (tp, 1); do_all_continuations_thread (tp, 1); - - delete_longjmp_breakpoint (tp->num); } static void -- Pedro Alves