[PATCH] PPC sim: Don't close file descriptors 0, 1, or 2

[PATCH] PPC sim: Don't close file descriptors 0, 1, or 2

[Kevin Buettner]

In my testing, this patch eliminates all of the "unresolved testcases"
when testing GDB against the powerpc simulator.

E.g. for gdb.base.exp, I see this in the log file:

    (gdb) next
    66	    return (value); /* set breakpoint 19 here */
    (gdb) PASS: gdb.base/break.exp: next over recursive call
    backtrace
    #0  factorial (value=120) at testsuite/gdb.base/break.c:66
    #1  0x10000428 in factorial (value=6) at testsuite/gdb.base/break.c:64
    #2  0x10000370 in main (argc=1, argv=0xdfffee20, envp=0xdfffee28) at testsuite/gdb.base/break.c:47
    (gdb) PASS: gdb.base/break.exp: backtrace from factorial(5.1)
    continue
    Continuing.
    720
    ERROR: Process no longer exists
    UNRESOLVED: gdb.base/break.exp: continue until exit at recursive next test

With this patch/change in place, this bit of the test runs as expected:

    (gdb) next
    66	    return (value); /* set breakpoint 19 here */
    (gdb) PASS: gdb.base/break.exp: next over recursive call
    backtrace
    #0  factorial (value=120) at testsuite/gdb.base/break.c:66
    #1  0x10000428 in factorial (value=6) at testsuite/gdb.base/break.c:64
    #2  0x10000370 in main (argc=1, argv=0xdfffee20, envp=0xdfffee28) at testsuite/gdb.base/break.c:47
    (gdb) PASS: gdb.base/break.exp: backtrace from factorial(5.1)
    continue
    Continuing.
    720
    [Inferior 1 (process 42000) exited normally]

This occurs because the powerpc simulator closes, on behalf of the
testcase, the file descriptors associated with stdin, stdout, and
stderr.  GDB still needs these descriptors to communicate with the
user or, in this case, with the testing framework.

sim/ppc/ChangeLog:
    
    	* emul_netbsd.c (do_close): Don't close file descriptors 0, 1,
    	or 2.
---
 sim/ppc/emul_netbsd.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/sim/ppc/emul_netbsd.c b/sim/ppc/emul_netbsd.c
index 6bef370..5e39975 100644
--- a/sim/ppc/emul_netbsd.c
+++ b/sim/ppc/emul_netbsd.c
@@ -440,8 +440,13 @@ do_close(os_emul_data *emul,
 
   SYS(close);
 
-  /* Can't combine these statements, cuz close sets errno. */
-  status = close(d);
+  /* Do not close stdin, stdout, or stderr. GDB may still need access to
+     these descriptors.  */
+  if (d == 0 || d == 1 || d == 2)
+    status = 0;
+  else
+    status = close(d);
+
   emul_write_status(processor, status, errno);
 }
 

Re: [PATCH] PPC sim: Don't close file descriptors 0, 1, or 2

[Mike Frysinger]

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

On 16 Nov 2015 14:58, Kevin Buettner wrote:
> This occurs because the powerpc simulator closes, on behalf of the
> testcase, the file descriptors associated with stdin, stdout, and
> stderr.  GDB still needs these descriptors to communicate with the
> user or, in this case, with the testing framework.

special casing this logic in the sim looks wrong to me.  why
is the testcase itself trying to close stdin/stdout/stderr ?
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] PPC sim: Don't close file descriptors 0, 1, or 2

[Kevin Buettner]

On Mon, 16 Nov 2015 18:53:17 -0500
Mike Frysinger <vapier@gentoo.org> wrote:

> On 16 Nov 2015 14:58, Kevin Buettner wrote:
> > This occurs because the powerpc simulator closes, on behalf of the
> > testcase, the file descriptors associated with stdin, stdout, and
> > stderr.  GDB still needs these descriptors to communicate with the
> > user or, in this case, with the testing framework.
> 
> special casing this logic in the sim looks wrong to me.  why
> is the testcase itself trying to close stdin/stdout/stderr ?

It's not just one test case, but a bunch of them, 229 to be exact.

I haven't investigated all of them, but in many (if not all) of them,
close() is being called during exit().  This problem arises when
GDB runs the testcase to completion.

sim/common achieves the same result by placing file descriptors 0, 1,
2, and MAX_CALLBACK_FDS together in a circular fd_buddy list.  (See
os_init() in sim/common/callback.c.) close() is never called on any of
these descriptors due to the fact that they're in a (circular) list of
greater than one element.  When one of these descriptors (0, 1, or 2)
is closed (in os_close()), it is simply removed from the fd_buddy
list, with no close() operation actually being performed.  Note that
when the final one is "closed", it is still on an fd_buddy list with
MAX_CALLBACK_FDS - hence it won't be closed either.

So sim/common is doing the same thing as my proposed patch for ppc;
sim/common is just using a more elegant mechanism to avoid calling
close() on these three file descriptors.

Kevin

Re: [PATCH] PPC sim: Don't close file descriptors 0, 1, or 2

[Mike Frysinger]

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

On 16 Nov 2015 21:05, Kevin Buettner wrote:
> On Mon, 16 Nov 2015 18:53:17 -0500 Mike Frysinger wrote:
> > On 16 Nov 2015 14:58, Kevin Buettner wrote:
> > > This occurs because the powerpc simulator closes, on behalf of the
> > > testcase, the file descriptors associated with stdin, stdout, and
> > > stderr.  GDB still needs these descriptors to communicate with the
> > > user or, in this case, with the testing framework.
> > 
> > special casing this logic in the sim looks wrong to me.  why
> > is the testcase itself trying to close stdin/stdout/stderr ?
> 
> It's not just one test case, but a bunch of them, 229 to be exact.
> 
> I haven't investigated all of them, but in many (if not all) of them,
> close() is being called during exit().  This problem arises when
> GDB runs the testcase to completion.

