[RFA/commit] Handle EOF on terminals opened with ENONBLOCK...

[RFA/commit] Handle EOF on terminals opened with ENONBLOCK...

[Joel Brobecker]

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

This is an interesting situation that we encountered on AIX.
At one point, I thought it might have been the same issue as the one
that prevents us from running the testsuite on AIX, but no such luck,
unfortunately.

What happens is that we start GDB in a pseudo-terminal that has the
O_NONBLOCK flag set. This is necessary because not doing so on AIX
results in the child process never really dying, for some reason.
The resulting processes appears as unkillable zombies:

    - 483384            - <exiting>

Only a reboot allowed us to get rid of them.  We tried very hard to
find out why they never died, but we couldn't figure it out. As far
as we can tell, we do everything the way we're supposed to.

Anyway, the attached patch enhances GDB to handle pseudo terminals
configured that way. Before this patch, you'd see something like this
happening:

        (gdb) start
        The program being debugged has been started already.
        Start it from the beginning
        ? (y or n) y
 !!!->  EOF [assumed Y]
        Breakpoint 2 at 0x100154f0: file foo.adb, line 4.
        Starting program: /dresden.a/brobecke/ex/foo
        foo () at foo.adb:4
        4       end Foo;
        
What happens is that, by the time GDB makes a read on the pseudo-
terminal, there is nothing to read yet, and thus gets EOF back.
The thing to do, to detect our case, is to check the errno value,
and try again a little later if it's EAGAIN.

2009-04-23  Joel Brobecker   <brobecker@adacore.com>

        * utils.c: Add include of gdb_usleep.h.
        (defaulted_query): Detect false EOF conditions that happen
        on terminals opened with the O_NONBLOCK flag when there is
        nothing to read.

We've tested this patch on all the hosts that AdaCore supports.
This includes GNU/Linux, Solaris, AIX, Tru64, MinGW...
Would it be OK to commit?

-- 
Joel

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

diff --git a/gdb/utils.c b/gdb/utils.c
index 57f267a..175b2fc 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -67,6 +67,8 @@
 #include <sys/time.h>
 #include <time.h>
 
+#include "gdb_usleep.h"
+
 #if !HAVE_DECL_MALLOC
 extern PTR malloc ();		/* OK: PTR */
 #endif
@@ -1477,6 +1479,20 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args)
       gdb_flush (gdb_stdout);
 
       answer = fgetc (stdin);
