From patchwork Tue Nov 17 20:31:54 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Buettner X-Patchwork-Id: 9712 X-Patchwork-Delegate: vapier@gentoo.org Received: (qmail 103289 invoked by alias); 17 Nov 2015 20:32:02 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 103264 invoked by uid 89); 17 Nov 2015 20:32:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.7 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 17 Nov 2015 20:32:00 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 1FB90C10045D for ; Tue, 17 Nov 2015 20:31:59 +0000 (UTC) Received: from pinnacle.lan (ovpn-113-161.phx2.redhat.com [10.3.113.161]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tAHKVwrA029110 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA256 bits=256 verify=NO) for ; Tue, 17 Nov 2015 15:31:58 -0500 Date: Tue, 17 Nov 2015 13:31:54 -0700 From: Kevin Buettner To: gdb-patches@sourceware.org Subject: Re: [PATCH] PPC sim: Don't close file descriptors 0, 1, or 2 Message-ID: <20151117133154.72b61a52@pinnacle.lan> In-Reply-To: <20151117054133.GI31395@vapier.lan> References: <20151116145807.4aedd84f@pinnacle.lan> <20151116235317.GF31395@vapier.lan> <20151116210533.058520d2@pinnacle.lan> <20151117054133.GI31395@vapier.lan> MIME-Version: 1.0 X-IsSubscribed: yes 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". 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); }