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

Message ID 20151116145807.4aedd84f@pinnacle.lan
State Superseded
Delegated to: Mike Frysinger
Headers

Commit Message

Kevin Buettner Nov. 16, 2015, 9:58 p.m. UTC
  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(-)
  

Comments

Mike Frysinger Nov. 16, 2015, 11:53 p.m. UTC | #1
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
  
Kevin Buettner Nov. 17, 2015, 4:05 a.m. UTC | #2
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
  
Mike Frysinger Nov. 17, 2015, 5:41 a.m. UTC | #3
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
  

Patch

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);
 }