From ca35d41cec77e095bf08cc48109210295b28cc08 Mon Sep 17 00:00:00 2001 From: Christopher Faylor Date: Sat, 17 Dec 2011 00:03:31 +0000 Subject: [PATCH] Implement fhandler reference counting. * cygheap.h (cygheap_fdmanip::release): Make virtual. (cygheap_fdnew::~cygheap_fdnew): New destructor increments reference count when fd has been allocated. (cygheap_fdget::fh): New (old?) field. (cygheap_fdget::cygheap_fdget): Increment reference count when we've found an active fd. Set fh appropriately. (cygheap_fdget::~cygheap_fdget): Decrement reference count when appropriate. Delete fh if reference count goes to zero. (cygheap_fdget::release): New function. Do more bookkeping on release. * dtable.cc (dtable::release): Change from void to boolean return. Only delete the fhandler when its reference count is <= 0 (this should be a fairly unusual case). Return true if fhandler has been deleted. (cygwin_attach_handle_to_fd): Increment reference count when fh is assigned. (dtable::init_std_file_from_handle): Ditto. * dtable.h (dtable::release): Change return to boolean. * fhandler.cc (fhandler_base::fhandler_base): Set new isclosed flag to false. Set _refcnt to zero. (fhandler_base::close): Simplify paranoid debugging output. Set new isclosed() flag. (fhandler_base_overlapped::wait_overlapped): Use isclosed() flag to avoid querying the exception handle. * fhandler.h (fhandler_base::_refcnt): New field. (fhandler_base::refcnt): New function. (fhandler_base::isclosed): Implement. (fhandler_base::fhandler_base): Set isclosed to false. * syscalls.cc: Remove space after function before parentheses for several strace printfs. (dup): Add standard strace "leaver" code. (dup2): Ditto. (dup3): Ditto. (remove): Ditto. (getpid): Ditto. (getppid): Ditto. (lseek64): Fix strace debugging to correctly use %R. * fhandler_termios.cc (fhandler_termios::tcsetpgrp): Avoid sending signals to other processes if we're debugging since it can cause a deadlock with the calling debugger. * exceptions.cc (_cygtls::call_signal_handler): Add debugging-only strace output. --- winsup/cygwin/ChangeLog | 48 +++++++++++++++++++++++ winsup/cygwin/cygheap.h | 34 +++++++++++++--- winsup/cygwin/dtable.cc | 18 +++++++-- winsup/cygwin/dtable.h | 2 +- winsup/cygwin/exceptions.cc | 1 + winsup/cygwin/fhandler.cc | 11 +++--- winsup/cygwin/fhandler.h | 6 ++- winsup/cygwin/fhandler_termios.cc | 2 +- winsup/cygwin/syscalls.cc | 65 +++++++++++++++++-------------- 9 files changed, 141 insertions(+), 46 deletions(-) diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index 7167510b2..b5f5d82cf 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -1,3 +1,51 @@ +2011-12-16 Christopher Faylor + + Implement fhandler reference counting. + * cygheap.h + (cygheap_fdmanip::release): Make virtual. + (cygheap_fdnew::~cygheap_fdnew): New destructor increments reference + count when fd has been allocated. + (cygheap_fdget::fh): New (old?) field. + (cygheap_fdget::cygheap_fdget): Increment reference count when we've + found an active fd. Set fh appropriately. + (cygheap_fdget::~cygheap_fdget): Decrement reference count when + appropriate. Delete fh if reference count goes to zero. + (cygheap_fdget::release): New function. Do more bookkeping on release. + * dtable.cc (dtable::release): Change from void to boolean return. + Only delete the fhandler when its reference count is <= 0 (this should + be a fairly unusual case). Return true if fhandler has been deleted. + (cygwin_attach_handle_to_fd): Increment reference count when fh is + assigned. + (dtable::init_std_file_from_handle): Ditto. + * dtable.h (dtable::release): Change return to boolean. + * fhandler.cc (fhandler_base::fhandler_base): Set new isclosed flag to + false. Set _refcnt to zero. + (fhandler_base::close): Simplify paranoid debugging output. Set new + isclosed() flag. + (fhandler_base_overlapped::wait_overlapped): Use isclosed() flag to + avoid querying the exception handle. + * fhandler.h (fhandler_base::_refcnt): New field. + (fhandler_base::refcnt): New function. + (fhandler_base::isclosed): Implement. + (fhandler_base::fhandler_base): Set isclosed to false. + + * syscalls.cc: Remove space after function before parentheses for + several strace printfs. + (dup): Add standard strace "leaver" code. + (dup2): Ditto. + (dup3): Ditto. + (remove): Ditto. + (getpid): Ditto. + (getppid): Ditto. + (lseek64): Fix strace debugging to correctly use %R. + + * fhandler_termios.cc (fhandler_termios::tcsetpgrp): Avoid sending + signals to other processes if we're debugging since it can cause a + deadlock with the calling debugger. + + * exceptions.cc (_cygtls::call_signal_handler): Add debugging-only + strace output. + 2011-12-16 Corinna Vinschen * dcrt0.cc (child_info_fork::alloc_stack): Correctly check if the diff --git a/winsup/cygwin/cygheap.h b/winsup/cygwin/cygheap.h index 204ebe2f8..5d7b93dcc 100644 --- a/winsup/cygwin/cygheap.h +++ b/winsup/cygwin/cygheap.h @@ -328,10 +328,7 @@ class cygheap_fdmanip if (locked) cygheap->fdtab.unlock (); } - void release () - { - cygheap->fdtab.release (fd); - } + virtual void release () { cygheap->fdtab.release (fd); } operator int &() {return fd;} operator fhandler_base* &() {return cygheap->fdtab[fd];} operator fhandler_socket* () const {return reinterpret_cast (cygheap->fdtab[fd]);} @@ -368,12 +365,18 @@ class cygheap_fdnew : public cygheap_fdmanip locked = false; } } + ~cygheap_fdnew () + { + if (cygheap->fdtab[fd]) + cygheap->fdtab[fd]->refcnt (1); + } void operator = (fhandler_base *fh) {cygheap->fdtab[fd] = fh;} }; class cygheap_fdget : public cygheap_fdmanip { - public: + fhandler_base *fh; +public: cygheap_fdget (int fd, bool lockit = false, bool do_set_errno = true) { if (lockit) @@ -382,6 +385,8 @@ class cygheap_fdget : public cygheap_fdmanip { this->fd = fd; locked = lockit; + fh = cygheap->fdtab[fd]; + fh->refcnt (1); } else { @@ -391,8 +396,27 @@ class cygheap_fdget : public cygheap_fdmanip if (lockit) cygheap->fdtab.unlock (); locked = false; + fh = NULL; } } + ~cygheap_fdget () + { + if (!fh) + /* nothing to do */; + else if (fh->refcnt (-1) > 0) + debug_only_printf ("fh %p, %s, refcnt %ld", fh, fh->get_name (), fh->refcnt ()); + else + { + debug_only_printf ("deleting fh %p, %s, refcnt %ld", fh, fh->get_name (), fh->refcnt ()); + delete fh; + } + } + void release () + { + fh = cygheap->fdtab[fd]; + if (cygheap->fdtab.release (fd)) + fh = NULL; + } }; class cygheap_fdenum : public cygheap_fdmanip diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc index 4da3a908b..c8619ca84 100644 --- a/winsup/cygwin/dtable.cc +++ b/winsup/cygwin/dtable.cc @@ -241,16 +241,26 @@ dtable::find_unused_handle (int start) return -1; } -void +bool dtable::release (int fd) { - if (!not_open (fd)) + bool deleted; + if (not_open (fd)) + deleted = false; + else { if (fds[fd]->need_fixup_before ()) dec_need_fixup_before (); - delete fds[fd]; + if (fds[fd]->refcnt (-1) > 0) + deleted = false; + else + { + deleted = true; + delete fds[fd]; + } fds[fd] = NULL; } + return deleted; } extern "C" int @@ -261,6 +271,7 @@ cygwin_attach_handle_to_fd (char *name, int fd, HANDLE handle, mode_t bin, fd = cygheap->fdtab.find_unused_handle (); fhandler_base *fh = build_fh_name (name); cygheap->fdtab[fd] = fh; + cygheap->fdtab[fd]->refcnt (1); fh->init (handle, myaccess, bin ?: fh->pc_binmode ()); return fd; } @@ -395,6 +406,7 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle) fh->open_setup (openflags); fh->usecount = 0; cygheap->fdtab[fd] = fh; + cygheap->fdtab[fd]->refcnt (1); set_std_handle (fd); paranoid_printf ("fd %d, handle %p", fd, handle); } diff --git a/winsup/cygwin/dtable.h b/winsup/cygwin/dtable.h index 1c487d131..cab9ac577 100644 --- a/winsup/cygwin/dtable.h +++ b/winsup/cygwin/dtable.h @@ -63,7 +63,7 @@ public: } int find_unused_handle (int start); int find_unused_handle () { return find_unused_handle (first_fd_for_open);} - void release (int fd); + bool release (int fd) __attribute__ ((regparm (2))); void init_std_file_from_handle (int fd, HANDLE handle); int dup3 (int oldfd, int newfd, int flags); void fixup_after_exec (); diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc index 9250b61f2..3fcd91c5e 100644 --- a/winsup/cygwin/exceptions.cc +++ b/winsup/cygwin/exceptions.cc @@ -1344,6 +1344,7 @@ _cygtls::call_signal_handler () if (!sig) break; + debug_only_printf ("dealing with signal %d", sig); this_sa_flags = sa_flags; int thissig = sig; void (*thisfunc) (int) = func; diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc index 5fd416ce2..04499e6e7 100644 --- a/winsup/cygwin/fhandler.cc +++ b/winsup/cygwin/fhandler.cc @@ -1148,11 +1148,10 @@ fhandler_base::close () res = 0; else { - paranoid_printf ("CloseHandle (%d <%s>) failed", get_handle (), - get_name ()); - + paranoid_printf ("CloseHandle failed, %E"); __seterrno (); } + isclosed (true); return res; } @@ -1485,6 +1484,7 @@ fhandler_base::fhandler_base () : access (0), io_handle (NULL), ino (0), + _refcnt (0), openflags (0), rabuf (NULL), ralen (0), @@ -1495,6 +1495,7 @@ fhandler_base::fhandler_base () : archetype (NULL), usecount (0) { + isclosed (false); } /* Normal I/O destructor */ @@ -1928,8 +1929,8 @@ fhandler_base_overlapped::wait_overlapped (bool inres, bool writing, DWORD *byte DWORD wfres = cygwait (get_overlapped ()->hEvent); HANDLE h = writing ? get_output_handle () : get_handle (); BOOL wores; - if (wfres == WAIT_OBJECT_0 + 1 && !get_overlapped ()) - wores = 0; + if (isclosed ()) + wores = 0; /* closed in another thread or via signal handler */ else { /* Cancelling here to prevent races. It's possible that the I/O has diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h index b1324c563..0a73af82f 100644 --- a/winsup/cygwin/fhandler.h +++ b/winsup/cygwin/fhandler.h @@ -142,12 +142,13 @@ class fhandler_base read or write access */ unsigned close_on_exec : 1; /* close-on-exec */ unsigned need_fork_fixup : 1; /* Set if need to fixup after fork. */ + unsigned isclosed : 1; /* Set when fhandler is closed. */ public: status_flags () : rbinary (0), rbinset (0), wbinary (0), wbinset (0), nohandle (0), did_lseek (0), query_open (no_query), close_on_exec (0), - need_fork_fixup (0) + need_fork_fixup (0), isclosed (0) {} } status, open_status; @@ -158,6 +159,7 @@ class fhandler_base HANDLE io_handle; __ino64_t ino; /* file ID or hashed filename, depends on FS. */ + long _refcnt; protected: /* File open flags from open () and fcntl () calls */ @@ -176,6 +178,7 @@ class fhandler_base HANDLE read_state; public: + long refcnt(long i = 0) {return _refcnt += i;} class fhandler_base *archetype; int usecount; @@ -239,6 +242,7 @@ class fhandler_base IMPLEMENT_STATUS_FLAG (query_state, query_open) IMPLEMENT_STATUS_FLAG (bool, close_on_exec) IMPLEMENT_STATUS_FLAG (bool, need_fork_fixup) + IMPLEMENT_STATUS_FLAG (bool, isclosed) int get_default_fmode (int flags); diff --git a/winsup/cygwin/fhandler_termios.cc b/winsup/cygwin/fhandler_termios.cc index 7dc5bf58c..fca180d99 100644 --- a/winsup/cygwin/fhandler_termios.cc +++ b/winsup/cygwin/fhandler_termios.cc @@ -78,7 +78,7 @@ fhandler_termios::tcsetpgrp (const pid_t pgid) { case bg_ok: tc ()->setpgid (pgid); - if (tc ()->is_console) + if (tc ()->is_console && (strace.active () || !being_debugged ())) tc ()->kill_pgrp (__SIGSETPGRP); res = 0; break; diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc index 6ac4b1c97..f77c451bf 100644 --- a/winsup/cygwin/syscalls.cc +++ b/winsup/cygwin/syscalls.cc @@ -120,51 +120,51 @@ close_all_files (bool norelease) extern "C" int dup (int fd) { - return cygheap->fdtab.dup3 (fd, cygheap_fdnew (), 0); + int res = cygheap->fdtab.dup3 (fd, cygheap_fdnew (), 0); + syscall_printf ("%R = dup(%d)", res, fd); + return res; } extern "C" int dup2 (int oldfd, int newfd) { + int res; if (newfd >= OPEN_MAX_MAX) { - syscall_printf ("-1 = dup2 (%d, %d) (%d too large)", oldfd, newfd, newfd); set_errno (EBADF); - return -1; + res = -1; } - if (newfd == oldfd) + else if (newfd == oldfd) { cygheap_fdget cfd (oldfd); - if (cfd < 0) - { - syscall_printf ("%R = dup2(%d,%d) (oldfd not open)", -1, oldfd, newfd); - return -1; - } - syscall_printf ("%R = dup2(%d, %d) (newfd==oldfd)", oldfd, oldfd, newfd); - return oldfd; + res = (cfd >= 0) ? oldfd : -1; } - return cygheap->fdtab.dup3 (oldfd, newfd, 0); + else + res = cygheap->fdtab.dup3 (oldfd, newfd, 0); + + syscall_printf ("%R = dup2(%d, %d)", res, oldfd, newfd); + return res; } extern "C" int dup3 (int oldfd, int newfd, int flags) { + int res; if (newfd >= OPEN_MAX_MAX) { - syscall_printf ("-1 = dup3 (%d, %d, %p) (%d too large)", - oldfd, newfd, flags, newfd); set_errno (EBADF); - return -1; + res = -1; } - if (newfd == oldfd) + else if (newfd == oldfd) { cygheap_fdget cfd (oldfd, false, false); set_errno (cfd < 0 ? EBADF : EINVAL); - syscall_printf ("-1 = dup3 (%d, %d, %p) (newfd==oldfd)", - oldfd, newfd, flags); - return -1; + res = -1; } - return cygheap->fdtab.dup3 (oldfd, newfd, flags); + else + res = cygheap->fdtab.dup3 (oldfd, newfd, flags); + syscall_printf ("%R = dup2(%d, %d, %p)", res, oldfd, newfd, flags); + return res; } static inline void @@ -965,12 +965,15 @@ remove (const char *ourname) return -1; } - return win32_name.isdir () ? rmdir (ourname) : unlink (ourname); + int res = win32_name.isdir () ? rmdir (ourname) : unlink (ourname); + syscall_printf ("%R = remove(%s)", res, ourname); + return res; } extern "C" pid_t getpid () { + syscall_printf ("%d = getpid()", myself->pid); return myself->pid; } @@ -984,6 +987,7 @@ _getpid_r (struct _reent *) extern "C" pid_t getppid () { + syscall_printf ("%d = getppid()", myself->ppid); return myself->ppid; } @@ -1041,6 +1045,7 @@ getsid (pid_t pid) res = -1; } } + syscall_printf ("%R = getsid(%d)", pid); return res; } @@ -1066,7 +1071,7 @@ read (int fd, void *ptr, size_t len) } /* Could block, so let user know we at least got here. */ - syscall_printf ("read (%d, %p, %d) %sblocking", + syscall_printf ("read(%d, %p, %d) %sblocking", fd, ptr, len, cfd->is_nonblocking () ? "non" : ""); cfd->read (ptr, res = len); @@ -1108,7 +1113,7 @@ readv (int fd, const struct iovec *const iov, const int iovcnt) } /* Could block, so let user know we at least got here. */ - syscall_printf ("readv (%d, %p, %d) %sblocking", + syscall_printf ("readv(%d, %p, %d) %sblocking", fd, iov, iovcnt, cfd->is_nonblocking () ? "non" : ""); res = cfd->readv (iov, iovcnt, tot); @@ -1158,9 +1163,9 @@ write (int fd, const void *ptr, size_t len) /* Could block, so let user know we at least got here. */ if (fd == 1 || fd == 2) - paranoid_printf ("write (%d, %p, %d)", fd, ptr, len); + paranoid_printf ("write(%d, %p, %d)", fd, ptr, len); else - syscall_printf ("write (%d, %p, %d)", fd, ptr, len); + syscall_printf ("write(%d, %p, %d)", fd, ptr, len); res = cfd->write (ptr, len); @@ -1206,9 +1211,9 @@ writev (const int fd, const struct iovec *const iov, const int iovcnt) /* Could block, so let user know we at least got here. */ if (fd == 1 || fd == 2) - paranoid_printf ("writev (%d, %p, %d)", fd, iov, iovcnt); + paranoid_printf ("writev(%d, %p, %d)", fd, iov, iovcnt); else - syscall_printf ("writev (%d, %p, %d)", fd, iov, iovcnt); + syscall_printf ("writev(%d, %p, %d)", fd, iov, iovcnt); res = cfd->writev (iov, iovcnt, tot); @@ -1248,7 +1253,7 @@ open (const char *unix_path, int flags, ...) va_list ap; mode_t mode = 0; - syscall_printf ("open (%s, %p)", unix_path, flags); + syscall_printf ("open(%s, %p)", unix_path, flags); pthread_testcancel (); myfault efault; if (efault.faulted (EFAULT)) @@ -1341,7 +1346,7 @@ lseek64 (int fd, _off64_t pos, int dir) else res = -1; } - syscall_printf ("%D = lseek (%d, %D, %d)", res, fd, pos, dir); + syscall_printf ("%R = lseek(%d, %D, %d)", res, fd, pos, dir); return res; } @@ -1361,7 +1366,7 @@ close (int fd) { int res; - syscall_printf ("close (%d)", fd); + syscall_printf ("close(%d)", fd); pthread_testcancel ();