[patch] Fix remote.c incorrectly using pop_target (wrt btrace)

[patch] Fix remote.c incorrectly using pop_target (wrt btrace)

[Jan Kratochvil]

Hi,

with the btrace patchset checked in one may get stuck when using btrace and
gdbserver:

./gdbserver :1234 true
./gdb true -ex 'target remote localhost:1234' -ex 'set debug remote 1' -ex 'record btrace'
(gdb) stepi
Sending packet: $qTStatus#49...qTStatus: Remote connection closed
Sending packet: $Z0,7ffff7debd10,1#09...Sending packet: $QPassSignals:e;10;14;17;1a;1b;1c;21;24;25;2c;4c;#5f...0x00007ffff7ddd420 in _start () from /lib64/ld-linux-x86-64.so.2
putpkt: write failed: Broken pipe.
(gdb) q
A debugging session is active.
        Inferior 1 [process 26817] will be killed.
Quit anyway? (y or n) y
Sending packet: $qTStatus#49...putpkt: write failed: Broken pipe.
(gdb) _

as discussed in:
	RE: Crash of GDB with gdbserver btrace enabled [Re: [patch v9 00/23] branch tracing support for Atom]
	http://sourceware.org/ml/gdb-patches/2013-03/msg00296.html
	Message-ID: <A78C989F6D9628469189715575E55B2307B9C2F6@IRSMSX102.ger.corp.intel.com>
	From: "Metzger, Markus T" <markus.t.metzger@intel.com>

with the fix below:

(gdb) stepi
Sending packet: $qTStatus#49...qTStatus: You can't do that when your target is `record-btrace'
PC register is not available
(gdb) stepi
The program is not being run.
(gdb) q
$ _

Those two removed pop_target calls around
	target_preopen (from_tty);
	unpush_target (target);

were redundant as they were added by:
	commit ef378e83937c25e9da9c4e545bb1a8bb5fd767f1
	Author: Daniel Jacobowitz <dan@debian.org>
	Date:   Wed Jan 30 00:51:50 2008 +0000

but later target_preopen started removing all the targets in target_preopen by:
	commit 3db54b199473ba136a4821c420f85096ff17e98e
	Author: Pedro Alves <pedro@codesourcery.com>
	Date:   Mon Aug 18 23:12:39 2008 +0000

No regressions on {x86_64,x86_64-m32,i686}-fedora19pre-linux-gnu and with
gdbserver.

I would like to get it checked in for 7.6 is it is some sort of regression
from btrace.


Thanks,
Jan


gdb/
2013-03-11  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* remote.c (remote_unpush_target): New function.
	(remote_open_1): Remove two pop_target calls, update one comment, add
	comment to target_preopen call.  Replace pop_target call by
	remote_unpush_target call.
	(interrupt_query, readchar, getpkt_or_notif_sane_1): Replace
	pop_target calls by remote_unpush_target calls.

diff --git a/gdb/remote.c b/gdb/remote.c
index 8fc6b85..6e6c0d6 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4188,6 +4188,14 @@ remote_query_supported (void)
       }
 }
 
