From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5733 invoked by alias); 19 Jan 2012 16:03:00 -0000 Received: (qmail 5557 invoked by uid 22791); 19 Jan 2012 16:02:57 -0000 X-SWARE-Spam-Status: No, hits=-6.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,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; Thu, 19 Jan 2012 16:02:35 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q0JG2XHa031886 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 19 Jan 2012 11:02:33 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q0JG2VQO025956; Thu, 19 Jan 2012 11:02:32 -0500 Message-ID: <4F183E97.4010704@redhat.com> Date: Thu, 19 Jan 2012 16:24:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20111222 Thunderbird/9.0 MIME-Version: 1.0 To: Jan Kratochvil CC: gdb-patches@sourceware.org, Tom Tromey , Kevin Pouget Subject: Call target_close after unpushing, not before (was: Re: [patch] Fix remote.c crash on gdbserver close (+fix py-finish-breakpoint.exp for gdbserver)) References: <20111225113745.GA16273@host2.jankratochvil.net> <20111227232323.GA6956@host2.jankratochvil.net> <4F020A20.2020801@gmail.com> In-Reply-To: <4F020A20.2020801@gmail.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-01/txt/msg00702.txt.bz2 On 01/02/2012 07:48 PM, Pedro Alves wrote: > On 12/27/2011 11:23 PM, Jan Kratochvil wrote: >> [rediff on top of HEAD] >> >> On Sun, 25 Dec 2011 12:37:45 +0100, Jan Kratochvil wrote: >> Hi, >> >> with gdbserver as from >> http://sourceware.org/gdb/wiki/TestingGDB#Testing_gdbserver_in_a_native_configuration >> >> it will crash: >> >> (gdb) PASS: gdb.python/py-finish-breakpoint.exp: set FinishBP after the exit() >> continue >> Continuing. >> Child exited with status 0 >> GDBserver exiting >> [Inferior 1 (Remote target) exited normally] >> SimpleFinishBreakpoint out of scope >> ERROR: Process no longer exists >> Program terminated with signal 11, Segmentation fault. >> #0 0x00000000007d48b9 in serial_debug_p (scb=0x0) at serial.c:584 >> 584 return scb->debug_p || global_serial_debug_p; >> (gdb) p scb >> $1 = (struct serial *) 0x0 >> (gdb) bt >> #0 in serial_debug_p (scb=0x0) at serial.c:584 >> #1 in serial_write (scb=0x0, str=0x7fff727e3300 "$z0,4008a3,1#93", len=15) at serial.c:427 >> #2 in putpkt_binary (buf=0x2a279b0 "z0,4008a3,1", cnt=11) at remote.c:6891 >> #3 in putpkt (buf=0x2a279b0 "z0,4008a3,1") at remote.c:6823 >> #4 in remote_remove_breakpoint (gdbarch=0x28e10f0, bp_tgt=0x2fb3d38) at remote.c:7749 >> #5 in target_remove_breakpoint (gdbarch=0x28e10f0, bp_tgt=0x2fb3d38) at target.c:2422 >> #6 in bkpt_remove_location (bl=0x2fb3cd0) at breakpoint.c:10967 >> #7 in remove_breakpoint_1 (bl=0x2fb3cd0, is=mark_uninserted) at breakpoint.c:2654 >> #8 in remove_breakpoint (bl=0x2fb3cd0, is=mark_uninserted) at breakpoint.c:2760 >> #9 in update_global_location_list (should_insert=0) at breakpoint.c:10539 >> #10 in delete_breakpoint (bpt=0x2f59290) at breakpoint.c:11392 >> #11 in bpfinishpy_out_of_scope (bpfinish_obj=0x7fdf3c9f8130) at ./python/py-finishbreakpoint.c:327 >> #12 in bpfinishpy_detect_out_scope_cb (b=0x2f59290, args=0x0) at ./python/py-finishbreakpoint.c:356 >> #13 in iterate_over_breakpoints (callback=0x65150d, data=0x0) at breakpoint.c:13385 >> #14 in bpfinishpy_handle_exit (inf=0x281d330) at ./python/py-finishbreakpoint.c:393 >> #15 in observer_inferior_exit_notification_stub (data=0x6516b1, args_data=0x7fff727e3740) at observer.inc:887 >> #16 in generic_observer_notify (subject=0x284f300, args=0x7fff727e3740) at observer.c:168 >> #17 in observer_notify_inferior_exit (inf=0x281d330) at observer.inc:912 >> #18 in exit_inferior_1 (inftoex=0x281d330, silent=1) at inferior.c:276 >> #19 in exit_inferior_silent (pid=42000) at inferior.c:305 >> #20 in discard_all_inferiors () at inferior.c:343 >> #21 in remote_close (quitting=0) at remote.c:2950 >> #22 in target_close (targ=0x1d19f80, quitting=0) at target.c:3387 >> #23 in unpush_target (t=0x1d19f80) at target.c:1024 >> #24 in remote_mourn_1 (target=0x1d19f80) at remote.c:7456 >> #25 in remote_mourn (ops=0x1d19f80) at remote.c:7449 >> #26 in target_mourn_inferior () at target.c:2747 >> #27 in handle_inferior_event (ecs=0x7fff727e3c30) at infrun.c:3408 >> #28 in wait_for_inferior () at infrun.c:2711 >> #29 in proceed (addr=18446744073709551615, siggnal=TARGET_SIGNAL_DEFAULT, step=0) at infrun.c:2276 >> #30 in continue_1 (all_threads=0) at infcmd.c:713 >> #31 in continue_command (args=0x0, from_tty=1) at infcmd.c:805 >> >> >> Reproducible with: >> ../gdbserver/gdbserver :1234 gdb.python/py-finish-breakpoint >> gdb -nx -x ~/.gdbinit -ex r --args ../gdb -nx -ex 'file gdb.python/py-finish-breakpoint' -ex 'target remote localhost:1234' -ex 'b test_exec_exit' -ex c -ex 'source gdb.python/py-finish-breakpoint.py' -ex 'python SimpleFinishBreakpoint(gdb.newest_frame())' -ex c >> >> I have seen this serial_debug_p crash in various GDB debugging cases but they >> were not well reproducible. I understand this bug is unrelated to >> gdb.python/py-finish-breakpoint.exp . >> >> I also tried to reorder remote_close a bit but this patch seems as the right >> one to me. > > The issue is that the target is already gone, and we're going through > teardown. What I think would be a bit better is to add a new function > to breakpoint.c, similar to mark_breakpoints_out, like: > > void > mark_all_breakpoints_out (void) > { > struct bp_location *bl, **blp_tmp; > > ALL_BP_LOCATIONS (bl, blp_tmp) > bl->inserted = 0; > } > > and call that from within remote_close, before discard_all_inferiors. > > remote_desc == NULL checks throughout remote.c are plain hacks. I tried this, and it works. However, > > Even better would be to do discard_all_inferiors after target_close, > though that's tricky. Perhaps it'd work to do it only if the target > just closed is of process_stratum. I think there's even a better way. Any target method call through the target vector from within target_close, while we're handling teardown, is likely to end up being broken. So I thought that it's better to consider that by the time target_close is called, the target is already not useable, and removed on the stack. If the target does want to call some other of its own target methods, it should call it directly, instead of through the target vector. I audited all target_close implementations, and only linux-nat.c needed such adjustment. This fixes the gdbserver crash. Also tested w/ native. Unfortunately, the py-finish-breakpoint.exp triggers internal errors in async mode, but that's unrelated. Any comments on this? gdb/ 2012-01-19 Pedro Alves * linux-nat.c (linux_nat_close): Call linux_nat_is_async_p and linux_nat_async directly instead of going through the target vector. * target.c (unpush_target): Close target after unpushing it, not before. --- gdb/linux-nat.c | 4 ++-- gdb/target.c | 17 ++++++++--------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c index f6b36a2..30f9062 100644 --- a/gdb/linux-nat.c +++ b/gdb/linux-nat.c @@ -5717,8 +5717,8 @@ static void linux_nat_close (int quitting) { /* Unregister from the event loop. */ - if (target_is_async_p ()) - target_async (NULL, 0); + if (linux_nat_is_async_p ()) + linux_nat_async (NULL, 0); if (linux_ops->to_close) linux_ops->to_close (quitting); diff --git a/gdb/target.c b/gdb/target.c index 9aaa0ea..6af4620 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -1010,16 +1010,10 @@ unpush_target (struct target_ops *t) break; } + /* If we don't find target_ops, quit. Only open targets should be + closed. */ if ((*cur) == NULL) - return 0; /* Didn't find target_ops, quit now. */ - - /* NOTE: cagney/2003-12-06: In '94 the close call was made - unconditional by moving it to before the above check that the - target was in the target stack (something about "Change the way - pushing and popping of targets work to support target overlays - and inheritance"). This doesn't make much sense - only open - targets should be closed. */ - target_close (t, 0); + return 0; /* Unchain the target. */ tmp = (*cur); @@ -1028,6 +1022,11 @@ unpush_target (struct target_ops *t) update_current_target (); + /* Finally close the target. Note we do this after unchaining, so + any target method calls from within the target_close + implementation don't end up in T anymore. */ + target_close (t, 0); + return 1; } -- Pedro Alves