ser-unix.c:do_unix_readchar() Don't timeout when told not to

[Jonathan Larmour <jlarmour@redhat.co.uk>]

A while back, folks in the eCos team reported that you could get "Watchdog
has expired. Target detached." messages even when the watchdog wasn't set.
The GDB team reacted in disbelief :-).

After getting irritated by it this evening, I looked for and found the
problem: do_unix_readchar() could still return SERIAL_TIMEOUT, even when the
requested timeout was -1 (i.e. infinite).

The attached patch fixes the problem, and I used the opportunity to make the
code slightly more robust, and more similar to e.g. do_hardwire_readchar in
the same file.

I don't have write permission, so please check this in. Thanks,

Jifl

2000-03-20  Jonathan Larmour  <jlarmour@redhat.co.uk>

	* ser-unix.c (do_unix_readchar): Reorganise to be more robust,
	particularly ensuring it can't return SERIAL_TIMEOUT when told
	not to time out.


-- 
Red Hat, 35 Cambridge Place, Cambridge, UK. CB2 1NS  Tel: +44 (1223) 728762
"Plan to be spontaneous tomorrow."  ||  These opinions are all my own fault
Index: ser-unix.c
===================================================================
RCS file: /cvs/src/src/gdb/ser-unix.c,v
retrieving revision 1.2
diff -u -5 -p -c -r1.2 ser-unix.c
*** ser-unix.c	2000/02/09 08:52:47	1.2
--- ser-unix.c	2000/03/20 07:30:48
*************** do_unix_readchar (serial_t scb, int time
*** 908,920 ****
    /* We have to be able to keep the GUI alive here, so we break the original
       timeout into steps of 1 second, running the "keep the GUI alive" hook 
       each time through the loop.
  
       Also, timeout = 0 means to poll, so we just set the delta to 0, so we
!      will only go through the loop once. */
  
!   delta = (timeout == 0 ? 0 : 1);
    while (1)
      {
  
        /* N.B. The UI may destroy our world (for instance by calling
           remote_stop,) in which case we want to get out of here as
--- 908,920 ----
    /* We have to be able to keep the GUI alive here, so we break the original
       timeout into steps of 1 second, running the "keep the GUI alive" hook 
       each time through the loop.
  
       Also, timeout = 0 means to poll, so we just set the delta to 0, so we
!      will only go through the loop once. timeout < 0 means to wait forever. */
  
!   delta = (timeout <= 0 ? 0 : 1);
    while (1)
      {
  
        /* N.B. The UI may destroy our world (for instance by calling
           remote_stop,) in which case we want to get out of here as
*************** do_unix_readchar (serial_t scb, int time
*** 926,980 ****
  	{
  	  if (ui_loop_hook (0))
  	    return SERIAL_TIMEOUT;
  	}
  
!       status = ser_unix_wait_for (scb, delta);
        timeout -= delta;
  
!       /* If we got a character or an error back from wait_for, then we can 
!          break from the loop before the timeout is completed. */
  
!       if (status != SERIAL_TIMEOUT)
! 	{
! 	  break;
! 	}
  
!       /* If we have exhausted the original timeout, then generate
!          a SERIAL_TIMEOUT, and pass it out of the loop. */
  
!       else if (timeout == 0)
! 	{
! 	  status = SERIAL_TIMEOUT;
! 	  break;
  	}
-     }
- 
-   if (status < 0)
-     return status;
  
!   while (1)
!     {
!       status = read (scb->fd, scb->buf, BUFSIZ);
!       if (status != -1 || errno != EINTR)
! 	break;
!     }
! 
!   if (status <= 0)
!     {
!       if (status == 0)
! 	return SERIAL_TIMEOUT;	/* 0 chars means timeout [may need to
! 				   distinguish between EOF & timeouts
! 				   someday] */
!       else
! 	return SERIAL_ERROR;	/* Got an error from read */
      }
- 
-   scb->bufcnt = status;
-   scb->bufcnt--;
-   scb->bufp = scb->buf;
-   return *scb->bufp++;
  }
  
  /* Perform operations common to both old and new readchar. */
  
  /* Return the next character from the input FIFO.  If the FIFO is
--- 926,967 ----
  	{
  	  if (ui_loop_hook (0))
  	    return SERIAL_TIMEOUT;
  	}
  
!       status = ser_unix_wait_for (scb, timeout < 0 ? timeout : delta);
        timeout -= delta;
  
!       /* If we got an error back from wait_for, then we can return */
  
!       if (status == SERIAL_ERROR)
!         return status;
  
!       status = read (scb->fd, scb->buf, BUFSIZ);
  
!       if (status <= 0)
!         {
!           if (status == 0)
!             {
!               if (timeout != 0)
!                 continue;
!               else
!                 return SERIAL_TIMEOUT;	/* 0 chars means timeout [may need to
! 					   distinguish between EOF & timeouts
! 					   someday] */
!             }
! 	  else if (errno == EINTR)
!             continue;
!           else
! 	    return SERIAL_ERROR;	/* Got an error from read */
  	}
  
!       scb->bufcnt = status;
!       scb->bufcnt--;
!       scb->bufp = scb->buf;
!       return *scb->bufp++;
      }
  }
  
  /* Perform operations common to both old and new readchar. */
  
  /* Return the next character from the input FIFO.  If the FIFO is
From ac131313@cygnus.com Mon Mar 20 00:20:00 2000
From: Andrew Cagney <ac131313@cygnus.com>
To: Philippe GIACINTI <Philippe.Giacinti@sxb.bsf.alcatel.fr>
Cc: gdb-patches@sourceware.cygnus.com
Subject: Re: [PATCH] update: minor fixes in remote.c
Date: Mon, 20 Mar 2000 00:20:00 -0000
Message-id: <38D5DEF3.B5482CF4@cygnus.com>
References: <14549.54618.39052.823724@gargle.gargle.HOWL>
X-SW-Source: 2000-03/msg00365.html
Content-length: 1458

Philippe GIACINTI wrote:

> @@ -3439,7 +3439,8 @@
>        int todo;
>        int i;
> 
> -      todo = min (len, max_buf_size / 2);      /* num bytes that will fit */
> +      todo = min (len, max_buf_size / 2 - 2);  /* num bytes that will fit */
> +      /* -2 for leading $ and trailing # + checksum (2 hex chars) */
> 
>        /* construct "m"<memaddr>","<len>" */
>        /* sprintf (buf, "m%lx,%x", (unsigned long) memaddr, todo); */
> 
> If the host side needs to read for example 500 bytes and assuming that the
> maximum buffer size is 400 chars, then without the fix, the remote side will
> be asked to send min (500, 400/2)=200 bytes, its answer will then be:
>    $ ... 200 hex encoded bytes = 400 chars #CS = 400 + 4 chars which is greater
> than the max buffer size and will lead to an error from the host side.
> So we need to subtract 4 chars (i.e.: 2 hex encoded bytes) from requested
> number of bytes.