sounds like that code is broken then ...

> sim/common achieves the same result by placing file descriptors 0, 1,
> 2, and MAX_CALLBACK_FDS together in a circular fd_buddy list.  (See
> os_init() in sim/common/callback.c.) close() is never called on any of
> these descriptors due to the fact that they're in a (circular) list of
> greater than one element.  When one of these descriptors (0, 1, or 2)
> is closed (in os_close()), it is simply removed from the fd_buddy
> list, with no close() operation actually being performed.  Note that
> when the final one is "closed", it is still on an fd_buddy list with
> MAX_CALLBACK_FDS - hence it won't be closed either.
> 
> So sim/common is doing the same thing as my proposed patch for ppc;
> sim/common is just using a more elegant mechanism to avoid calling
> close() on these three file descriptors.

the difference is that this code sequence misbehaves after your change:
	close(1);
	write(1, "foo", 3);
under the common sim, the write will return EBADF.

considering how much of common/ came from ppc/ i'm a little surprised
virtualization of the fd table didn't.

it would be nice if we could at least hide these three fds (a static
array of 3 bools maybe?), but i won't push hard for you to do that.
you should however be making the same change to the other emulation
layers.  you've done it only to netbsd, but the ppc sim has the same
code for the other layers too (like unix, and ...).
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] PPC sim: Don't close file descriptors 0, 1, or 2

[Kevin Buettner]

On Tue, 17 Nov 2015 00:41:33 -0500
Mike Frysinger <vapier@gentoo.org> wrote:

> > So sim/common is doing the same thing as my proposed patch for ppc;
> > sim/common is just using a more elegant mechanism to avoid calling
> > close() on these three file descriptors.
> 
> the difference is that this code sequence misbehaves after your change:
> 	close(1);
> 	write(1, "foo", 3);
> under the common sim, the write will return EBADF.
> 
> considering how much of common/ came from ppc/ i'm a little surprised
> virtualization of the fd table didn't.
> 
> it would be nice if we could at least hide these three fds (a static
> array of 3 bools maybe?), but i won't push hard for you to do that.

Do you mean an array which indicates the open / closed status of each
of stdin, stdout, and stderr?  This status would then be used to
return EBADF in the right places when the descriptor is "closed".

If so, I'm willing to pursue this, but I want to make sure that I
understand what you want.

> you should however be making the same change to the other emulation
> layers.  you've done it only to netbsd, but the ppc sim has the same
> code for the other layers too (like unix, and ...).

I've done this for emul_unix.  I've looked at emul_bugapi.c, emul_chirp.c,
and emul_generic.c, but it doesn't appear that any of these call close().

Here's the revised patch...

    PPC sim: Don't close file descriptors 0, 1, or 2.
    
    In my testing, this patch eliminates all of the "unresolved
    testcases", 229 in current upstream GDB, when testing GDB against the
    powerpc simulator.
    
    E.g. for gdb.base/break.exp, I see this in the log file:
    
        (gdb) next
        66	    return (value); /* set breakpoint 19 here */
        (gdb) PASS: gdb.base/break.exp: next over recursive call
        backtrace
        #0  factorial (value=120) at testsuite/gdb.base/break.c:66
        #1  0x10000428 in factorial (value=6) at testsuite/gdb.base/break.c:64
        #2  0x10000370 in main (argc=1, argv=0xdfffee20, envp=0xdfffee28) at testsuite/gdb.base/break.c:47
        (gdb) PASS: gdb.base/break.exp: backtrace from factorial(5.1)
        continue
        Continuing.
        720
        ERROR: Process no longer exists
        UNRESOLVED: gdb.base/break.exp: continue until exit at recursive next test
    
    With this patch/change in place, this bit of the test runs as expected:
    
        (gdb) next
        66	    return (value); /* set breakpoint 19 here */
        (gdb) PASS: gdb.base/break.exp: next over recursive call
        backtrace
        #0  factorial (value=120) at testsuite/gdb.base/break.c:66
        #1  0x10000428 in factorial (value=6) at testsuite/gdb.base/break.c:64
        #2  0x10000370 in main (argc=1, argv=0xdfffee20, envp=0xdfffee28) at testsuite/gdb.base/break.c:47
        (gdb) PASS: gdb.base/break.exp: backtrace from factorial(5.1)
        continue
        Continuing.
        720
        [Inferior 1 (process 42000) exited normally]
    
    This occurs because the powerpc simulator closes, on behalf of the
    testcase, the file descriptors associated with stdin, stdout, and
    stderr.  GDB still needs these descriptors to communicate with the
    user or, in this case, with the testing framework.
    
    I should also note that in the most (if not all) instances where these
    descriptors are closed, they are closed as part of exit() when GDB
    allows the inferior to run to completion.
    
    sim/ppc/ChangeLog:
    
    	* emul_netbsd.c (do_close): Don't close file descriptors 0, 1,
    	or 2.
    	* emul_unix.c (do_unix_close): Likewise.
---
 sim/ppc/emul_netbsd.c | 9 +++++++--
 sim/ppc/emul_unix.c   | 6 +++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/sim/ppc/emul_netbsd.c b/sim/ppc/emul_netbsd.c
