remote protocol patch

remote protocol patch

[Zoltán Filyó]

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


Hi,

I found that the serial remote protocol runs into a deadlock when the serial line is noisy. Below a gdb log fragment with original code:

r +$S05#b8
w +$g#67
r <Timeout: 3 seconds>
w $g#67
r <Timeout: 3 seconds>
w $g#67
r <Timeout: 3 seconds>
w $g#67
r <Timeout: 3 seconds><Timeout: 3 seconds>
w -
r <Timeout: 3 seconds>
w -
r <Timeout: 3 seconds>
w -+

infinitely. Because the target side NetBSD kernel throws all characters until get a new packet start ('$') char.

The patch separates "acknowledge timeout" and "packet retry" cases.
After patching the GDB behaviour changed. The log below generated by patched GDB:

r +$S05#b8
w +$g#67
r <Timeout: 3 seconds><Timeout: 3 seconds><Timeout: 3 seconds>
w $g#67
r <Timeout: 3 seconds><Timeout: 3 seconds><Timeout: 3 seconds>
w $g#67
r

and so on. GDB stay good state after in/out packet loss.

I tried on the GDB 6.5 version under SuSE 10.2 distribution.

Thank you your work,
Zoltán Filyó
zoltan dot filyo at ericsson dot com


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: remote-protocol.diff --]
[-- Type: text/x-patch; name="remote-protocol.diff", Size: 2999 bytes --]

--- remote.c.org	2008-01-10 11:51:10.000000000 +0100
+++ remote.c	2008-01-10 15:16:22.000000000 +0100
@@ -3809,7 +3809,10 @@
   char *buf2 = alloca (cnt + 6);
 
   int ch;
-  int tcount = 0;
+  static const int max_ack_read_count = 3;
+  int ack_timeout_count = 0;
+  static const int max_packet_retry_count = 1000; /* XXX Is this value acceptable? */ 
+  int packet_trying_count = 0;
   char *p;
 
   /* Copy the packet into buffer BUF2, encapsulating it
@@ -3829,7 +3832,7 @@
 
   /* Send it over and over until we get a positive ack.  */
 