+
+      /* On terminals opened with the NONBLOCK flag, fgetc can return EOF
+         when there is nothing to read.  To distinguish this case from
+         a true EOF, we need to check the file error condition and see
+         if fgetc might have set errno to EAGAIN.  */
+      while (answer == EOF && ferror (stdin) && errno == EAGAIN)
+        {
+          /* Not a real EOF.  Wait a little while and try again until
+             we read something.  */
+          clearerr (stdin);
+          gdb_usleep (10000);
+          answer = fgetc (stdin);
+        }
+
       clearerr (stdin);		/* in case of C-d */
       if (answer == EOF)	/* C-d */
 	{

Re: [RFA/commit] Handle EOF on terminals opened with ENONBLOCK...

[Mark Kettenis]

> Date: Thu, 23 Apr 2009 12:14:46 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> What happens is that, by the time GDB makes a read on the pseudo-
> terminal, there is nothing to read yet, and thus gets EOF back.
> The thing to do, to detect our case, is to check the errno value,
> and try again a little later if it's EAGAIN.
> 
> 2009-04-23  Joel Brobecker   <brobecker@adacore.com>
> 
>         * utils.c: Add include of gdb_usleep.h.
>         (defaulted_query): Detect false EOF conditions that happen
>         on terminals opened with the O_NONBLOCK flag when there is
>         nothing to read.
> 
> We've tested this patch on all the hosts that AdaCore supports.
> This includes GNU/Linux, Solaris, AIX, Tru64, MinGW...
> Would it be OK to commit?

Ugh, yuck, this is gross!

I suppose the errno == EAGAIN is supposed to check whether fgetc()
failed.  However, your code has an ferror() in between, which could
clobber errno.  So perhaps it is better to reverse those checks.

Re: [RFA/commit] Handle EOF on terminals opened with ENONBLOCK...

[Joel Brobecker]

> I suppose the errno == EAGAIN is supposed to check whether fgetc()
> failed.  However, your code has an ferror() in between, which could
> clobber errno.  So perhaps it is better to reverse those checks.

I did it in that order because I wanted to test the error condition
on the FILE before actually checking errno. The C99 draft that I have
does not say anything about errno, but the man page on my GNU/Linux
machine does say explicitly that errno doesn't get set during
a call to ferror.

I can try reversing them, though... Let me know if you think I should.

-- 
Joel

Re: [RFA/commit] Handle EOF on terminals opened with ENONBLOCK...

[Mark Kettenis]

> Date: Thu, 23 Apr 2009 13:37:10 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > I suppose the errno == EAGAIN is supposed to check whether fgetc()
> > failed.  However, your code has an ferror() in between, which could
> > clobber errno.  So perhaps it is better to reverse those checks.
> 
> I did it in that order because I wanted to test the error condition
> on the FILE before actually checking errno. The C99 draft that I have
> does not say anything about errno, but the man page on my GNU/Linux
> machine does say explicitly that errno doesn't get set during
> a call to ferror.
> 
> I can try reversing them, though... Let me know if you think I should.

Hmm, looking at the OpenBSD and Solaris implementations of ferror(3),
they both are just checking a flag. so what you currently have should
be safe.

Re: [RFA/commit] Handle EOF on terminals opened with ENONBLOCK...

[Eli Zaretskii]

> Date: Thu, 23 Apr 2009 12:14:46 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> 2009-04-23  Joel Brobecker   <brobecker@adacore.com>
> 
>         * utils.c: Add include of gdb_usleep.h.
>         (defaulted_query): Detect false EOF conditions that happen
>         on terminals opened with the O_NONBLOCK flag when there is
>         nothing to read.
> 
> We've tested this patch on all the hosts that AdaCore supports.
> This includes GNU/Linux, Solaris, AIX, Tru64, MinGW...
> Would it be OK to commit?

Fine with me, but I'd prefer that you tell more details in the
comment, and/or maybe add a link to this thread of discussion.  As
written, the comment doesn't give any clues, not even to what OS was
that discovered on.

Thanks.

Re: [RFA/commit] Handle EOF on terminals opened with ENONBLOCK...

[Joel Brobecker]

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

> 2009-04-23  Joel Brobecker   <brobecker@adacore.com>
> 
>         * utils.c: Add include of gdb_usleep.h.
>         (defaulted_query): Detect false EOF conditions that happen
>         on terminals opened with the O_NONBLOCK flag when there is
>         nothing to read.

This is what I ended up checking in. Eli, is the comment clearer
for you?

-- 
Joel

[-- Attachment #2: utils.c.diff --]
[-- Type: text/x-diff, Size: 1418 bytes --]

Index: utils.c
===================================================================
RCS file: /cvs/src/src/gdb/utils.c,v
retrieving revision 1.210
diff -u -p -r1.210 utils.c
--- utils.c	24 Apr 2009 22:10:03 -0000	1.210
+++ utils.c	6 May 2009 22:51:37 -0000
@@ -67,6 +67,8 @@
 #include <sys/time.h>
 #include <time.h>
 
+#include "gdb_usleep.h"
+
 #if !HAVE_DECL_MALLOC
 extern PTR malloc ();		/* ARI: PTR */
 #endif
@@ -1477,6 +1479,25 @@ defaulted_query (const char *ctlstr, con
       gdb_flush (gdb_stdout);
 
       answer = fgetc (stdin);
+
+      /* We expect fgetc to block until a character is read.  But
+         this may not be the case if the terminal was opened with
+         the NONBLOCK flag.  In that case, if there is nothing to
+         read on stdin, fgetc returns EOF, but also sets the error
+         condition flag on stdin and errno to EAGAIN.  With a true
+         EOF, stdin's error condition flag is not set.
+
+         A situation where this behavior was observed is a pseudo
+         terminal on AIX.  */
+      while (answer == EOF && ferror (stdin) && errno == EAGAIN)
+        {
+          /* Not a real EOF.  Wait a little while and try again until
+             we read something.  */
+          clearerr (stdin);
+          gdb_usleep (10000);
+          answer = fgetc (stdin);
+        }
+
       clearerr (stdin);		/* in case of C-d */
       if (answer == EOF)	/* C-d */
 	{

Re: [RFA/commit] Handle EOF on terminals opened with ENONBLOCK...

[Eli Zaretskii]

> Date: Wed, 6 May 2009 16:02:02 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > 2009-04-23  Joel Brobecker   <brobecker@adacore.com>
> > 
> >         * utils.c: Add include of gdb_usleep.h.
> >         (defaulted_query): Detect false EOF conditions that happen
> >         on terminals opened with the O_NONBLOCK flag when there is
> >         nothing to read.
> 
> This is what I ended up checking in. Eli, is the comment clearer
> for you?

Yes, thank you.