index 6bef370..5e39975 100644
--- a/sim/ppc/emul_netbsd.c
+++ b/sim/ppc/emul_netbsd.c
@@ -440,8 +440,13 @@ do_close(os_emul_data *emul,
 
   SYS(close);
 
-  /* Can't combine these statements, cuz close sets errno. */
-  status = close(d);
+  /* Do not close stdin, stdout, or stderr. GDB may still need access to
+     these descriptors.  */
+  if (d == 0 || d == 1 || d == 2)
+    status = 0;
+  else
+    status = close(d);
+
   emul_write_status(processor, status, errno);
 }
 
diff --git a/sim/ppc/emul_unix.c b/sim/ppc/emul_unix.c
index d72525d..47b2b98 100644
--- a/sim/ppc/emul_unix.c
+++ b/sim/ppc/emul_unix.c
@@ -310,7 +310,11 @@ do_unix_close(os_emul_data *emul,
   if (WITH_TRACE && ppc_trace[trace_os_emul])
     printf_filtered ("%d", d);
 
-  status = close(d);
+  if (d == 0 || d == 1 || d == 2)
+    status = 0;
+  else
+    status = close(d);
+
   emul_write_status(processor, status, errno);
 }
 

Re: [PATCH] PPC sim: Don't close file descriptors 0, 1, or 2

[Mike Frysinger]

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

On 17 Nov 2015 13:31, Kevin Buettner wrote:
> On Tue, 17 Nov 2015 00:41:33 -0500 Mike Frysinger wrote:
> > > So sim/common is doing the same thing as my proposed patch for ppc;
> > > sim/common is just using a more elegant mechanism to avoid calling
> > > close() on these three file descriptors.
> > 
> > the difference is that this code sequence misbehaves after your change:
> > 	close(1);
> > 	write(1, "foo", 3);
> > under the common sim, the write will return EBADF.
> > 
> > considering how much of common/ came from ppc/ i'm a little surprised
> > virtualization of the fd table didn't.
> > 
> > it would be nice if we could at least hide these three fds (a static
> > array of 3 bools maybe?), but i won't push hard for you to do that.
> 
> Do you mean an array which indicates the open / closed status of each
> of stdin, stdout, and stderr?  This status would then be used to
> return EBADF in the right places when the descriptor is "closed".

correct.  maybe add a helper func like the common code does, and then
have every wrapper (read, write, etc...) check that before making the
actual call.  i don't think we have to get fancy and preserve exact
behavior since the standard does not require it; i.e. this code:
	close(2)
	int fd = open(...)
fd would normally be 2, but since we haven't really closed it, you'd
get back a higher fd.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] PPC sim: Don't close file descriptors 0, 1, or 2

[Kevin Buettner]

On Tue, 17 Nov 2015 16:05:40 -0500
Mike Frysinger <vapier@gentoo.org> wrote:

> On 17 Nov 2015 13:31, Kevin Buettner wrote:
> > On Tue, 17 Nov 2015 00:41:33 -0500 Mike Frysinger wrote:
> > > > So sim/common is doing the same thing as my proposed patch for ppc;
> > > > sim/common is just using a more elegant mechanism to avoid calling
> > > > close() on these three file descriptors.
> > > 
> > > the difference is that this code sequence misbehaves after your change:
> > > 	close(1);
> > > 	write(1, "foo", 3);
> > > under the common sim, the write will return EBADF.
> > > 
> > > considering how much of common/ came from ppc/ i'm a little surprised
> > > virtualization of the fd table didn't.
> > > 
> > > it would be nice if we could at least hide these three fds (a static
> > > array of 3 bools maybe?), but i won't push hard for you to do that.
> > 
> > Do you mean an array which indicates the open / closed status of each
> > of stdin, stdout, and stderr?  This status would then be used to
> > return EBADF in the right places when the descriptor is "closed".
> 
> correct.  maybe add a helper func like the common code does, and then
> have every wrapper (read, write, etc...) check that before making the
> actual call.  i don't think we have to get fancy and preserve exact
> behavior since the standard does not require it; i.e. this code:
> 	close(2)
> 	int fd = open(...)
> fd would normally be 2, but since we haven't really closed it, you'd
> get back a higher fd.

Below is a patch which adds and uses the helper function, fdbad, which
mimics (as much as makes sense) what is done in sim/common.

This adds on to my earlier patch:

https://sourceware.org/ml/gdb-patches/2015-11/msg00354.html

I.e. my earlier patch must be applied first before applying the patch
below.

    ppc sim: Track closed state of file descriptors 0, 1, and 2.
    
    This change tracks the "closed" state of file descriptors 0, 1, and 2,
    introducing the function fdbad() to emul_netbsd.c and emul_unix.c.
    Note that a function of the same name and purpose exists in
    sim/common/callback.c.
    
    sim/ppc/ChangeLog:
    
    	* emul_netbsd.c (fd_closed): New static array.
    	(fdbad): New function.
    	(do_read, do_write, do_close, do_dup, do_ioctl, do_dup2, do_fcntl)
    	(do_fstatfs, do_fstat, do_lseek): Call `fdbad'.
    	(emul_netbsd_init): Initialize `fd_closed'.
    	* emul_unix.c (fd_closed): New static array.
    	(fdbad): New function.
    	(do_unix_read, do_unix_write, do_unix_close, do_unix_dup)
    	(do_unix_dup2, do_unix_lseek, do_solaris_fstat, do_solaris_ioctl)
    	(do_linux_fstat, do_linux_ioctl): Call `fdbad'.
    	(emul_solaris_init, emul_linux_init): Initialize `fd_closed'.