I'm still not sure that there is a problem here (I'm not saying that
there isn't though).  Several changes late last year and the testsuite
addition remote.exp addressed/tested many of the buffer overflow
problems that were lingering in that code.

Do you have a debug session or test case that demonstrates the problem?

One thing to keep in mind is that, when receiving packets, the ``$'' and
``#NN'' are not included in the size of incomming packets as the remote
code does not use the packet buffer for those characters.

	Andrew
From ac131313@cygnus.com Mon Mar 20 01:03:00 2000
From: Andrew Cagney <ac131313@cygnus.com>
To: Tom Tromey <tromey@cygnus.com>
Cc: gdb-patches@sourceware.cygnus.com
Subject: Re: Patch: make "!" a command unconditionally
Date: Mon, 20 Mar 2000 01:03:00 -0000
Message-id: <38D5E957.7604102@cygnus.com>
References: <87itz9cabi.fsf@cygnus.com> <38BB5D9F.1A9BF1B3@cygnus.com> <200002290701.XAA15591@ferrule.cygnus.com>
X-SW-Source: 2000-03/msg00366.html
Content-length: 2223

Tom Tromey wrote:
> 
> >>>>> "Andrew" == Andrew Cagney <ac131313@cygnus.com> writes:
> 
> Andrew> From memory this has been suggested before.  The problem pointed out
> Andrew> last time was that you need to specify:
> 
> Andrew>         ``!'' <space> <command>
> 
> I suggested it last time but didn't supply a patch.
> I agree it would be nice to eliminate the space.
> That's probably more than a 20 second hack though :-(
> 
> Andrew> I think there also may have been a debate over ``!'' as a
> Andrew> shell escape vs ``!'' for history.  (personally it didn't
> Andrew> worry me).  Fernando?
> 
> This doesn't bother me either.  For these kinds of tools "!" is more
> typically a shell escape.

FYI, I've checked in the attatched.

> Speaking of history, I wish gdb would pick up bash's "C-o"
> (operate-and-get-next) binding.

