Call target_close after unpushing, not before (was: Re: [patch] Fix remote.c crash on gdbserver close (+fix py-finish-breakpoint.exp for gdbserver))

[Pedro Alves <palves@redhat.com>]

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<bpfinishpy_detect_out_scope_cb>, 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  <palves@redhat.com>

	* 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