---
 sim/ppc/emul_netbsd.c | 81 +++++++++++++++++++++++++++++++++++++++++----------
 sim/ppc/emul_unix.c   | 79 ++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 131 insertions(+), 29 deletions(-)

diff --git a/sim/ppc/emul_netbsd.c b/sim/ppc/emul_netbsd.c
index 5e39975..c50fea0 100644
--- a/sim/ppc/emul_netbsd.c
+++ b/sim/ppc/emul_netbsd.c
@@ -292,6 +292,31 @@ write_rusage(unsigned_word addr,
 }
 #endif
 
+
+/* File descriptors 0, 1, and 2 should not be closed.  fd_closed[]
+   tracks whether these descriptors have been closed in do_close()
+   below.  */
+
+static int fd_closed[3];
+
+/* Check for some occurrences of bad file descriptors.  We only check
+   whether fd 0, 1, or 2 are "closed".  By "closed" we mean that these
+   descriptors aren't actually closed, but are considered to be closed
+   by this layer.
+   
+   Other checks are performed by the underlying OS call.  */
+
+static int
+fdbad (int fd)
+{
+  if (fd >=0 && fd <= 2 && fd_closed[fd])
+    {
+      errno = EBADF;
+      return -1;
+    }
+  return 0;
+}
+
 static void
 do_exit(os_emul_data *emul,
 	unsigned call,
@@ -339,7 +364,9 @@ do_read(os_emul_data *emul,
       status = -1;
   }
 #endif
-  status = read (d, scratch_buffer, nbytes);
+  status = fdbad (d);
+  if (status == 0)
+    status = read (d, scratch_buffer, nbytes);
 
   emul_write_status(processor, status, errno);
   if (status > 0)
@@ -374,7 +401,10 @@ do_write(os_emul_data *emul,
 		   processor, cia);
 
   /* write */
-  status = write(d, scratch_buffer, nbytes);
+  status = fdbad (d);
+  if (status == 0)
+    status = write(d, scratch_buffer, nbytes);
+
   emul_write_status(processor, status, errno);
   free(scratch_buffer);
 
@@ -440,12 +470,19 @@ do_close(os_emul_data *emul,
 
   SYS(close);
 
-  /* Do not close stdin, stdout, or stderr. GDB may still need access to
-     these descriptors.  */
-  if (d == 0 || d == 1 || d == 2)
-    status = 0;
-  else
-    status = close(d);
+  status = fdbad (d);
+  if (status == 0)
+    {
+      /* Do not close stdin, stdout, or stderr. GDB may still need access to
+	 these descriptors.  */
+      if (d == 0 || d == 1 || d == 2)
+        {
+	  fd_closed[d] = 1;
+	  status = 0;
+	}
+      else
+	status = close(d);
+    }
 
   emul_write_status(processor, status, errno);
 }
@@ -554,7 +591,7 @@ do_dup(os_emul_data *emul,
        unsigned_word cia)
 {
   int oldd = cpu_registers(processor)->gpr[arg0];
-  int status = dup(oldd);
+  int status = (fdbad (oldd) < 0) ? -1 : dup(oldd);
   int err = errno;
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -645,7 +682,9 @@ do_ioctl(os_emul_data *emul,
       || dir & IOC_OUT
       || !(dir & IOC_VOID))
     error("do_ioctl() read or write of parameter not implemented\n");
-  status = ioctl(d, request, NULL);
+  status = fdbad (d);
+  if (status == 0)
+    status = ioctl(d, request, NULL);
   emul_write_status(processor, status, errno);
 #endif
 
@@ -686,7 +725,7 @@ do_dup2(os_emul_data *emul,
 {
   int oldd = cpu_registers(processor)->gpr[arg0];
   int newd = cpu_registers(processor)->gpr[arg0+1];
-  int status = dup2(oldd, newd);
+  int status = (fdbad (oldd) < 0) ? -1 : dup2(oldd, newd);
   int err = errno;
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -716,7 +755,9 @@ do_fcntl(os_emul_data *emul,
     printf_filtered ("%d, %d, %d", fd, cmd, arg);
 
   SYS(fcntl);
-  status = fcntl(fd, cmd, arg);
+  status = fdbad (fd);
+  if (status == 0)
+    status = fcntl(fd, cmd, arg);
   emul_write_status(processor, status, errno);
 }
 #endif
@@ -801,7 +842,9 @@ do_fstatfs(os_emul_data *emul,
     printf_filtered ("%d, 0x%lx", fd, (long)buf_addr);
 
   SYS(fstatfs);
-  status = fstatfs(fd, (buf_addr == 0 ? NULL : &buf));
+  status = fdbad (fd);
+  if (status == 0)
+    status = fstatfs(fd, (buf_addr == 0 ? NULL : &buf));
   emul_write_status(processor, status, errno);
   if (status == 0) {
     if (buf_addr != 0)
@@ -854,7 +897,9 @@ do_fstat(os_emul_data *emul,
   SYS(fstat);
 #endif
   /* Can't combine these statements, cuz fstat sets errno. */
-  status = fstat(fd, &buf);
+  status = fdbad (fd);
+  if (status == 0)
+    status = fstat(fd, &buf);
   emul_write_status(processor, status, errno);
   write_stat(stat_buf_addr, buf, processor, cia);
 }
@@ -956,7 +1001,9 @@ do_lseek(os_emul_data *emul,
   int whence = cpu_registers(processor)->gpr[arg0+4];
   off_t status;
   SYS(lseek);
-  status = lseek(fildes, offset, whence);
+  status = fdbad (fildes);
+  if (status == 0)
+    status = lseek(fildes, offset, whence);
   if (status == -1)
     emul_write_status(processor, -1, errno);
   else {
@@ -1455,7 +1502,9 @@ static void
 emul_netbsd_init(os_emul_data *emul_data,
 		 int nr_cpus)
 {
-  /* nothing yet */
+  fd_closed[0] = 0;
+  fd_closed[1] = 0;
+  fd_closed[2] = 0;
 }
 
 static void
diff --git a/sim/ppc/emul_unix.c b/sim/ppc/emul_unix.c
index 47b2b98..a884e24 100644
--- a/sim/ppc/emul_unix.c
+++ b/sim/ppc/emul_unix.c
@@ -195,6 +195,31 @@ struct	unix_rusage {
 };
 
 
+/* File descriptors 0, 1, and 2 should not be closed.  fd_closed[]
+   tracks whether these descriptors have been closed in do_close()
+   below.  */
+
+static int fd_closed[3];
+
+/* Check for some occurrences of bad file descriptors.  We only check
+   whether fd 0, 1, or 2 are "closed".  By "closed" we mean that these
+   descriptors aren't actually closed, but are considered to be closed
+   by this layer.
+   
+   Other checks are performed by the underlying OS call.  */
+
+static int
+fdbad (int fd)
+{
+  if (fd >=0 && fd <= 2 && fd_closed[fd])
+    {
+      errno = EBADF;
+      return -1;
+    }
+  return 0;
+}
+
+
 static void
 do_unix_exit(os_emul_data *emul,
 	     unsigned call,
@@ -232,8 +257,10 @@ do_unix_read(os_emul_data *emul,
   /* check if buffer exists by reading it */
   emul_read_buffer(scratch_buffer, buf, nbytes, processor, cia);
 
+  status = fdbad (d);
   /* read */
-  status = read (d, scratch_buffer, nbytes);
+  if (status == 0)
+    status = read (d, scratch_buffer, nbytes);
 
   emul_write_status(processor, status, errno);
   if (status > 0)
@@ -266,8 +293,10 @@ do_unix_write(os_emul_data *emul,
   emul_read_buffer(scratch_buffer, buf, nbytes,
 		   processor, cia);
 
+  status = fdbad (d);
   /* write */
-  status = write(d, scratch_buffer, nbytes);
+  if (status == 0)
+    status = write(d, scratch_buffer, nbytes);
   emul_write_status(processor, status, errno);
   free(scratch_buffer);
 
@@ -310,10 +339,14 @@ do_unix_close(os_emul_data *emul,
   if (WITH_TRACE && ppc_trace[trace_os_emul])
     printf_filtered ("%d", d);
 
-  if (d == 0 || d == 1 || d == 2)
-    status = 0;
-  else
-    status = close(d);
+  status = fdbad (d);
+  if (status == 0)
+    {
+      if (d == 0 || d == 1 || d == 2)
+	status = 0;
+      else
+	status = close(d);
+    }
 
   emul_write_status(processor, status, errno);
 }
@@ -494,7 +527,7 @@ do_unix_dup(os_emul_data *emul,
 	    unsigned_word cia)
 {
   int oldd = cpu_registers(processor)->gpr[arg0];
-  int status = dup(oldd);
+  int status = (fdbad (oldd) < 0) ? -1 : dup(oldd);
   int err = errno;
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -516,7 +549,7 @@ do_unix_dup2(os_emul_data *emul,
 {
   int oldd = cpu_registers(processor)->gpr[arg0];
   int newd = cpu_registers(processor)->gpr[arg0+1];
-  int status = dup2(oldd, newd);
+  int status = (fdbad (oldd) < 0) ? -1 : dup2(oldd, newd);
   int err = errno;
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -544,7 +577,9 @@ do_unix_lseek(os_emul_data *emul,
   if (WITH_TRACE && ppc_trace[trace_os_emul])
     printf_filtered ("%d %ld %d", fildes, (long)offset, whence);
 
-  status = lseek(fildes, offset, whence);
+  status = fdbad (fildes);
+  if (status == 0)
+    status = lseek(fildes, offset, whence);
   emul_write_status(processor, (int)status, errno);
 }
 #endif
@@ -1200,7 +1235,9 @@ do_solaris_fstat(os_emul_data *emul,
   if (WITH_TRACE && ppc_trace[trace_os_emul])
     printf_filtered ("%d, 0x%lx", fildes, (long)stat_pkt);
 
-  status = fstat (fildes, &buf);
+  status = fdbad (fildes);
+  if (status == 0)
+    status = fstat (fildes, &buf);
   if (status == 0)
     convert_to_solaris_stat (stat_pkt, &buf, processor, cia);
 
@@ -1437,6 +1474,10 @@ do_solaris_ioctl(os_emul_data *emul,
 #endif
 #endif
 
+  status = fdbad (fildes);
+  if (status != 0)
+    goto done;
+
   switch (request)
     {
     case 0:					/* make sure we have at least one case */
@@ -1478,6 +1519,7 @@ do_solaris_ioctl(os_emul_data *emul,
 #endif /* HAVE_TERMIOS_STRUCTURE */
     }
 
+done:
   emul_write_status(processor, status, errno);
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -1929,7 +1971,9 @@ static void
 emul_solaris_init(os_emul_data *emul_data,
 		  int nr_cpus)
 {
-  /* nothing yet */
+  fd_closed[0] = 0;
+  fd_closed[1] = 0;
+  fd_closed[2] = 0;
 }
 
 static void
@@ -2128,7 +2172,9 @@ do_linux_fstat(os_emul_data *emul,
   if (WITH_TRACE && ppc_trace[trace_os_emul])
     printf_filtered ("%d, 0x%lx", fildes, (long)stat_pkt);
 
-  status = fstat (fildes, &buf);
+  status = fdbad (fildes);
+  if (status == 0)
+    status = fstat (fildes, &buf);
   if (status == 0)
     convert_to_linux_stat (stat_pkt, &buf, processor, cia);
 
@@ -2392,6 +2438,10 @@ do_linux_ioctl(os_emul_data *emul,
 #endif
 #endif
 
+  status = fdbad (fildes);
+  if (status != 0)
+    goto done;
+
   switch (request)
     {
     case 0:					/* make sure we have at least one case */
@@ -2433,6 +2483,7 @@ do_linux_ioctl(os_emul_data *emul,
 #endif /* HAVE_TERMIOS_STRUCTURE */
     }
 
+done:
   emul_write_status(processor, status, errno);
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -2799,7 +2850,9 @@ static void
 emul_linux_init(os_emul_data *emul_data,
 		int nr_cpus)
 {
-  /* nothing yet */
+  fd_closed[0] = 0;
+  fd_closed[1] = 0;
+  fd_closed[2] = 0;
 }
 
 static void

Re: [PATCH] PPC sim: Don't close file descriptors 0, 1, or 2

[Mike Frysinger]

[-- Attachment #1.1: Type: text/plain, Size: 515 bytes --]

On 07 Dec 2015 13:29, Kevin Buettner wrote:
> Below is a patch which adds and uses the helper function, fdbad, which
> mimics (as much as makes sense) what is done in sim/common.
> 
> This adds on to my earlier patch:
> 
> https://sourceware.org/ml/gdb-patches/2015-11/msg00354.html
> 
> I.e. my earlier patch must be applied first before applying the patch
> below.

the linux code was slightly broken (forgot to set fd_close), but i fixed
that and pushed it.  final commit is below.  thanks !
-mike

[-- Attachment #1.2: 0001-sim-ppc-track-closed-state-of-file-descriptors-0-1-a.patch --]
[-- Type: text/x-diff, Size: 11900 bytes --]

From bd9de4340c0ae6c96b2d32df15a6f355154f05e2 Mon Sep 17 00:00:00 2001
From: Kevin Buettner <kevinb@redhat.com>
Date: Mon, 16 Nov 2015 14:58:07 -0700
Subject: [PATCH] sim: ppc: track closed state of file descriptors 0, 1, and 2.

This change tracks the "closed" state of file descriptors 0, 1, and 2,
introducing the function fdbad() to emul_netbsd.c and emul_unix.c.
Note that a function of the same name and purpose exists in
sim/common/callback.c.

This patch eliminates all of the "unresolved testcases" when testing
GDB against the powerpc simulator.

This occurs because the powerpc simulator closes, on behalf of the
testcase, the file descriptors associated with stdin, stdout, and
stderr.  GDB still needs these descriptors to communicate with the
user or, in this case, with the testing framework.
---
 sim/ppc/ChangeLog     | 14 +++++++++
 sim/ppc/emul_netbsd.c | 78 +++++++++++++++++++++++++++++++++++++++++--------
 sim/ppc/emul_unix.c   | 81 ++++++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 151 insertions(+), 22 deletions(-)

diff --git a/sim/ppc/ChangeLog b/sim/ppc/ChangeLog
index 413a99d..7ce9a47 100644
--- a/sim/ppc/ChangeLog
+++ b/sim/ppc/ChangeLog
@@ -1,3 +1,17 @@
+2015-12-29  Kevin Buettner  <kevinb@redhat.com>
+
+	* emul_netbsd.c (fd_closed): New static array.
+	(fdbad): New function.
+	(do_read, do_write, do_close, do_dup, do_ioctl, do_dup2, do_fcntl)
+	(do_fstatfs, do_fstat, do_lseek): Call `fdbad'.
+	(emul_netbsd_init): Initialize `fd_closed'.
+	* emul_unix.c (fd_closed): New static array.
+	(fdbad): New function.
+	(do_unix_read, do_unix_write, do_unix_close, do_unix_dup)
+	(do_unix_dup2, do_unix_lseek, do_solaris_fstat, do_solaris_ioctl)
+	(do_linux_fstat, do_linux_ioctl): Call `fdbad'.
+	(emul_solaris_init, emul_linux_init): Initialize `fd_closed'.
+
 2015-12-26  Mike Frysinger  <vapier@gentoo.org>
 
 	* Makefile.in (TCONFIG_H): Delete.
diff --git a/sim/ppc/emul_netbsd.c b/sim/ppc/emul_netbsd.c
index 6bef370..12dfb21 100644
--- a/sim/ppc/emul_netbsd.c
+++ b/sim/ppc/emul_netbsd.c
@@ -292,6 +292,31 @@ write_rusage(unsigned_word addr,
 }
 #endif
 
+
+/* File descriptors 0, 1, and 2 should not be closed.  fd_closed[]
+   tracks whether these descriptors have been closed in do_close()
+   below.  */
+
+static int fd_closed[3];
+
+/* Check for some occurrences of bad file descriptors.  We only check
+   whether fd 0, 1, or 2 are "closed".  By "closed" we mean that these
+   descriptors aren't actually closed, but are considered to be closed
+   by this layer.
+
+   Other checks are performed by the underlying OS call.  */
+
+static int
+fdbad (int fd)
+{
+  if (fd >=0 && fd <= 2 && fd_closed[fd])
+    {
+      errno = EBADF;
+      return -1;
+    }
+  return 0;
+}
+
 static void
 do_exit(os_emul_data *emul,
 	unsigned call,
@@ -339,7 +364,9 @@ do_read(os_emul_data *emul,
       status = -1;
   }
 #endif
-  status = read (d, scratch_buffer, nbytes);
+  status = fdbad (d);
+  if (status == 0)
+    status = read (d, scratch_buffer, nbytes);
 
   emul_write_status(processor, status, errno);
   if (status > 0)
@@ -374,7 +401,10 @@ do_write(os_emul_data *emul,
 		   processor, cia);
 
   /* write */
-  status = write(d, scratch_buffer, nbytes);
+  status = fdbad (d);
+  if (status == 0)
+    status = write(d, scratch_buffer, nbytes);
+
   emul_write_status(processor, status, errno);
   free(scratch_buffer);
 
@@ -440,8 +470,20 @@ do_close(os_emul_data *emul,
 
   SYS(close);
 
-  /* Can't combine these statements, cuz close sets errno. */
-  status = close(d);
+  status = fdbad (d);
+  if (status == 0)
+    {
+      /* Do not close stdin, stdout, or stderr. GDB may still need access to
+	 these descriptors.  */
+      if (d == 0 || d == 1 || d == 2)
+	{
+	  fd_closed[d] = 1;
+	  status = 0;
+	}
+      else
+	status = close(d);
+    }
+
   emul_write_status(processor, status, errno);
 }
 
@@ -549,7 +591,7 @@ do_dup(os_emul_data *emul,
        unsigned_word cia)
 {
   int oldd = cpu_registers(processor)->gpr[arg0];
-  int status = dup(oldd);
+  int status = (fdbad (oldd) < 0) ? -1 : dup(oldd);
   int err = errno;
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -640,7 +682,9 @@ do_ioctl(os_emul_data *emul,
       || dir & IOC_OUT
       || !(dir & IOC_VOID))
     error("do_ioctl() read or write of parameter not implemented\n");
-  status = ioctl(d, request, NULL);
+  status = fdbad (d);
+  if (status == 0)
+    status = ioctl(d, request, NULL);
   emul_write_status(processor, status, errno);
 #endif
 
@@ -681,7 +725,7 @@ do_dup2(os_emul_data *emul,
 {
   int oldd = cpu_registers(processor)->gpr[arg0];
   int newd = cpu_registers(processor)->gpr[arg0+1];
-  int status = dup2(oldd, newd);
+  int status = (fdbad (oldd) < 0) ? -1 : dup2(oldd, newd);
   int err = errno;
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -711,7 +755,9 @@ do_fcntl(os_emul_data *emul,
     printf_filtered ("%d, %d, %d", fd, cmd, arg);
 
   SYS(fcntl);
-  status = fcntl(fd, cmd, arg);
+  status = fdbad (fd);
+  if (status == 0)
+    status = fcntl(fd, cmd, arg);
   emul_write_status(processor, status, errno);
 }
 #endif
@@ -796,7 +842,9 @@ do_fstatfs(os_emul_data *emul,
     printf_filtered ("%d, 0x%lx", fd, (long)buf_addr);
 
   SYS(fstatfs);
-  status = fstatfs(fd, (buf_addr == 0 ? NULL : &buf));
+  status = fdbad (fd);
+  if (status == 0)
+    status = fstatfs(fd, (buf_addr == 0 ? NULL : &buf));
   emul_write_status(processor, status, errno);
   if (status == 0) {
     if (buf_addr != 0)
@@ -849,7 +897,9 @@ do_fstat(os_emul_data *emul,
   SYS(fstat);
 #endif
   /* Can't combine these statements, cuz fstat sets errno. */
-  status = fstat(fd, &buf);
+  status = fdbad (fd);
+  if (status == 0)
+    status = fstat(fd, &buf);
   emul_write_status(processor, status, errno);
   write_stat(stat_buf_addr, buf, processor, cia);
 }
@@ -951,7 +1001,9 @@ do_lseek(os_emul_data *emul,
   int whence = cpu_registers(processor)->gpr[arg0+4];
   off_t status;
   SYS(lseek);
-  status = lseek(fildes, offset, whence);
+  status = fdbad (fildes);
+  if (status == 0)
+    status = lseek(fildes, offset, whence);
   if (status == -1)
     emul_write_status(processor, -1, errno);
   else {
@@ -1450,7 +1502,9 @@ static void
 emul_netbsd_init(os_emul_data *emul_data,
 		 int nr_cpus)
 {
-  /* nothing yet */
+  fd_closed[0] = 0;
+  fd_closed[1] = 0;
+  fd_closed[2] = 0;
 }
 
 static void
diff --git a/sim/ppc/emul_unix.c b/sim/ppc/emul_unix.c
index d72525d..1475474 100644
--- a/sim/ppc/emul_unix.c
+++ b/sim/ppc/emul_unix.c
@@ -195,6 +195,30 @@ struct	unix_rusage {
 };
 
 
+/* File descriptors 0, 1, and 2 should not be closed.  fd_closed[]
+   tracks whether these descriptors have been closed in do_close()
+   below.  */
+
+static int fd_closed[3];
+
+/* Check for some occurrences of bad file descriptors.  We only check
+   whether fd 0, 1, or 2 are "closed".  By "closed" we mean that these
+   descriptors aren't actually closed, but are considered to be closed
+   by this layer.
+
+   Other checks are performed by the underlying OS call.  */
+
+static int
+fdbad (int fd)
+{
+  if (fd >=0 && fd <= 2 && fd_closed[fd])
+    {
+      errno = EBADF;
+      return -1;
+    }
+  return 0;
+}
+
 static void
 do_unix_exit(os_emul_data *emul,
 	     unsigned call,
@@ -232,8 +256,10 @@ do_unix_read(os_emul_data *emul,
   /* check if buffer exists by reading it */
   emul_read_buffer(scratch_buffer, buf, nbytes, processor, cia);
 
+  status = fdbad (d);
   /* read */
-  status = read (d, scratch_buffer, nbytes);
+  if (status == 0)
+    status = read (d, scratch_buffer, nbytes);
 
   emul_write_status(processor, status, errno);
   if (status > 0)
@@ -266,8 +292,10 @@ do_unix_write(os_emul_data *emul,
   emul_read_buffer(scratch_buffer, buf, nbytes,
 		   processor, cia);
 
+  status = fdbad (d);
   /* write */
-  status = write(d, scratch_buffer, nbytes);
+  if (status == 0)
+    status = write(d, scratch_buffer, nbytes);
   emul_write_status(processor, status, errno);
   free(scratch_buffer);
 
@@ -310,7 +338,20 @@ do_unix_close(os_emul_data *emul,
   if (WITH_TRACE && ppc_trace[trace_os_emul])
     printf_filtered ("%d", d);
 
-  status = close(d);
+  status = fdbad (d);
+  if (status == 0)
+    {
+      /* Do not close stdin, stdout, or stderr. GDB may still need access to
+	 these descriptors.  */
+      if (d == 0 || d == 1 || d == 2)
+	{
+	  fd_closed[d] = 1;
+	  status = 0;
+	}
+      else
+	status = close(d);
+    }
+
   emul_write_status(processor, status, errno);
 }
 
@@ -490,7 +531,7 @@ do_unix_dup(os_emul_data *emul,
 	    unsigned_word cia)
 {
   int oldd = cpu_registers(processor)->gpr[arg0];
-  int status = dup(oldd);
+  int status = (fdbad (oldd) < 0) ? -1 : dup(oldd);
   int err = errno;
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -512,7 +553,7 @@ do_unix_dup2(os_emul_data *emul,
 {
   int oldd = cpu_registers(processor)->gpr[arg0];
   int newd = cpu_registers(processor)->gpr[arg0+1];
-  int status = dup2(oldd, newd);
+  int status = (fdbad (oldd) < 0) ? -1 : dup2(oldd, newd);
   int err = errno;
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -540,7 +581,9 @@ do_unix_lseek(os_emul_data *emul,
   if (WITH_TRACE && ppc_trace[trace_os_emul])
     printf_filtered ("%d %ld %d", fildes, (long)offset, whence);
 
-  status = lseek(fildes, offset, whence);
+  status = fdbad (fildes);
+  if (status == 0)
+    status = lseek(fildes, offset, whence);
   emul_write_status(processor, (int)status, errno);
 }
 #endif
@@ -1196,7 +1239,9 @@ do_solaris_fstat(os_emul_data *emul,
   if (WITH_TRACE && ppc_trace[trace_os_emul])
     printf_filtered ("%d, 0x%lx", fildes, (long)stat_pkt);
 
-  status = fstat (fildes, &buf);
+  status = fdbad (fildes);
+  if (status == 0)
+    status = fstat (fildes, &buf);
   if (status == 0)
     convert_to_solaris_stat (stat_pkt, &buf, processor, cia);
 
@@ -1433,6 +1478,10 @@ do_solaris_ioctl(os_emul_data *emul,
 #endif
 #endif
 
+  status = fdbad (fildes);
+  if (status != 0)
+    goto done;
+
   switch (request)
     {
     case 0:					/* make sure we have at least one case */
@@ -1474,6 +1523,7 @@ do_solaris_ioctl(os_emul_data *emul,
 #endif /* HAVE_TERMIOS_STRUCTURE */
     }
 
+done:
   emul_write_status(processor, status, errno);
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -1925,7 +1975,9 @@ static void
 emul_solaris_init(os_emul_data *emul_data,
 		  int nr_cpus)
 {
-  /* nothing yet */
+  fd_closed[0] = 0;
+  fd_closed[1] = 0;
+  fd_closed[2] = 0;
 }
 
 static void
@@ -2124,7 +2176,9 @@ do_linux_fstat(os_emul_data *emul,
   if (WITH_TRACE && ppc_trace[trace_os_emul])
     printf_filtered ("%d, 0x%lx", fildes, (long)stat_pkt);
 
-  status = fstat (fildes, &buf);
+  status = fdbad (fildes);
+  if (status == 0)
+    status = fstat (fildes, &buf);
   if (status == 0)
     convert_to_linux_stat (stat_pkt, &buf, processor, cia);
 
@@ -2388,6 +2442,10 @@ do_linux_ioctl(os_emul_data *emul,
 #endif
 #endif
 
+  status = fdbad (fildes);
+  if (status != 0)
+    goto done;
+
   switch (request)
     {
     case 0:					/* make sure we have at least one case */
@@ -2429,6 +2487,7 @@ do_linux_ioctl(os_emul_data *emul,
 #endif /* HAVE_TERMIOS_STRUCTURE */
     }
 
+done:
   emul_write_status(processor, status, errno);
 
   if (WITH_TRACE && ppc_trace[trace_os_emul])
@@ -2795,7 +2854,9 @@ static void
 emul_linux_init(os_emul_data *emul_data,
 		int nr_cpus)
 {
-  /* nothing yet */
+  fd_closed[0] = 0;
+  fd_closed[1] = 0;
+  fd_closed[2] = 0;
 }
 
 static void
-- 
2.6.2


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]