Hmm,

       operate-and-get-next (C-o)
              Accept the current line for execution and fetch the
              next  line  relative  to  the current line from the
              history for editing.  Any argument is ignored.

learn something new every day.  Probably involves tweeks to the readline
interface.

	Andrew
Mon Mar 20 19:58:45 2000  Andrew Cagney  <cagney@b1.cygnus.com>

	* command.c (_initialize_command): Document requirements for ``!''
 	command.

Index: command.c
===================================================================
RCS file: /cvs/src/src/gdb/command.c,v
retrieving revision 1.2
diff -p -r1.2 command.c
*** command.c	2000/02/09 08:52:45	1.2
--- command.c	2000/03/20 08:59:25
*************** _initialize_command ()
*** 1678,1683 ****
--- 1678,1689 ----
  	   "Execute the rest of the line as a shell command.  \n\
  With no arguments, run an inferior shell.");
  
+   /* NOTE: cagney/2000-03-20: Being able to enter ``(gdb) !ls'' would
+      be a really useful feature.  Unfortunatly, the below wont do
+      this.  Instead it adds support for the form ``(gdb) ! ls''
+      (i.e. the space is required).  If the ``!'' command below is
+      added the complains about no ``!'' command would be replaced by
+      complains about how the ``!'' command is broken :-) */
    if (xdb_commands)
      add_com_alias ("!", "shell", class_support, 0);
  
From Philippe.Giacinti@sxb.bsf.alcatel.fr Mon Mar 20 01:10:00 2000
From: Philippe GIACINTI <Philippe.Giacinti@sxb.bsf.alcatel.fr>
To: Andrew Cagney <ac131313@cygnus.com>
Cc: Philippe GIACINTI <Philippe.Giacinti@sxb.bsf.alcatel.fr>, gdb-patches@sourceware.cygnus.com
Subject: Re: [PATCH] update: minor fixes in remote.c
Date: Mon, 20 Mar 2000 01:10:00 -0000
Message-id: <14549.60108.37967.719735@gargle.gargle.HOWL>
References: <14549.54618.39052.823724@gargle.gargle.HOWL> <38D5DEF3.B5482CF4@cygnus.com>
X-SW-Source: 2000-03/msg00367.html
Content-length: 2140

Andrew Cagney <ac131313@cygnus.com> wrote:
 > Philippe GIACINTI wrote:
 > 
 > > @@ -3439,7 +3439,8 @@
 > >        int todo;
 > >        int i;
 > > 
 > > -      todo = min (len, max_buf_size / 2);      /* num bytes that will fit */
 > > +      todo = min (len, max_buf_size / 2 - 2);  /* num bytes that will fit */
 > > +      /* -2 for leading $ and trailing # + checksum (2 hex chars) */
 > > 
 > >        /* construct "m"<memaddr>","<len>" */
 > >        /* sprintf (buf, "m%lx,%x", (unsigned long) memaddr, todo); */
 > > 
 > > If the host side needs to read for example 500 bytes and assuming that the
 > > maximum buffer size is 400 chars, then without the fix, the remote side will
 > > be asked to send min (500, 400/2)=200 bytes, its answer will then be:
 > >    $ ... 200 hex encoded bytes = 400 chars #CS = 400 + 4 chars which is greater
 > > than the max buffer size and will lead to an error from the host side.
 > > So we need to subtract 4 chars (i.e.: 2 hex encoded bytes) from requested
 > > number of bytes.
 > 
 > I'm still not sure that there is a problem here (I'm not saying that
 > there isn't though).  Several changes late last year and the testsuite
 > addition remote.exp addressed/tested many of the buffer overflow
 > problems that were lingering in that code.
 > 
 > Do you have a debug session or test case that demonstrates the problem?
 > 
 > One thing to keep in mind is that, when receiving packets, the ``$'' and
 > ``#NN'' are not included in the size of incomming packets as the remote
 > code does not use the packet buffer for those characters.
 > 
 > 	Andrew
 > 
OK, you're right. I've checked with the last snapshot, and the problem doesn't
happen anymore. I had this problem with GDB-4.18 (see attached log) and fixed
it with the previous patch.
Sorry for the inconvenience ...
Anyway, the first part of the patch (using set_thread) should be OK.

Regards.
-- 
Philippe Giacinti - Alcatel Business Systems - PCD/SMB/SWPCC - CC S0645
1, route du Dr Albert Schweitzer - 67408 Illkirch Cedex - FRANCE
Tel: +33 (0)3.90.67.62.10 - Fax: +33 (0)3.90.67.77.93
mailto:Philippe.Giacinti@sxb.bsf.alcatel.fr