"target remote | " stderr

"target remote | " stderr

[Vladimir Prus]

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


At the moment, when handling

	target remote | whatever

gdb does not do anything with 'whatever''s stderr. This is not good,
because when using MI, frontend might not even look at stderr at all,
so messages from 'whatever' get lost.

This patch fixes that. The fix is only for Linux, I plan to do the
same with mingw support, but only if this patch is considered OK.

- Volodya

	Pass stderr of program run with "target remote |"
	via gdb_stderr.
	* serial.c (serial_open): Set error_fd to -1.
	* serial.h (struct serial): New field error_fd.
	* ser-pipe.c (pipe_open): Create another pair
	of sockets.  Pass stderr to gdb.
	* ser-base.c (generic_readchar): Check if there's
	anything in stderr channel and route that to gdb_stderr.

[-- Attachment #2: remote_stderr__gdb_mainline.diff --]
[-- Type: text/x-diff, Size: 3660 bytes --]

--- gdb/serial.c	(/mirrors/gdb_mainline)	(revision 3222)
+++ gdb/serial.c	(/patches/gdb/remote_stderr/gdb_mainline)	(revision 3222)
@@ -211,6 +211,7 @@ serial_open (const char *name)
 
   scb->bufcnt = 0;
   scb->bufp = scb->buf;
+  scb->error_fd = -1;
 
   if (scb->ops->open (scb, open_name))
     {
--- gdb/serial.h	(/mirrors/gdb_mainline)	(revision 3222)
+++ gdb/serial.h	(/patches/gdb/remote_stderr/gdb_mainline)	(revision 3222)
@@ -191,6 +191,12 @@ extern int serial_debug_p (struct serial
 struct serial
   {
     int fd;			/* File descriptor */
+    int error_fd;               /* File descriptor for a separate
+				   error stream that should be
+				   immediately forwarded to gdb_stderr.
+				   This may be -1.  
+				   If != -1, this descriptor should
+				   be non-blocking.  */
     struct serial_ops *ops;	/* Function vector */
     void *state;       		/* Local context info for open FD */
     serial_ttystate ttystate;	/* Not used (yet) */
--- gdb/ser-pipe.c	(/mirrors/gdb_mainline)	(revision 3222)
+++ gdb/ser-pipe.c	(/patches/gdb/remote_stderr/gdb_mainline)	(revision 3222)
@@ -62,9 +62,12 @@ pipe_open (struct serial *scb, const cha
    * published in UNIX Review, Vol. 6, No. 8.
    */
   int pdes[2];
+  int err_pdes[2];
   int pid;
   if (socketpair (AF_UNIX, SOCK_STREAM, 0, pdes) < 0)
     return -1;
+  if (socketpair (AF_UNIX, SOCK_STREAM, 0, err_pdes) < 0)
+    return -1;
 
   /* Create the child process to run the command in.  Note that the
      apparent call to vfork() below *might* actually be a call to
@@ -77,9 +80,18 @@ pipe_open (struct serial *scb, const cha
     {
       close (pdes[0]);
       close (pdes[1]);
+      close (err_pdes[0]);
+      close (err_pdes[1]);
       return -1;
     }
 
+  if (fcntl (err_pdes[0], F_SETFL, O_NONBLOCK) == -1)
+    {
+      close (err_pdes[0]);
+      close (err_pdes[1]);
+      err_pdes[0] = err_pdes[1] = -1;
+    }
+
   /* Child. */
   if (pid == 0)
     {
@@ -91,6 +103,13 @@ pipe_open (struct serial *scb, const cha
 	  close (pdes[1]);
 	}
       dup2 (STDOUT_FILENO, STDIN_FILENO);
+
+      if (err_pdes[0] != -1)
+	{
+	  close (err_pdes[0]);
+	  dup2 (err_pdes[1], STDERR_FILENO);
+	  close (err_pdes[1]);
+	}
 #if 0
       /* close any stray FD's - FIXME - how? */
       /* POSIX.2 B.3.2.2 "popen() shall ensure that any streams
@@ -109,6 +128,7 @@ pipe_open (struct serial *scb, const cha
   state = XMALLOC (struct pipe_state);
   state->pid = pid;
   scb->fd = pdes[0];
+  scb->error_fd = err_pdes[0];
   scb->state = state;
 
   /* If we don't do this, GDB simply exits when the remote side dies.  */
--- gdb/ser-base.c	(/mirrors/gdb_mainline)	(revision 3222)
+++ gdb/ser-base.c	(/patches/gdb/remote_stderr/gdb_mainline)	(revision 3222)
@@ -314,6 +314,33 @@ generic_readchar (struct serial *scb, in
 		  int (do_readchar) (struct serial *scb, int timeout))
 {
   int ch;
+
+  /* Read any error output we might have.  */
+  if (scb->error_fd != -1)
+    {
+      ssize_t s;
+      char buf[81];
+      while ((s = read (scb->error_fd, &buf, 80)) > 0)
+	{
+	  char *current;
+	  char *newline;
+	  /* 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);
+	}
+    }
+
   if (scb->bufcnt > 0)
     {
       ch = *scb->bufp;

Re: "target remote | " stderr

[Daniel Jacobowitz]

On Fri, Jan 26, 2007 at 04:53:53PM +0300, Vladimir Prus wrote:
> 
> At the moment, when handling
> 
> 	target remote | whatever
> 
> gdb does not do anything with 'whatever''s stderr. This is not good,
> because when using MI, frontend might not even look at stderr at all,
> so messages from 'whatever' get lost.
> 
> This patch fixes that. The fix is only for Linux, I plan to do the
> same with mingw support, but only if this patch is considered OK.
> 
> - Volodya
> 
> 	Pass stderr of program run with "target remote |"
> 	via gdb_stderr.
> 	* serial.c (serial_open): Set error_fd to -1.
> 	* serial.h (struct serial): New field error_fd.
> 	* ser-pipe.c (pipe_open): Create another pair
> 	of sockets.  Pass stderr to gdb.
> 	* ser-base.c (generic_readchar): Check if there's
> 	anything in stderr channel and route that to gdb_stderr.

The patch seems OK to me; though I would like to be sure we can
implement this for MinGW before we get too used to the idea.

-- 
Daniel Jacobowitz
CodeSourcery

Re: "target remote | " stderr

[Vladimir Prus]

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

Daniel Jacobowitz wrote:

>> Pass stderr of program run with "target remote |"
>> via gdb_stderr.
>> * serial.c (serial_open): Set error_fd to -1.
>> * serial.h (struct serial): New field error_fd.
>> * ser-pipe.c (pipe_open): Create another pair
>> of sockets.  Pass stderr to gdb.
>> * ser-base.c (generic_readchar): Check if there's
>> anything in stderr channel and route that to gdb_stderr.
> 
> The patch seems OK to me; though I would like to be sure we can
> implement this for MinGW before we get too used to the idea.

Implementing for MinGW required modifying libiberty so that 
it can catch stderr to a pipe. Here's a patch for gdb that
relies on the libiberty patch. Does this sound OK
provided libiberty patch is approved?

- Volodya


        Pass stderr of program run with "target remote |"
        via gdb_stderr.
        * serial.c (serial_open): Set error_fd to -1.
        * serial.h (struct serial): New field error_fd.
        * ser-pipe.c (pipe_open): Create another pair
        of sockets.  Pass stderr to gdb.
        * ser-mingw.c (pipe_windows_open): Pass
        PEX_STDERR_TO_PIPE to pex_run.  Initialize
        sd->error_fd.
        * ser-base.c (generic_readchar): Check if there's
        anything in stderr channel and route that to gdb_stderr.

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

--- gdb/serial.c	(/patches/gdb/mingw_cpp_declarations/gdb_mainline)	(revision 3263)
+++ gdb/serial.c	(/patches/gdb/remote_stderr_try2/gdb_mainline)	(revision 3263)
@@ -211,6 +211,7 @@ serial_open (const char *name)
 
   scb->bufcnt = 0;
   scb->bufp = scb->buf;
+  scb->error_fd = -1;
 
   if (scb->ops->open (scb, open_name))
     {
--- gdb/serial.h	(/patches/gdb/mingw_cpp_declarations/gdb_mainline)	(revision 3263)
+++ gdb/serial.h	(/patches/gdb/remote_stderr_try2/gdb_mainline)	(revision 3263)
@@ -191,6 +191,12 @@ extern int serial_debug_p (struct serial
 struct serial
   {
     int fd;			/* File descriptor */
+    int error_fd;               /* File descriptor for a separate
+				   error stream that should be
+				   immediately forwarded to gdb_stderr.
+				   This may be -1.  
+				   If != -1, this descriptor should
+				   be non-blocking.  */
     struct serial_ops *ops;	/* Function vector */
     void *state;       		/* Local context info for open FD */
     serial_ttystate ttystate;	/* Not used (yet) */
--- gdb/ser-pipe.c	(/patches/gdb/mingw_cpp_declarations/gdb_mainline)	(revision 3263)
+++ gdb/ser-pipe.c	(/patches/gdb/remote_stderr_try2/gdb_mainline)	(revision 3263)
@@ -62,9 +62,12 @@ pipe_open (struct serial *scb, const cha
    * published in UNIX Review, Vol. 6, No. 8.
    */
   int pdes[2];
+  int err_pdes[2];
   int pid;
   if (socketpair (AF_UNIX, SOCK_STREAM, 0, pdes) < 0)
     return -1;
+  if (socketpair (AF_UNIX, SOCK_STREAM, 0, err_pdes) < 0)
+    return -1;
 
   /* Create the child process to run the command in.  Note that the
      apparent call to vfork() below *might* actually be a call to
@@ -77,9 +80,18 @@ pipe_open (struct serial *scb, const cha
     {
       close (pdes[0]);
       close (pdes[1]);
+      close (err_pdes[0]);
+      close (err_pdes[1]);
       return -1;
     }
 
+  if (fcntl (err_pdes[0], F_SETFL, O_NONBLOCK) == -1)
+    {
+      close (err_pdes[0]);
+      close (err_pdes[1]);
+      err_pdes[0] = err_pdes[1] = -1;
+    }
+
   /* Child. */
   if (pid == 0)
     {
@@ -91,6 +103,13 @@ pipe_open (struct serial *scb, const cha
 	  close (pdes[1]);
 	}
       dup2 (STDOUT_FILENO, STDIN_FILENO);
+
+      if (err_pdes[0] != -1)
+	{
+	  close (err_pdes[0]);
+	  dup2 (err_pdes[1], STDERR_FILENO);
+	  close (err_pdes[1]);
+	}
 #if 0
       /* close any stray FD's - FIXME - how? */
       /* POSIX.2 B.3.2.2 "popen() shall ensure that any streams
@@ -109,6 +128,7 @@ pipe_open (struct serial *scb, const cha
   state = XMALLOC (struct pipe_state);
   state->pid = pid;
   scb->fd = pdes[0];
+  scb->error_fd = err_pdes[0];
   scb->state = state;
 
   /* If we don't do this, GDB simply exits when the remote side dies.  */
--- gdb/ser-base.c	(/patches/gdb/mingw_cpp_declarations/gdb_mainline)	(revision 3263)
+++ gdb/ser-base.c	(/patches/gdb/remote_stderr_try2/gdb_mainline)	(revision 3263)
@@ -314,6 +314,33 @@ generic_readchar (struct serial *scb, in
 		  int (do_readchar) (struct serial *scb, int timeout))
 {
   int ch;
+
+  /* Read any error output we might have.  */
+  if (scb->error_fd != -1)
+    {
+      ssize_t s;
+      char buf[81];
+      while ((s = read (scb->error_fd, &buf, 80)) > 0)
+	{
+	  char *current;
+	  char *newline;
+	  /* 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);
+	}
+    }
+
   if (scb->bufcnt > 0)
     {
       ch = *scb->bufp;
--- gdb/ser-mingw.c	(/patches/gdb/mingw_cpp_declarations/gdb_mainline)	(revision 3263)
+++ gdb/ser-mingw.c	(/patches/gdb/remote_stderr_try2/gdb_mainline)	(revision 3263)
@@ -697,6 +697,7 @@ static int
 pipe_windows_open (struct serial *scb, const char *name)
 {
   struct pipe_state *ps;
+  FILE *pex_stderr;
 
   char **argv = buildargv (name);
   struct cleanup *back_to = make_cleanup_freeargv (argv);
@@ -717,7 +718,8 @@ pipe_windows_open (struct serial *scb, c
   {
     int err;
     const char *err_msg
-      = pex_run (ps->pex, PEX_SEARCH | PEX_BINARY_INPUT | PEX_BINARY_OUTPUT,
+      = pex_run (ps->pex, PEX_SEARCH | PEX_BINARY_INPUT | PEX_BINARY_OUTPUT
+		 | PEX_STDERR_TO_PIPE,
                  argv[0], argv, NULL, NULL,
                  &err);
 
@@ -739,8 +741,13 @@ pipe_windows_open (struct serial *scb, c
   ps->output = pex_read_output (ps->pex, 1);
   if (! ps->output)
     goto fail;
-
   scb->fd = fileno (ps->output);
+
+  pex_stderr = pex_read_err (ps->pex, 1);
+  if (! pex_stderr)
+    goto fail;
+  scb->error_fd = fileno (pex_stderr);
+
   scb->state = (void *) ps;
 
   discard_cleanups (back_to);

Property changes on: 
___________________________________________________________________
Name: csl:base
 +/all/patches/gdb/mingw_cpp_declarations/gdb_mainline

Re: "target remote | " stderr

[Daniel Jacobowitz]

On Wed, Jan 31, 2007 at 05:34:21PM +0300, Vladimir Prus wrote:
> Implementing for MinGW required modifying libiberty so that 
> it can catch stderr to a pipe. Here's a patch for gdb that
> relies on the libiberty patch. Does this sound OK
> provided libiberty patch is approved?

Do we automatically get a non-blocking pipe on mingw, or do we have to
set it ourselves?  Other than that, it looks fine.


-- 
Daniel Jacobowitz
CodeSourcery

Re: "target remote | " stderr

[Vladimir Prus]

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

Daniel Jacobowitz wrote:

> On Wed, Jan 31, 2007 at 05:34:21PM +0300, Vladimir Prus wrote:
>> Implementing for MinGW required modifying libiberty so that
>> it can catch stderr to a pipe. Here's a patch for gdb that
>> relies on the libiberty patch. Does this sound OK
>> provided libiberty patch is approved?
> 
> Do we automatically get a non-blocking pipe on mingw, or do we have to
> set it ourselves?  

We have to set it ourself, unfortunately. I had one test case working
fine, but got gdb hanging on some other.

The attached patch works around the blocking issues, and more extensively
tested. OK?

- Volodya

        Pass stderr of program run with "target remote |"
        via gdb_stderr.
        * serial.c (serial_open): Set error_fd to -1.
        * serial.h (struct serial): New field error_fd.
        * ser-pipe.c (pipe_open): Create another pair
        of sockets.  Pass stderr to gdb.
        * ser-mingw.c (pipe_windows_open): Pass
        PEX_STDERR_TO_PIPE to pex_run.  Initialize
        sd->error_fd.
        * ser-base.c (generic_readchar): Check if there's
        anything in stderr channel and route that to gdb_stderr.

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

--- gdb/serial.c	(/mirrors/gdb_mainline)	(revision 3417)
+++ gdb/serial.c	(/patches/gdb/remote_stderr_try2/gdb_mainline)	(revision 3417)
@@ -211,6 +211,7 @@ serial_open (const char *name)
 
   scb->bufcnt = 0;
   scb->bufp = scb->buf;
+  scb->error_fd = -1;
 
   if (scb->ops->open (scb, open_name))
     {
--- gdb/serial.h	(/mirrors/gdb_mainline)	(revision 3417)
+++ gdb/serial.h	(/patches/gdb/remote_stderr_try2/gdb_mainline)	(revision 3417)
@@ -191,6 +191,12 @@ extern int serial_debug_p (struct serial
 struct serial
   {
     int fd;			/* File descriptor */
+    int error_fd;               /* File descriptor for a separate
+				   error stream that should be
+				   immediately forwarded to gdb_stderr.
+				   This may be -1.  
+				   If != -1, this descriptor should
+				   be non-blocking.  */
     struct serial_ops *ops;	/* Function vector */
     void *state;       		/* Local context info for open FD */
     serial_ttystate ttystate;	/* Not used (yet) */
--- gdb/ser-pipe.c	(/mirrors/gdb_mainline)	(revision 3417)
+++ gdb/ser-pipe.c	(/patches/gdb/remote_stderr_try2/gdb_mainline)	(revision 3417)
@@ -62,9 +62,12 @@ pipe_open (struct serial *scb, const cha
    * published in UNIX Review, Vol. 6, No. 8.
    */
   int pdes[2];
+  int err_pdes[2];
   int pid;
   if (socketpair (AF_UNIX, SOCK_STREAM, 0, pdes) < 0)
     return -1;
+  if (socketpair (AF_UNIX, SOCK_STREAM, 0, err_pdes) < 0)
+    return -1;
 
   /* Create the child process to run the command in.  Note that the
      apparent call to vfork() below *might* actually be a call to
@@ -77,9 +80,18 @@ pipe_open (struct serial *scb, const cha
     {
       close (pdes[0]);
       close (pdes[1]);
+      close (err_pdes[0]);
+      close (err_pdes[1]);
       return -1;
     }
 
+  if (fcntl (err_pdes[0], F_SETFL, O_NONBLOCK) == -1)
+    {
+      close (err_pdes[0]);
+      close (err_pdes[1]);
+      err_pdes[0] = err_pdes[1] = -1;
+    }
+
   /* Child. */
   if (pid == 0)
     {
@@ -91,6 +103,13 @@ pipe_open (struct serial *scb, const cha
 	  close (pdes[1]);
 	}
       dup2 (STDOUT_FILENO, STDIN_FILENO);
+
+      if (err_pdes[0] != -1)
+	{
+	  close (err_pdes[0]);
+	  dup2 (err_pdes[1], STDERR_FILENO);
+	  close (err_pdes[1]);
+	}
 #if 0
       /* close any stray FD's - FIXME - how? */
       /* POSIX.2 B.3.2.2 "popen() shall ensure that any streams
@@ -109,6 +128,7 @@ pipe_open (struct serial *scb, const cha
   state = XMALLOC (struct pipe_state);
   state->pid = pid;
   scb->fd = pdes[0];
+  scb->error_fd = err_pdes[0];
   scb->state = state;
 
   /* If we don't do this, GDB simply exits when the remote side dies.  */
--- gdb/ser-base.c	(/mirrors/gdb_mainline)	(revision 3417)
+++ gdb/ser-base.c	(/patches/gdb/remote_stderr_try2/gdb_mainline)	(revision 3417)
@@ -314,6 +314,7 @@ generic_readchar (struct serial *scb, in
 		  int (do_readchar) (struct serial *scb, int timeout))
 {
   int ch;
+
   if (scb->bufcnt > 0)
     {
       ch = *scb->bufp;
@@ -343,6 +344,51 @@ generic_readchar (struct serial *scb, in
 	    }
 	}
     }
+  /* Read any error output we might have.  */
+  if (scb->error_fd != -1)
+    {
+      ssize_t s;
+      char buf[81];
+
+      for (;;)
+        {
+ 	  char *current;
+ 	  char *newline;
+
+#ifdef _WIN32
+	  /* On windows, it's not easy to make a pipe non-blocking,
+	     if at all possible. Therefore, first find out
+	     how many bytes are in the pipes and read only the available
+	     number of bytes so that to avoid blocking.  */
+	  HANDLE h = (HANDLE) _get_osfhandle (scb->error_fd);
+	  DWORD numBytes;
+	  BOOL r = PeekNamedPipe (h, NULL, 0, NULL, &numBytes, NULL);
+	  if (r == FALSE || numBytes == 0)
+	    break;
+	  s = read (scb->error_fd, &buf, numBytes > 80 ? 80 : numBytes);
+#else
+	  s = read (scb->error_fd, &buf, 80);
+#endif	  
+	  if (s == -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);
+	}
+    }
+
   reschedule (scb);
   return ch;
 }
--- gdb/ser-mingw.c	(/mirrors/gdb_mainline)	(revision 3417)
+++ gdb/ser-mingw.c	(/patches/gdb/remote_stderr_try2/gdb_mainline)	(revision 3417)
@@ -697,6 +697,7 @@ static int
 pipe_windows_open (struct serial *scb, const char *name)
 {
   struct pipe_state *ps;
+  FILE *pex_stderr;
 
   char **argv = buildargv (name);
   struct cleanup *back_to = make_cleanup_freeargv (argv);
@@ -717,7 +718,8 @@ pipe_windows_open (struct serial *scb, c
   {
     int err;
     const char *err_msg
-      = pex_run (ps->pex, PEX_SEARCH | PEX_BINARY_INPUT | PEX_BINARY_OUTPUT,
+      = pex_run (ps->pex, PEX_SEARCH | PEX_BINARY_INPUT | PEX_BINARY_OUTPUT
+		 | PEX_STDERR_TO_PIPE,
                  argv[0], argv, NULL, NULL,
                  &err);
 
@@ -739,8 +741,13 @@ pipe_windows_open (struct serial *scb, c
   ps->output = pex_read_output (ps->pex, 1);
   if (! ps->output)
     goto fail;
-
   scb->fd = fileno (ps->output);
+
+  pex_stderr = pex_read_err (ps->pex, 1);
+  if (! pex_stderr)
+    goto fail;
+  scb->error_fd = fileno (pex_stderr);
+
   scb->state = (void *) ps;
 
   discard_cleanups (back_to);

Property changes on: 
___________________________________________________________________
Name: csl:base
 +/all/mirrors/gdb_mainline
Name: svk:merge
 +e7755896-6108-0410-9592-8049d3e74e28:/mirrors/gdb/trunk:162899

Re: "target remote | " stderr

[Mark Kettenis]

> From: Vladimir Prus <ghost@cs.msu.su>
> Date:  Sun, 11 Feb 2007 19:55:03 +0300
> 
> Daniel Jacobowitz wrote:
> 
> > On Wed, Jan 31, 2007 at 05:34:21PM +0300, Vladimir Prus wrote:
> >> Implementing for MinGW required modifying libiberty so that
> >> it can catch stderr to a pipe. Here's a patch for gdb that
> >> relies on the libiberty patch. Does this sound OK
> >> provided libiberty patch is approved?
> > 
> > Do we automatically get a non-blocking pipe on mingw, or do we have to
> > set it ourselves?  
> 
> We have to set it ourself, unfortunately. I had one test case working
> fine, but got gdb hanging on some other.
> 
> The attached patch works around the blocking issues, and more extensively
> tested. OK?

Please keep Windows-specific code out of ser-base.c.  Is read() on
MinGW fully blocking?  Or will it return whatever is available and
only block if nothing is available.  Then you could use gdb_select()
to check whether input is available on the pipe.

Otherwise, you should think about introducing an asbtraction layer for
reading from error_fd.

Mark

Re: "target remote | " stderr

[Vladimir Prus]

Mark Kettenis wrote:

>> From: Vladimir Prus <ghost@cs.msu.su>
>> Date:  Sun, 11 Feb 2007 19:55:03 +0300
>> 
>> Daniel Jacobowitz wrote:
>> 
>> > On Wed, Jan 31, 2007 at 05:34:21PM +0300, Vladimir Prus wrote:
>> >> Implementing for MinGW required modifying libiberty so that
>> >> it can catch stderr to a pipe. Here's a patch for gdb that
>> >> relies on the libiberty patch. Does this sound OK
>> >> provided libiberty patch is approved?
>> > 
>> > Do we automatically get a non-blocking pipe on mingw, or do we have to
>> > set it ourselves?
>> 
>> We have to set it ourself, unfortunately. I had one test case working
>> fine, but got gdb hanging on some other.
>> 
>> The attached patch works around the blocking issues, and more extensively
>> tested. OK?
> 
> Please keep Windows-specific code out of ser-base.c.  Is read() on
> MinGW fully blocking?  Or will it return whatever is available and
> only block if nothing is available.  

Documentation for ReadFile suggests that it will block until the
requested number of bytes is available.

> Then you could use gdb_select() 
> to check whether input is available on the pipe.

How the possibility of using gdb_select() depends on read() being fully
blocking or not? I think I miss some connection here.

> Otherwise, you should think about introducing an asbtraction layer for
> reading from error_fd.

Is it indeed worthwhile to introduce abstraction layer to hide 5 lines of code?

- Volodya

Re: "target remote | " stderr

[Eli Zaretskii]

> From:  Vladimir Prus <ghost@cs.msu.su>
> Date:  Sun, 11 Feb 2007 19:55:03 +0300
>
> --- gdb/serial.h	(/mirrors/gdb_mainline)	(revision 3417)
> +++ gdb/serial.h	(/patches/gdb/remote_stderr_try2/gdb_mainline)	(revision 3417)
> @@ -191,6 +191,12 @@ extern int serial_debug_p (struct serial
>  struct serial
>    {
>      int fd;			/* File descriptor */
> +    int error_fd;               /* File descriptor for a separate
> +				   error stream that should be
> +				   immediately forwarded to gdb_stderr.
> +				   This may be -1.  
> +				   If != -1, this descriptor should
> +				   be non-blocking.  */

This comment isn't according to GNU coding standards, I think.

> +#ifdef _WIN32

Won't this catch Cygwin as well?  Do we want that?

Thanks.

Re: "target remote | " stderr

[Vladimir Prus]

Eli Zaretskii wrote:

>> From:  Vladimir Prus <ghost@cs.msu.su>
>> Date:  Sun, 11 Feb 2007 19:55:03 +0300
>>
>> --- gdb/serial.h     (/mirrors/gdb_mainline) (revision 3417)
>> +++ gdb/serial.h     (/patches/gdb/remote_stderr_try2/gdb_mainline)  (revision
>> 3417) @@ -191,6 +191,12 @@ extern int serial_debug_p (struct serial
>>  struct serial
>>    {
>>      int fd;                 /* File descriptor */
>> +    int error_fd;               /* File descriptor for a separate
>> +                               error stream that should be
>> +                               immediately forwarded to gdb_stderr.
>> +                               This may be -1.
>> +                               If != -1, this descriptor should
>> +                               be non-blocking.  */
> 
> This comment isn't according to GNU coding standards, I think.

Can you please be more specific?

>> +#ifdef _WIN32
> 
> Won't this catch Cygwin as well?  Do we want that?

I would hope this won't catch cygwin, but I don't know. I'll check.

- Volodya

Re: "target remote | " stderr

[Eli Zaretskii]

> From: Vladimir Prus <ghost@cs.msu.su>
> Date: Sat, 17 Feb 2007 10:34:36 +0300
> 
> >> +    int error_fd;               /* File descriptor for a separate
> >> +                               error stream that should be
> >> +                               immediately forwarded to gdb_stderr.
> >> +                               This may be -1.
> >> +                               If != -1, this descriptor should
> >> +                               be non-blocking.  */
> > 
> > This comment isn't according to GNU coding standards, I think.
> 
> Can you please be more specific?

Not sure what you want me to say.  Multiline, multiple-sentence
comments should not be alongside of the code, they should precede the
code.  Example:

    /* File descriptor for a separate error stream that should be
       immediately forwarded to gdb_stderr.  This may be -1.
       If != -1, this descriptor should be non-blocking.  */
    int error_fd;

> >> +#ifdef _WIN32
> > 
> > Won't this catch Cygwin as well?  Do we want that?
> 
> I would hope this won't catch cygwin, but I don't know. I'll check.

Thanks.

Re: "target remote | " stderr

[Vladimir Prus]

On Saturday 17 February 2007 13:56, Eli Zaretskii wrote:
> > From: Vladimir Prus <ghost@cs.msu.su>
> > Date: Sat, 17 Feb 2007 10:34:36 +0300
> > 
> > >> +    int error_fd;               /* File descriptor for a separate
> > >> +                               error stream that should be
> > >> +                               immediately forwarded to gdb_stderr.
> > >> +                               This may be -1.
> > >> +                               If != -1, this descriptor should
> > >> +                               be non-blocking.  */
> > > 
> > > This comment isn't according to GNU coding standards, I think.
> > 
> > Can you please be more specific?
> 
> Not sure what you want me to say.  

Well...

> Multiline, multiple-sentence 
> comments should not be alongside of the code, they should precede the
> code.  

... the above. With your original comment I went looking for 
missing double spaces, found none, and had no idea what you mean.

- Volodya