[Oleg Nesterov] PATCH? gdb remote.c: readchar() should pop_target() if SERIAL_ERROR?

[Oleg Nesterov] PATCH? gdb remote.c: readchar() should pop_target() if SERIAL_ERROR?

[Tom Tromey]

[-- Attachment #1: Type: text/plain, Size: 313 bytes --]

I am forwarding this from the archer list because I think this report is
reasonably complete -- I couldn't really improve on it.

Please comment on this patch.  I don't know this area of gdb well.

If the enclosed has insufficient information, please contact me and I
will get whatever is needed.  Thanks.

Tom



[-- Attachment #2: Type: message/rfc822, Size: 3290 bytes --]

From: Oleg Nesterov <oleg@redhat.com>
To: Tom Tromey <tromey@redhat.com>, Roland McGrath <roland@redhat.com>, "Frank Ch. Eigler" <fche@redhat.com>
Cc: archer@sourceware.org
Subject: PATCH? gdb remote.c: readchar() should pop_target() if SERIAL_ERROR?
Date: Fri, 2 Jul 2010 03:13:02 +0200
Message-ID: <20100702011302.GA24599@redhat.com>

Hello.

I was trying to learn how gdb works with gdbserver and hit the bug.
The patch below seems to fix the problem for me but I am not sure
it is correct, I never looked into the (complex!) gdb sources before.

In short, sometimes gdb doing "target remote :tcp_port" refuses to
exit, putpkt_binary() complains that putpkt failed and longjmp()s
to the main loop. This happens sometimes if gdbserver dies.

	(gdb) q
	A debugging session is active.

		Inferior 1 [Remote target] will be killed.

	Quit anyway? (y or n) y
	putpkt: write failed: Broken pipe.
	(gdb)

Not sure my analysis is correct, but I think that readchar() needs
a fix. A read/recv from a socket doesn't necessarily returns EOF if
the peer has closed the socket. It can return sock->sk_err set by
the previous send if the peer dies and sets sk->sk_shutdown.

The patch merely adds pop_target(). The more sophisticates fix
should probably continue the reading, sock->sk_err was cleared and
the socket may have the packets which we could read.

Oleg.

--- gdb-7.1/gdb/remote.c~	2010-03-07 15:39:53.000000000 +0100
+++ gdb-7.1/gdb/remote.c	2010-07-02 02:38:09.000000000 +0200
@@ -6364,6 +6364,7 @@ readchar (int timeout)
       error (_("Remote connection closed"));
       /* no return */
     case SERIAL_ERROR:
+      pop_target ();
       perror_with_name (_("Remote communication error"));
       /* no return */
     case SERIAL_TIMEOUT:

Re: [Oleg Nesterov] PATCH? gdb remote.c: readchar() should pop_target() if SERIAL_ERROR?

[Michael Snyder]

Tom Tromey wrote:
> I am forwarding this from the archer list because I think this report is
> reasonably complete -- I couldn't really improve on it.
> 
> Please comment on this patch.  I don't know this area of gdb well.
> 
> If the enclosed has insufficient information, please contact me and I
> will get whatever is needed.  Thanks.

I believe I've run into this.
Can we get this on the 7.2 branch as well?

Re: [Oleg Nesterov] PATCH? gdb remote.c: readchar() should pop_target() if SERIAL_ERROR?

[Michael Snyder]

Tom Tromey wrote:
> I am forwarding this from the archer list because I think this report is
> reasonably complete -- I couldn't really improve on it.
> 
> Please comment on this patch.  I don't know this area of gdb well.
> 
> If the enclosed has insufficient information, please contact me and I
> will get whatever is needed.  Thanks.

I'm sorry I haven't been able to recall how I've encountered this 
symptom, so I can't provide a meaningful review.

Is there a chance of getting this into the 7.2 release?

Re: [Oleg Nesterov] PATCH? gdb remote.c: readchar() should pop_target() if SERIAL_ERROR?

[Tom Tromey]

>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> writes:

Michael> Tom Tromey wrote:
>> I am forwarding this from the archer list because I think this report is
>> reasonably complete -- I couldn't really improve on it.
>> 
>> Please comment on this patch.  I don't know this area of gdb well.
>> 
>> If the enclosed has insufficient information, please contact me and I
>> will get whatever is needed.  Thanks.

Michael> I'm sorry I haven't been able to recall how I've encountered this
Michael> symptom, so I can't provide a meaningful review.

Michael> Is there a chance of getting this into the 7.2 release?

The change seems correct to me, and I'm interested in getting it in.
But since I don't know this code well I am reluctant to commit it
without someone else's ok.  If that isn't forthcoming I will try to look
into the code more deeply and see if I can figure it out.  Maybe I can
do some kind of failure injection to see the bug live.

Tom

Re: [Oleg Nesterov] PATCH? gdb remote.c: readchar() should pop_target() if SERIAL_ERROR?

[Pedro Alves]

Oleg Nesterov wrote:

>         (gdb) q
>         A debugging session is active.
> 
>                 Inferior 1 [Remote target] will be killed.
> 
>         Quit anyway? (y or n) y
>         putpkt: write failed: Broken pipe.
>         (gdb)
> 
> Not sure my analysis is correct, but I think that readchar() needs
> a fix. A read/recv from a socket doesn't necessarily returns EOF if
> the peer has closed the socket. 

> It can return sock->sk_err set by
> the previous send if the peer dies and sets sk->sk_shutdown.

Talking in terms of Linux kernel internals, eh?
Do note that you may actually be debugging with something
not tcp, say, a bog dumb serial line, and an error may happen
at any time, not just when quitting/killing the target
(although quitting is more prone to these kinds of issues, since
the 'k' packet doesn't actually require a reply).

> The patch merely adds pop_target(). The more sophisticates fix
> should probably continue the reading, sock->sk_err was cleared and
> the socket may have the packets which we could read.

I guess the idea must have been that the user would just retry
whatever failed.  But of course, that's prone for messing
things up too.  I'm thinking this should be rare enough under normal
conditions, and that it's not worth the hassle to do anything smarter
here.  We have no clue in what state the connection or the target
was left in.  We could claim that it's just better to reconnect
and refresh the whole debug state.

> --- gdb-7.1/gdb/remote.c~       2010-03-07 15:39:53.000000000 +0100
> +++ gdb-7.1/gdb/remote.c        2010-07-02 02:38:09.000000000 +0200
> @@ -6364,6 +6364,7 @@ readchar (int timeout)
>        error (_("Remote connection closed"));
>        /* no return */
>      case SERIAL_ERROR:
> +      pop_target ();
>        perror_with_name (_("Remote communication error"));
>        /* no return */
>      case SERIAL_TIMEOUT:

... thus, I have no problems with this.  Could you tweak the
string to say something like:

 "Remote communication error.  Target disconnected."

so that user is informed we're no longer talking to the
target?.

Okay with that change.

-- 
Pedro Alves

Re: [Oleg Nesterov] PATCH? gdb remote.c: readchar() should pop_target() if SERIAL_ERROR?

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Talking in terms of Linux kernel internals, eh?

Yeah, see the discussion on the archer list...

Pedro> ... thus, I have no problems with this.  Could you tweak the
Pedro> string to say something like:
Pedro>  "Remote communication error.  Target disconnected."
Pedro> so that user is informed we're no longer talking to the
Pedro> target?.

Pedro> Okay with that change.

Thank you.  Here is the patch I am committing.
I'm putting it in 7.2 as well.

Tom

2010-07-28  Oleg Nesterov  <oleg@redhat.com>

	* remote.c (readchar): Call pop_target in case of SERIAL_ERROR.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.420
diff -u -r1.420 remote.c
--- remote.c	28 Jul 2010 18:04:19 -0000	1.420
+++ remote.c	28 Jul 2010 20:19:02 -0000
@@ -6667,7 +6667,8 @@
       error (_("Remote connection closed"));
       /* no return */
     case SERIAL_ERROR:
-      perror_with_name (_("Remote communication error"));
+      pop_target ();
+      perror_with_name (_("Remote communication error.  Target disconnected."));
       /* no return */
     case SERIAL_TIMEOUT:
       break;