-  while (1)
+  for (packet_trying_count = 0; packet_trying_count < max_packet_retry_count; ++packet_trying_count)
     {
       int started_error_output = 0;
 
@@ -3845,7 +3848,7 @@
 	perror_with_name (_("putpkt: write failed"));
 
       /* Read until either a timeout occurs (-2) or '+' is read.  */
-      while (1)
+      for (ack_timeout_count = 0; ack_timeout_count < max_ack_read_count; ++ack_timeout_count)
 	{
 	  ch = readchar (remote_timeout);
 
@@ -3870,15 +3873,13 @@
 	    case '+':
 	      if (remote_debug)
 		fprintf_unfiltered (gdb_stdlog, "Ack\n");
-	      return 1;
+	      return 1; /* We are happy, packet sending was successful. */
 	    case '-':
 	      if (remote_debug)
 		fprintf_unfiltered (gdb_stdlog, "Nak\n");
-	    case SERIAL_TIMEOUT:
-	      tcount++;
-	      if (tcount > 3)
-		return 0;
 	      break;		/* Retransmit buffer.  */
+	    case SERIAL_TIMEOUT:
+		continue;	/* Remote have not answered. Try again. */
 	    case '$':
 	      {
 	        if (remote_debug)
@@ -3893,33 +3894,25 @@
 		continue;	/* Now, go look for +.  */
 	      }
 	    default:
-	      if (remote_debug)
-		{
-		  if (!started_error_output)
-		    {
-		      started_error_output = 1;
-		      fprintf_unfiltered (gdb_stdlog, "putpkt: Junk: ");
-		    }
-		  fputc_unfiltered (ch & 0177, gdb_stdlog);
-		}
-	      continue;
-	    }
-	  break;		/* Here to retransmit.  */
-	}
-
-#if 0
-      /* This is wrong.  If doing a long backtrace, the user should be
-         able to get out next time we call QUIT, without anything as
-         violent as interrupt_query.  If we want to provide a way out of
-         here without getting to the next QUIT, it should be based on
-         hitting ^C twice as in remote_wait.  */
-      if (quit_flag)
-	{
-	  quit_flag = 0;
-	  interrupt_query ();
-	}
-#endif
-    }
+	      {
+	        if (remote_debug)
+		  {
+		    if (!started_error_output)
+		      {
+		        started_error_output = 1;
+		        fprintf_unfiltered (gdb_stdlog, "putpkt: Junk: ");
+		      }
+		    fputc_unfiltered (ch & 0177, gdb_stdlog);
+		  }
+	        continue;
+	      } /* endcase default */
+	    } /* endswitch read acknowedge */
+	  break;		/* Here to retransmit packet.  */
+	} /* endfor acknowledge reading */
+
+    } /* endfor packet sending */
+  /* Giving up. */
+  return 0; /* XXX At this moment nobody handle this return code! */
 }
 
 /* Come here after finding the start of a frame when we expected an

Re: remote protocol patch

[Daniel Jacobowitz]

On Wed, Jan 16, 2008 at 11:11:26AM +0100, Zoltán Filyó wrote:
> 
> Hi,
> 
> I found that the serial remote protocol runs into a deadlock when
> the serial line is noisy. Below a gdb log fragment with original
> code:

My strong recommendation is to not use the GDB serial protocol over a
noisy line.  There's other problems, for instance with packets which
have different effects when repeated.

> r +$S05#b8
> w +$g#67
> r <Timeout: 3 seconds>
> w $g#67
> r <Timeout: 3 seconds>
> w $g#67
> r <Timeout: 3 seconds>
> w $g#67
> r <Timeout: 3 seconds><Timeout: 3 seconds>
> w -
> r <Timeout: 3 seconds>
> w -
> r <Timeout: 3 seconds>
> w -+
> 
> infinitely. Because the target side NetBSD kernel throws all
> characters until get a new packet start ('$') char.

What's happened here, the four repetitions of $g#67 were all
completely lost, and then GDB timed out?  IMO, we should treat some of
these timeouts as fatal to the protocol conversation.

-- 
Daniel Jacobowitz
CodeSourcery

Re: remote protocol patch

[Zoltan Filyo]

Hi Daniel!

I did not fall in love with noisy lines but the real world...

I have GDB 6.5 code. I am debugging netbsd kernel with KGDB stub. Whole
stuff runs in vmware virtual machines. The KGDB eat the processor, and
the emulated serial chips does not honour this.

Imagine the situation. The GDB sends packets. Packets maybe arrived
{successfully, partially, nothing}.
Target stub have to follow instructions from GDB. Stub can do {wait a
new command, wait part of a command, refuse the packet}, but stub can
not know its response if arrived {successfully, partially, nothing}.

The KGDB stub does not handle timeouts (fact). The stub go in a state,
and waiting until an end packet character or a new packet start
character. (Maybe this is a misbehaviour.)

If the GDB does not get positive acknowledge in a period of time or gets
negative acknowledge or gets junk, then resends the previous packet. The
GDB logic is in putpkt_binary() function. The old code does not take
difference the above three case. When timeout expired three times,
putpkt_binary() returns with error code. But caller code (see {putpkg()
getpkg()} pair calls in remote.c file!) do not handle the error
condition. This causes that the GDB run into a state where never send
packet start nor packet end. See log: GDB sends packet nack infinitely
and KGDB wait packet start infinitely.

I could not find protocol definition (the requested behaviour, the state
diagrams for two sides etc.) of GDB. The exact solution will be a full,
closed protocol definition and conformance test suite for target and
remote side too. I have no enough spirit for this, sorry.

My patch causes a "better" behaviour only on the GDB side, but without
affect previous behaviour (if you want that). It takes differences
between "single send/response session"  and a "whole command resend"
case. Two parameters "max_ack_retry_count" and "max_packet_retry_count"
to be able to use regulate the GDB behaviour.
(And a better programming style, without "while(1)".)

Zoltan Filyo


On Tue, 2008-01-29 at 14:27 -0500, Daniel Jacobowitz wrote:
> On Wed, Jan 16, 2008 at 11:11:26AM +0100, Zoltán Filyó wrote:
> > 
> > Hi,
> > 
> > I found that the serial remote protocol runs into a deadlock when
> > the serial line is noisy. Below a gdb log fragment with original
> > code:
> 
> My strong recommendation is to not use the GDB serial protocol over a
> noisy line.  There's other problems, for instance with packets which
> have different effects when repeated.
> 
> > r +$S05#b8
> > w +$g#67
> > r <Timeout: 3 seconds>
> > w $g#67
> > r <Timeout: 3 seconds>
> > w $g#67
> > r <Timeout: 3 seconds>
> > w $g#67
> > r <Timeout: 3 seconds><Timeout: 3 seconds>
> > w -
> > r <Timeout: 3 seconds>
> > w -
> > r <Timeout: 3 seconds>
> > w -+
> > 
> > infinitely. Because the target side NetBSD kernel throws all
> > characters until get a new packet start ('$') char.
> 
> What's happened here, the four repetitions of $g#67 were all
> completely lost, and then GDB timed out?  IMO, we should treat some of
> these timeouts as fatal to the protocol conversation.
> 

Re: remote protocol patch

[Daniel Jacobowitz]

On Wed, Jan 30, 2008 at 03:30:27PM +0100, Zoltan Filyo wrote:
> The KGDB stub does not handle timeouts (fact). The stub go in a state,
> and waiting until an end packet character or a new packet start
> character. (Maybe this is a misbehaviour.)

This does not sound right.  Could you describe your original example
as seen by the KGDB stub - what text did it lose, and what text did it
receive and ignore?

> I could not find protocol definition (the requested behaviour, the state
> diagrams for two sides etc.) of GDB. The exact solution will be a full,
> closed protocol definition and conformance test suite for target and
> remote side too. I have no enough spirit for this, sorry.

It won't help you, anyway.  The protocol is not robust against a noisy
line, and modeling it more accurately will just make it clear how
broken it is if the line is noisy.

> My patch causes a "better" behaviour only on the GDB side, but without
> affect previous behaviour (if you want that). It takes differences
> between "single send/response session"  and a "whole command resend"
> case. Two parameters "max_ack_retry_count" and "max_packet_retry_count"
> to be able to use regulate the GDB behaviour.

I don't understand the difference between these two cases.

-- 
Daniel Jacobowitz
CodeSourcery

Re: remote protocol patch

[Zoltan Filyo]

On Wed, 2008-01-30 at 10:45 -0500, Daniel Jacobowitz wrote:
> On Wed, Jan 30, 2008 at 03:30:27PM +0100, Zoltan Filyo wrote:
> > The KGDB stub does not handle timeouts (fact). The stub go in a state,
> > and waiting until an end packet character or a new packet start
> > character. (Maybe this is a misbehaviour.)
> 
> This does not sound right.  Could you describe your original example
> as seen by the KGDB stub - what text did it lose, and what text did it
> receive and ignore?
> 
The code in the netbsd 4.0 is: /usr/src/sys/kern/kgdb_stub.c, line 239,
kgdb_recv(). After the kgdb_recv() read packet start step into the while
statement in line 253. It is waiting here until gets a "packet end char"
or the temp buffer run out of space.
It seems characters generally looses in set. Many times kgdb collects
"$6" or "$7" strings instead of "$g#67". The GDB before the patch gives
up after 3 trying, and sends "-" characters. But at this moment program
control had been leave the putpkt_binary() function.

Finally netbsd uses the com_common_getc() function to read characters
from the line (/usr/src/sys/dev/ic/com.c). I think this function should
be improved to handle "timeout" -- in that case too when the interrupts
are disabled. And a "timeout char" should be passed back as in GDB.


> > I could not find protocol definition (the requested behaviour, the state
> > diagrams for two sides etc.) of GDB. The exact solution will be a full,
> > closed protocol definition and conformance test suite for target and
> > remote side too. I have no enough spirit for this, sorry.
> 
> It won't help you, anyway.  The protocol is not robust against a noisy
> line, and modeling it more accurately will just make it clear how
> broken it is if the line is noisy.
> 
> > My patch causes a "better" behaviour only on the GDB side, but without
> > affect previous behaviour (if you want that). It takes differences
> > between "single send/response session"  and a "whole command resend"
> > case. Two parameters "max_ack_retry_count" and "max_packet_retry_count"
> > to be able to use regulate the GDB behaviour.
> 
> I don't understand the difference between these two cases.
> 
First case (state): the gdb waiting for an acknowledge from the peer
after packet send (sub state of the next). The second case (state): GDB
trying to send a packet to the peer.

In this particular debug session after the patch, gdb does not giving up
the packet resending, and kernel stub sooner or later get a "packet end
char" or its buffer run out of space. This causes that kgdb stub leaves
"collecting the packet" state and go into the "wait for packet start"
state.

Yes, this patch does not solve command duplication.