+/* Remove any of the remote.c targets from target stack.  */
+
+static void
+remote_unpush_target (void)
+{
+  unpush_target (&remote_ops);
+  unpush_target (&extended_remote_ops);
+}
 
 static void
 remote_open_1 (char *name, int from_tty,
@@ -4205,30 +4213,18 @@ remote_open_1 (char *name, int from_tty,
     wait_forever_enabled_p = 1;
 
   /* If we're connected to a running target, target_preopen will kill it.
-     But if we're connected to a target system with no running process,
-     then we will still be connected when it returns.  Ask this question
-     first, before target_preopen has a chance to kill anything.  */
+     Ask this question first, before target_preopen has a chance to kill
+     anything.  */
   if (remote_desc != NULL && !have_inferiors ())
     {
-      if (!from_tty
-	  || query (_("Already connected to a remote target.  Disconnect? ")))
-	pop_target ();
-      else
+      if (from_tty
+	  && !query (_("Already connected to a remote target.  Disconnect? ")))
 	error (_("Still connected."));
     }
 
+  /* Here the possibly existing remote target gets unpushed.  */
   target_preopen (from_tty);
 
-  unpush_target (target);
-
-  /* This time without a query.  If we were connected to an
-     extended-remote target and target_preopen killed the running
-     process, we may still be connected.  If we are starting "target
-     remote" now, the extended-remote target will not have been
-     removed by unpush_target.  */
-  if (remote_desc != NULL && !have_inferiors ())
-    pop_target ();
-
   /* Make sure we send the passed signals list the next time we resume.  */
   xfree (last_pass_packet);
   last_pass_packet = NULL;
@@ -4348,7 +4344,7 @@ remote_open_1 (char *name, int from_tty,
 	/* Pop the partially set up target - unless something else did
 	   already before throwing the exception.  */
 	if (remote_desc != NULL)
-	  pop_target ();
+	  remote_unpush_target ();
 	if (target_async_permitted)
 	  wait_forever_enabled_p = 1;
 	throw_exception (ex);
@@ -5096,7 +5092,7 @@ interrupt_query (void)
       if (query (_("Interrupted while waiting for the program.\n\
 Give up (and stop debugging it)? ")))
 	{
-	  pop_target ();
+	  remote_unpush_target ();
 	  deprecated_throw_reason (RETURN_QUIT);
 	}
     }
@@ -7051,11 +7047,11 @@ readchar (int timeout)
   switch ((enum serial_rc) ch)
     {
     case SERIAL_EOF:
-      pop_target ();
+      remote_unpush_target ();
       error (_("Remote connection closed"));
       /* no return */
     case SERIAL_ERROR:
-      pop_target ();
+      remote_unpush_target ();
       perror_with_name (_("Remote communication error.  "
 			  "Target disconnected."));
       /* no return */
@@ -7579,7 +7575,7 @@ getpkt_or_notif_sane_1 (char **buf, long *sizeof_buf, int forever,
 	      if (forever)	/* Watchdog went off?  Kill the target.  */
 		{
 		  QUIT;
-		  pop_target ();
+		  remote_unpush_target ();
 		  error (_("Watchdog timeout has expired.  Target detached."));
 		}
 	      if (remote_debug)

Re: [patch] Fix remote.c incorrectly using pop_target (wrt btrace)

[Joel Brobecker]

> I would like to get it checked in for 7.6 is it is some sort of regression
> from btrace.

Agreed on getting this fixed before 7.6 gets released. I propose
we still go ahead with the branch tomorrow, and just port the fix
to the branch as soon as approved.

> gdb/
> 2013-03-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* remote.c (remote_unpush_target): New function.
> 	(remote_open_1): Remove two pop_target calls, update one comment, add
> 	comment to target_preopen call.  Replace pop_target call by
> 	remote_unpush_target call.
> 	(interrupt_query, readchar, getpkt_or_notif_sane_1): Replace
> 	pop_target calls by remote_unpush_target calls.
-- 
Joel

Re: [patch] Fix remote.c incorrectly using pop_target (wrt btrace)

[Yao Qi]

On 03/12/2013 01:28 AM, Jan Kratochvil wrote:
> @@ -4348,7 +4344,7 @@ remote_open_1 (char *name, int from_tty,
>   	/* Pop the partially set up target - unless something else did
>   	   already before throwing the exception.  */
>   	if (remote_desc != NULL)
> -	  pop_target ();
> +	  remote_unpush_target ();


Since it is in remote_open_1, the remote target or exteneded-remote 
target is just pushed and top most, so pop_target should be fine here. 
It is not necessary to change it to remote_unpush_target.

>   	if (target_async_permitted)
>   	  wait_forever_enabled_p = 1;
>   	throw_exception (ex);
> @@ -5096,7 +5092,7 @@ interrupt_query (void)
>         if (query (_("Interrupted while waiting for the program.\n\
>   Give up (and stop debugging it)? ")))
>   	{
> -	  pop_target ();
> +	  remote_unpush_target ();
>   	  deprecated_throw_reason (RETURN_QUIT);
>   	}
>       }
> @@ -7051,11 +7047,11 @@ readchar (int timeout)
>     switch ((enum serial_rc) ch)
>       {
>       case SERIAL_EOF:
> -      pop_target ();
> +      remote_unpush_target ();

Supposing we are in 'record-btrace' target and get a communication 
error, the current target stack looks like:

   record-btrace
   remote
   exec
   none

remote_unpush_target will unpush or remove the remote target in the 
stack.  So the stack is changed to:

   record-btrace
   exec
   none

Is it what we want?  When GDB is in record-btrace target, and get some 
errors in communication, both record-btrace and remote target should be 
popped from the stack.

-- 
Yao (齐尧)

Re: [patch] Fix remote.c incorrectly using pop_target (wrt btrace)

[Jan Kratochvil]

On Tue, 12 Mar 2013 11:03:45 +0100, Yao Qi wrote:
> On 03/12/2013 01:28 AM, Jan Kratochvil wrote:
> >@@ -4348,7 +4344,7 @@ remote_open_1 (char *name, int from_tty,
> >  	/* Pop the partially set up target - unless something else did
> >  	   already before throwing the exception.  */
> >  	if (remote_desc != NULL)
> >-	  pop_target ();
> >+	  remote_unpush_target ();
> 
> Since it is in remote_open_1, the remote target or exteneded-remote
> target is just pushed and top most, so pop_target should be fine
> here. It is not necessary to change it to remote_unpush_target.

OK; but one should remove pop_target later, there remain only few uses of it.
When its use is not incorrect it is at least fragile/dangerous.


> Is it what we want?  When GDB is in record-btrace target, and get
> some errors in communication, both record-btrace and remote target
> should be popped from the stack.

It causes on killed gdbserver:

(gdb) stepi
You can't do that when your target is `record-btrace'
(gdb) _

As btrace target then no longer has methods it expects underneath.

So I have changed remote_unpush_target below.

Testing of this patch generally requires:
	[patch+7.6] Fix 7.5 regression crashing GDB if gdbserver dies
	http://sourceware.org/ml/gdb-patches/2013-03/msg00691.html
	Message-ID: <20130315195359.GA19841@host2.jankratochvil.net>

No regressions on {x86_64,x86_64-m32,i686}-fedora19pre-linux-gnu and with
gdbserver.


Thanks,
Jan


gdb/
2013-03-15  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* remote.c (remote_unpush_target): New function.
	(remote_open_1): Remove two pop_target calls, update one comment, add
	comment to target_preopen call.  Replace pop_target call by
	remote_unpush_target call.
	(interrupt_query, readchar, getpkt_or_notif_sane_1): Replace
	pop_target calls by remote_unpush_target calls.

diff --git a/gdb/remote.c b/gdb/remote.c
index 21d86f7..207180d 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4188,6 +4188,14 @@ remote_query_supported (void)
       }
 }
 
+/* Remove any of the remote.c targets from target stack.  Upper targets depend
+   on it so remove them first.  */
+
+static void
+remote_unpush_target (void)
+{
+  pop_all_targets_above (process_stratum - 1, 0);
+}
 
 static void
 remote_open_1 (char *name, int from_tty,
@@ -4205,30 +4213,18 @@ remote_open_1 (char *name, int from_tty,
     wait_forever_enabled_p = 1;
 
   /* If we're connected to a running target, target_preopen will kill it.
-     But if we're connected to a target system with no running process,
-     then we will still be connected when it returns.  Ask this question
-     first, before target_preopen has a chance to kill anything.  */
+     Ask this question first, before target_preopen has a chance to kill
+     anything.  */
   if (remote_desc != NULL && !have_inferiors ())
     {
-      if (!from_tty
-	  || query (_("Already connected to a remote target.  Disconnect? ")))
-	pop_target ();
-      else
+      if (from_tty
+	  && !query (_("Already connected to a remote target.  Disconnect? ")))
 	error (_("Still connected."));
     }
 
+  /* Here the possibly existing remote target gets unpushed.  */
   target_preopen (from_tty);
 
-  unpush_target (target);
-
-  /* This time without a query.  If we were connected to an
-     extended-remote target and target_preopen killed the running
-     process, we may still be connected.  If we are starting "target
-     remote" now, the extended-remote target will not have been
-     removed by unpush_target.  */
-  if (remote_desc != NULL && !have_inferiors ())
-    pop_target ();
-
   /* Make sure we send the passed signals list the next time we resume.  */
   xfree (last_pass_packet);
   last_pass_packet = NULL;
@@ -4348,7 +4344,7 @@ remote_open_1 (char *name, int from_tty,
 	/* Pop the partially set up target - unless something else did
 	   already before throwing the exception.  */
 	if (remote_desc != NULL)
-	  pop_target ();
+	  remote_unpush_target ();
 	if (target_async_permitted)
 	  wait_forever_enabled_p = 1;
 	throw_exception (ex);
@@ -5096,7 +5092,7 @@ interrupt_query (void)
       if (query (_("Interrupted while waiting for the program.\n\
 Give up (and stop debugging it)? ")))
 	{
-	  pop_target ();
+	  remote_unpush_target ();
 	  deprecated_throw_reason (RETURN_QUIT);
 	}
     }
@@ -7051,11 +7047,11 @@ readchar (int timeout)
   switch ((enum serial_rc) ch)
     {
     case SERIAL_EOF:
-      pop_target ();
+      remote_unpush_target ();
       error (_("Remote connection closed"));
       /* no return */
     case SERIAL_ERROR:
-      pop_target ();
+      remote_unpush_target ();
       perror_with_name (_("Remote communication error.  "
 			  "Target disconnected."));
       /* no return */
@@ -7579,7 +7575,7 @@ getpkt_or_notif_sane_1 (char **buf, long *sizeof_buf, int forever,
 	      if (forever)	/* Watchdog went off?  Kill the target.  */
 		{
 		  QUIT;
-		  pop_target ();
+		  remote_unpush_target ();
 		  error (_("Watchdog timeout has expired.  Target detached."));
 		}
 	      if (remote_debug)

Re: [patch] Fix remote.c incorrectly using pop_target (wrt btrace)

[Yao Qi]

On 03/16/2013 03:54 AM, Jan Kratochvil wrote:
>> Since it is in remote_open_1, the remote target or exteneded-remote
>> >target is just pushed and top most, so pop_target should be fine
>> >here. It is not necessary to change it to remote_unpush_target.
> OK; but one should remove pop_target later, there remain only few uses of it.
> When its use is not incorrect it is at least fragile/dangerous.
>

Jan,
I can imagine that using pop_target is fragile in general, but is it 
fragile/dangerous to use pop_target in remote_open_1?  IMO, pop_target 
naturally fits the needs there (push target on stack -> setting up -> 
pop target out of stack on error).  pop_target is also used in 
tracepoint.c:tfile_open, which is similar to this case.

The rest of this patch looks right to me.

-- 
Yao (齐尧)

Re: [patch] Fix remote.c incorrectly using pop_target (wrt btrace)

[Pedro Alves]

On 03/15/2013 07:54 PM, Jan Kratochvil wrote:

> gdb/
> 2013-03-15  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* remote.c (remote_unpush_target): New function.
> 	(remote_open_1): Remove two pop_target calls, update one comment, add
> 	comment to target_preopen call.  Replace pop_target call by
> 	remote_unpush_target call.
> 	(interrupt_query, readchar, getpkt_or_notif_sane_1): Replace
> 	pop_target calls by remote_unpush_target calls.
> 

Thanks.  I think this is fine.

I notice that some targets, like spu-multiarch.c or dec-thread.c
don't seem to be very prepared to be brute-force deactivated.  E.g.,
those targets (and linux-thread-db.c and bsd-uthread.c too), probably
more) are deactivated when symbols or shared libraries are unloaded,
and their to_close methods don't take care of cleaning up the target's
variables / local state.  A similar scenario that doesn't
involve remote, is when going from 'threaded core debugging'
  -> 'non-threaded core debugging'.  But since target_pre_inferior
discards the DSOs of the current inferior, and these targets are
generally not multi-inferior aware, I guess things end up alright.
This is very much a preexisting wart.  I suspect we'll end up
getting back to this at some not-so-distant point.  :-)

-- 
Pedro Alves

[commit+7.6] [patch] Fix remote.c incorrectly using pop_target (wrt btrace)

[Jan Kratochvil]

On Fri, 22 Mar 2013 17:58:01 +0100, Pedro Alves wrote:
> On 03/15/2013 07:54 PM, Jan Kratochvil wrote:
> 
> > gdb/
> > 2013-03-15  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* remote.c (remote_unpush_target): New function.
> > 	(remote_open_1): Remove two pop_target calls, update one comment, add
> > 	comment to target_preopen call.  Replace pop_target call by
> > 	remote_unpush_target call.
> > 	(interrupt_query, readchar, getpkt_or_notif_sane_1): Replace
> > 	pop_target calls by remote_unpush_target calls.
> > 
> 
> Thanks.  I think this is fine.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2013-03/msg00193.html
and for 7.6.
	http://sourceware.org/ml/gdb-cvs/2013-03/msg00194.html
	(This one has additional ", 0" for the QUITTING parameter.)


> I notice that some targets, like spu-multiarch.c or dec-thread.c
> don't seem to be very prepared to be brute-force deactivated.  E.g.,
> those targets (and linux-thread-db.c and bsd-uthread.c too), probably
> more) are deactivated when symbols or shared libraries are unloaded,
> and their to_close methods don't take care of cleaning up the target's
> variables / local state.  A similar scenario that doesn't
> involve remote, is when going from 'threaded core debugging'
>   -> 'non-threaded core debugging'.  But since target_pre_inferior
> discards the DSOs of the current inferior, and these targets are
> generally not multi-inferior aware, I guess things end up alright.
> This is very much a preexisting wart.  I suspect we'll end up
> getting back to this at some not-so-distant point.  :-)

OK, I did not notice before.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2013-03/msg00193.html

--- src/gdb/ChangeLog	2013/03/22 14:52:26	1.15301
+++ src/gdb/ChangeLog	2013/03/22 19:07:03	1.15302
@@ -1,3 +1,12 @@
+2013-03-22  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* remote.c (remote_unpush_target): New function.
+	(remote_open_1): Remove two pop_target calls, update one comment, add
+	comment to target_preopen call.  Replace pop_target call by
+	remote_unpush_target call.
+	(interrupt_query, readchar, getpkt_or_notif_sane_1): Replace
+	pop_target calls by remote_unpush_target calls.
+
 2013-03-22  Pedro Alves  <palves@redhat.com>
 
 	* linux-nat.c (linux_child_follow_fork): Don't call
--- src/gdb/remote.c	2013/03/20 15:46:24	1.530
+++ src/gdb/remote.c	2013/03/22 19:07:03	1.531
@@ -4188,6 +4188,14 @@
       }
 }
 
+/* Remove any of the remote.c targets from target stack.  Upper targets depend
+   on it so remove them first.  */
+
+static void
+remote_unpush_target (void)
+{
+  pop_all_targets_above (process_stratum - 1);
+}
 
 static void
 remote_open_1 (char *name, int from_tty,
@@ -4205,30 +4213,18 @@
     wait_forever_enabled_p = 1;
 
   /* If we're connected to a running target, target_preopen will kill it.
-     But if we're connected to a target system with no running process,
-     then we will still be connected when it returns.  Ask this question
-     first, before target_preopen has a chance to kill anything.  */
+     Ask this question first, before target_preopen has a chance to kill
+     anything.  */
   if (remote_desc != NULL && !have_inferiors ())
     {
-      if (!from_tty
-	  || query (_("Already connected to a remote target.  Disconnect? ")))
-	pop_target ();
-      else
+      if (from_tty
+	  && !query (_("Already connected to a remote target.  Disconnect? ")))
 	error (_("Still connected."));
     }
 
+  /* Here the possibly existing remote target gets unpushed.  */
   target_preopen (from_tty);
 
-  unpush_target (target);
-
-  /* This time without a query.  If we were connected to an
-     extended-remote target and target_preopen killed the running
-     process, we may still be connected.  If we are starting "target
-     remote" now, the extended-remote target will not have been
-     removed by unpush_target.  */
-  if (remote_desc != NULL && !have_inferiors ())
-    pop_target ();
-
   /* Make sure we send the passed signals list the next time we resume.  */
   xfree (last_pass_packet);
   last_pass_packet = NULL;
@@ -4348,7 +4344,7 @@
 	/* Pop the partially set up target - unless something else did
 	   already before throwing the exception.  */
 	if (remote_desc != NULL)
-	  pop_target ();
+	  remote_unpush_target ();
 	if (target_async_permitted)
 	  wait_forever_enabled_p = 1;
 	throw_exception (ex);
@@ -5096,7 +5092,7 @@
       if (query (_("Interrupted while waiting for the program.\n\
 Give up (and stop debugging it)? ")))
 	{
-	  pop_target ();
+	  remote_unpush_target ();
 	  deprecated_throw_reason (RETURN_QUIT);
 	}
     }
@@ -7051,11 +7047,11 @@
   switch ((enum serial_rc) ch)
     {
     case SERIAL_EOF:
-      pop_target ();
+      remote_unpush_target ();
       error (_("Remote connection closed"));
       /* no return */
     case SERIAL_ERROR:
-      pop_target ();
+      remote_unpush_target ();
       perror_with_name (_("Remote communication error.  "
 			  "Target disconnected."));
       /* no return */
@@ -7579,7 +7575,7 @@
 	      if (forever)	/* Watchdog went off?  Kill the target.  */
 		{
 		  QUIT;
-		  pop_target ();
+		  remote_unpush_target ();
 		  error (_("Watchdog timeout has expired.  Target detached."));
 		}
 	      if (remote_debug)