RE: [PATCH]Fix that GDB will get hang on Windows when using pipe to get stdout and stderr from stub

["Terry Guo" <terry.guo@arm.com>]

Hi Eli,

Could you please help to review this updated patch when you have time?
Thanks.

BR,
Terry

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Terry Guo
> Sent: Wednesday, July 04, 2012 4:20 PM
> To: 'Eli Zaretskii'
> Cc: gdb-patches@sourceware.org; Joey Ye
> Subject: RE: [PATCH]Fix that GDB will get hang on Windows when using
> pipe to get stdout and stderr from stub
> 
> Hi Eli,
> 
> > -----Original Message-----
> > From: Eli Zaretskii [mailto:eliz@gnu.org]
> > Sent: Thursday, June 28, 2012 12:54 AM
> > To: Terry Guo
> > Cc: gdb-patches@sourceware.org; Joey Ye; Matthew Gretton-Dann;
> > palves@redhat.com; daniel.jacobowitz@gmail.com
> > Subject: Re: [PATCH]Fix that GDB will get hang on Windows when using
> > pipe to get stdout and stderr from stub
> >
> > > From: "Terry Guo" <terry.guo@arm.com>
> > > Cc: <eliz@gnu.org>,
> > > 	"Joey Ye" <Joey.Ye@arm.com>,
> > > 	"Matthew Gretton-Dann" <Matthew.Gretton-Dann@arm.com>,
> > > 	"'Pedro Alves'" <palves@redhat.com>,
> > > 	<daniel.jacobowitz@gmail.com>
> > > Date: Tue, 26 Jun 2012 09:13:14 +0800
> > >
> > > I noticed a cross-built MINGW arm-none-eabi GDB will get hang on
> > Windows
> > > when use pipe to get stderr and stdout from stub. The command used
> to
> > start
> > > stub in GDB is "target extended-remote |
> > > stub-that-write-stderr-before-stdout". For my case, after send
> > > "$vFlashDone#ea" to stub, GDB get hang. The GDB source show that
> GDB
> > will
> > > keep waiting for ACK message from stdout of stub, after send the
> > packet.
> > > Unfortunately my stub will write some kind of log information into
> > stderr
> > > and this action takes place before stub write ACK message to its
> > stdout. So
> > > the only pipe is occupied by stderr which is waiting for GDB to
> > consume,
> > > while GDB keep waiting for message from the stdout which hasn't
> pipe
> > to use.
> > > We finally end up with a deadlock on pipe between GDB/stderr/stdout.
> > >
> > > The following patch can avoid such deadlock by letting GDB also
> probe
> > and
> > > consume stderr when waiting for stdout. Please review and comment.
> >
> > I only read it superficially, but it looked fine to me.
> >
> > Thanks.
> 
> Thanks for your review. Here I updated my patch to make it more robust.
> No
> changes on its functions. Please help to review again. Thanks.
> 
> The original patch is at:
> http://sourceware.org/ml/gdb-patches/2012-06/msg00790.html
> 
> BR,
> Terry
> 
> 2012-07-04  Terry Guo  <terry.guo@arm.com>
> 
>         * defs.h (GDB_MI_MSG_WIDTH): New.
>         * ser_base (ser_base_read_error_fd): New function.
>         (do_ser_base_readchar): Poll error file descriptor as well as
>         standard output.
>         (generic_readchar): Refactor error handling.
> 
> diff --git a/gdb/defs.h b/gdb/defs.h
> index 9531c5a..303721b 100644
> --- a/gdb/defs.h
> +++ b/gdb/defs.h
> @@ -1214,6 +1214,9 @@ extern int use_windows;
>  #define ISATTY(FP)     (isatty (fileno (FP)))
>  #endif
> 
> +/* A width that can achieve a better legibility for GDB MI mode.  */
> +#define GDB_MI_MSG_WIDTH  80
> +
>  /* Ensure that V is aligned to an N byte boundary (B's assumed to be a
>     power of 2).  Round up/down when necessary.  Examples of correct
>     use include:
> diff --git a/gdb/ser-base.c b/gdb/ser-base.c
> index 368afa6..661c5a9 100644
> --- a/gdb/ser-base.c
> +++ b/gdb/ser-base.c
> @@ -26,6 +26,7 @@
> 
>  #include "gdb_select.h"
>  #include "gdb_string.h"
> +#include "gdb_assert.h"
>  #include <sys/time.h>
>  #ifdef USE_WIN32API
>  #include <winsock2.h>
> @@ -223,6 +224,64 @@ ser_base_wait_for (struct serial *scb, int timeout)
>      }
>  }
> 
> +/* Read any error output we might have.  */
> +
> +void
> +ser_base_read_error_fd (struct serial *scb, int close_fd)
> +{
> +  if (scb->error_fd != -1)
> +    {
> +      ssize_t s;
> +      char buf[GDB_MI_MSG_WIDTH + 1];
> +
> +      for (;;)
> +        {
> +         char *current;
> +         char *newline;
> +         int to_read = GDB_MI_MSG_WIDTH;
> +         int num_bytes = -1;
> +
> +         if (scb->ops->avail)
> +           num_bytes = (scb->ops->avail)(scb, scb->error_fd);
> +
> +         if (num_bytes != -1)
> +           to_read = (num_bytes < to_read) ? num_bytes : to_read;
> +
> +         if (to_read == 0)
> +           break;
> +
> +         s = read (scb->error_fd, &buf, to_read);
> +         if ((s == -1) || (s == 0 && !close_fd))
> +           break;
> +
> +         if (s == 0 && close_fd)
> +           {
> +             /* End of file.  */
> +             close (scb->error_fd);
> +             scb->error_fd = -1;
> +             break;
> +           }
> +
> +         /* In theory, embedded newlines are not a problem.
> +            But for MI, we want each output line to have just
> +            one newline for legibility.  So output things
> +            in newline chunks.  */
> +         gdb_assert (s > 0 && s <= GDB_MI_MSG_WIDTH);
> +         buf[s] = '\0';
> +         current = buf;
> +         while ((newline = strstr (current, "\n")) != NULL)
> +           {
> +             *newline = '\0';
> +             fputs_unfiltered (current, gdb_stderr);
> +             fputs_unfiltered ("\n", gdb_stderr);
> +             current = newline + 1;
> +           }
> +
> +         fputs_unfiltered (current, gdb_stderr);
> +       }
> +    }
> +}
> +
>  /* Read a character with user-specified timeout.  TIMEOUT is number of
> seconds
>     to wait, or -1 to wait forever.  Use timeout of 0 to effect a poll.
> Returns
>     char if successful.  Returns -2 if timeout expired, EOF if line
> dropped
> @@ -273,6 +332,11 @@ do_ser_base_readchar (struct serial *scb, int
> timeout)
>           status = SERIAL_TIMEOUT;
>           break;
>         }
> +
> +      /* We also need to check and consume the stderr because it could
> +         come before the stdout for some stubs.  If we just sit and
> wait
> +         for stdout, we would hit a deadlock for that case.  */
> +      ser_base_read_error_fd (scb, 0);
>      }
> 
>    if (status < 0)
> @@ -344,53 +408,7 @@ generic_readchar (struct serial *scb, int timeout,
>         }
>      }
>    /* Read any error output we might have.  */
> -  if (scb->error_fd != -1)
> -    {
> -      ssize_t s;
> -      char buf[81];
> -
> -      for (;;)
> -        {
> -         char *current;
> -         char *newline;
> -         int to_read = 80;
> -
> -         int num_bytes = -1;
> -         if (scb->ops->avail)
> -           num_bytes = (scb->ops->avail)(scb, scb->error_fd);
> -         if (num_bytes != -1)
> -           to_read = (num_bytes < to_read) ? num_bytes : to_read;
> -
> -         if (to_read == 0)
> -           break;
> -
> -         s = read (scb->error_fd, &buf, to_read);
> -         if (s == -1)
> -           break;
> -         if (s == 0)
> -           {
> -             /* EOF */
> -             close (scb->error_fd);
> -             scb->error_fd = -1;
> -             break;
> -           }
> -
> -         /* In theory, embedded newlines are not a problem.
> -            But for MI, we want each output line to have just
> -            one newline for legibility.  So output things
> -            in newline chunks.  */
> -         buf[s] = '\0';
> -         current = buf;
> -         while ((newline = strstr (current, "\n")) != NULL)
> -           {
> -             *newline = '\0';
> -             fputs_unfiltered (current, gdb_stderr);
> -             fputs_unfiltered ("\n", gdb_stderr);
> -             current = newline + 1;
> -           }
> -         fputs_unfiltered (current, gdb_stderr);
> -       }
> -    }
> +  ser_base_read_error_fd (scb, 1);
> 
>    reschedule (scb);
>    return ch;
> 
> 
>