This is the mail archive of the
newlib@sourceware.org
mailing list for the newlib project.
Re: [RFA] More aggressive use of atexit locking
- From: Dave Korn <dave dot korn dot cygwin at googlemail dot com>
- To: newlib at sourceware dot org
- Date: Tue, 06 Apr 2010 20:53:39 +0100
- Subject: Re: [RFA] More aggressive use of atexit locking
- References: <20100202021054.GA1117@ednor.casa.cgf.cx>
On 02/02/2010 02:10, Christopher Faylor wrote:
> It seems like we should be locking both the feeder and the emptier for
> atexit operations shouldn't we? Otherwise one can be deleting things
> while another is adding things. This is especially important for the
> __cxa_* functions since the emptier can be called at any time, not just
> on process exit.
Heh, there's always a catch. Apparently it's legitimate for an atexit
callback function to register further atexit functions! At the moment,
though, the initial atexit callback function is called with the atexit lock
held, so when it calls atexit, that attempts to take the lock again, resulting
in deadlock.
The existing code in __call_exitprocs seems to be prepared for the
possibility of the atexit function list getting adjusted beneath its feet as a
result of actions taken by the function it's calling, so that should be OK. I
considered unlocking the lock around the function callback, but that seemed a
bit risky, so this patch changes the lock to a recursive one, allowing the
nested acquire to succeed instead of deadlocking.
Tested on i686-pc-cygwin, by running the g++ testsuite; the current state of
CVS HEAD trips this bug in two tests, leading to these failures:
> WARNING: program timed out.
> FAIL: g++.old-deja/g++.other/init18.C execution test
> WARNING: program timed out.
> FAIL: g++.old-deja/g++.other/init19.C execution test
With this patch, those two tests pass. Ok?
newlib/ChangeLog
* libc/stdlib/__atexit.c (__atexit_lock): Initialise as recursive
rather than non-recursive lock type.
(__register_exitproc): Use recursive locking APIs on it.
* libc/stdlib/__call_atexit.c (__call_exitprocs): Likewise.
cheers,
DaveK
? newlib/libc/libc.info
? newlib/libm/libm.info
Index: newlib/libc/stdlib/__atexit.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdlib/__atexit.c,v
retrieving revision 1.6
diff -p -u -r1.6 __atexit.c
--- newlib/libc/stdlib/__atexit.c 4 Feb 2010 17:57:30 -0000 1.6
+++ newlib/libc/stdlib/__atexit.c 5 Apr 2010 16:45:08 -0000
@@ -10,7 +10,7 @@
/* Make this a weak reference to avoid pulling in malloc. */
void * malloc(size_t) _ATTRIBUTE((__weak__));
-__LOCK_INIT(, __atexit_lock);
+__LOCK_INIT_RECURSIVE(, __atexit_lock);
/*
* Register a function to be performed at exit or on shared library unload.
@@ -28,7 +28,7 @@ _DEFUN (__register_exitproc,
register struct _atexit *p;
#ifndef __SINGLE_THREAD__
- __lock_acquire(__atexit_lock);
+ __lock_acquire_recursive(__atexit_lock);
#endif
p = _GLOBAL_REENT->_atexit;
@@ -48,7 +48,7 @@ _DEFUN (__register_exitproc,
if (p == NULL)
{
#ifndef __SINGLE_THREAD__
- __lock_release(__atexit_lock);
+ __lock_release_recursive(__atexit_lock);
#endif
return -1;
}
@@ -93,7 +93,7 @@ _DEFUN (__register_exitproc,
}
p->_fns[p->_ind++] = fn;
#ifndef __SINGLE_THREAD__
- __lock_release(__atexit_lock);
+ __lock_release_recursive(__atexit_lock);
#endif
return 0;
}
Index: newlib/libc/stdlib/__call_atexit.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdlib/__call_atexit.c,v
retrieving revision 1.8
diff -p -u -r1.8 __call_atexit.c
--- newlib/libc/stdlib/__call_atexit.c 24 Feb 2010 19:58:17 -0000 1.8
+++ newlib/libc/stdlib/__call_atexit.c 5 Apr 2010 16:45:08 -0000
@@ -33,7 +33,7 @@ _DEFUN (__call_exitprocs, (code, d),
#ifndef __SINGLE_THREAD__
- __lock_acquire(__atexit_lock);
+ __lock_acquire_recursive(__atexit_lock);
#endif
restart:
@@ -115,7 +115,7 @@ _DEFUN (__call_exitprocs, (code, d),
#endif
}
#ifndef __SINGLE_THREAD__
- __lock_release(__atexit_lock);
+ __lock_release_recursive(__atexit_lock);
#endif
}