Created attachment 22383 [details] test case hello, i have just hit a bug in the implementation of shared_ptr in GCC 4.5 on Windows. the bug wasn't present in GCC 4.4. the bug is also specific to libstdc++: when using boost or MSVC, there is no problem. basically, any creation of a shared_ptr increases the amount of semaphores used by the process. this can be viewed by running the Task Manager and looking into the "Handles" column. the semaphores are never released and the count therefore keeps increasing without rest. the following simple prog shows it: int main() { int n=0; while (++n < 100) { //shared_ptr<string> x = make_shared<string>("test"); shared_ptr<string> x(new string("test")); Sleep(100); } return 0; } (using either make_shared or the constructor doesn't make a difference.) when running it, you will notice a steady increase of the amount of handles, reaching more than 100 handles after 10 seconds. compile the same prog with MSVC or GCC 4.4 and notice that the amount of handles doesn't grow. of course, for this simple example, it is inoffensive, but programs that make a more important use of shared_ptr can quickly run out of available handles and crash in unexpected ways. for reference, the whole discussion is on <https://bugs.launchpad.net/dcplusplus/+bug/654040>.
I don't think anything in shared_ptr changed between 4.4 and 4.5, though I don't know what thread layer is used by MinGW. I'll look into it
What's the value of __gnu_cxx::__default_lock_policy on Mingw? Is it the same for GCC 4.4 and 4.5? Between 4.4 and 4.5 there are some changes to the ghtr-win32.h file which provides the gthreads abstraction layer, but there are no significant changes to shared_ptr. So for the moment I think it's either a change in the atomic operations supported by mingw or a change in the threading layer, not shared_ptr
(In reply to comment #2) > What's the value of __gnu_cxx::__default_lock_policy on Mingw? > Is it the same for GCC 4.4 and 4.5? it is 1 on both. (i got it using std::cout << __gnu_cxx::__default_lock_policy; hope that's correct.)
yes, that's fine, thanks
(In reply to comment #2) > What's the value of __gnu_cxx::__default_lock_policy on Mingw? > Is it the same for GCC 4.4 and 4.5? > > Between 4.4 and 4.5 there are some changes to the ghtr-win32.h file which > provides the gthreads abstraction layer, but there are no significant changes > to shared_ptr. So for the moment I think it's either a change in the atomic > operations supported by mingw or a change in the threading layer, not > shared_ptr Well, I've tested it with mingw-w64 toolchains (64-bit and 32-bit) and I couldn't reproduce this issue. Nevertheless it could be related here to some modifications done by mingw.org on their toolchain. So threading model is here something to look for. Kai
if mingw.org modify GCC then this bug should be reported there, as I don't see anything wrong in the libstdc++ code
Could you try this, which is a simplified version of the shared_ptr refcounting code and should have the same behaviour: #include <ext/atomicity.h> #include <ext/concurrence.h> #include <assert.h> static int count0 = 0; struct X { X() : count1(1), count2(1) { ++count0; } ~X() { assert( count1==0 ); assert( count2==0 ); --count0; } void release() { if (__gnu_cxx::__exchange_and_add_dispatch(&count1, -1) == 1) { if (__gnu_cxx::__exchange_and_add_dispatch(&count2, -1) == 1) delete this; } } __gnu_cxx::__mutex m; _Atomic_word count1; _Atomic_word count2; }; int main() { for (int i=0; i<100; ++i) { X* x = new X; x->release(); } assert( count0 == 0 ); }
(In reply to comment #5) > (In reply to comment #2) > > What's the value of __gnu_cxx::__default_lock_policy on Mingw? > > Is it the same for GCC 4.4 and 4.5? > > > > Between 4.4 and 4.5 there are some changes to the ghtr-win32.h file which > > provides the gthreads abstraction layer, but there are no significant changes > > to shared_ptr. So for the moment I think it's either a change in the atomic > > operations supported by mingw or a change in the threading layer, not > > shared_ptr > Well, I've tested it with mingw-w64 toolchains (64-bit and 32-bit) and I > couldn't reproduce this issue. Nevertheless it could be related here to some > modifications done by mingw.org on their toolchain. So threading model is here > something to look for. > Kai i have just tried with the mingw64 currently in Cygwin (GCC 4.5.1) and it indeed doesn't exhibit this problem. i then tried to get the value of __gnu_cxx::__default_lock_policy on mingw64 and there, it is 2 (whereas it is 1 on vanilla mingw).
(In reply to comment #7) > Could you try this, which is a simplified version of the shared_ptr refcounting > code and should have the same behaviour: > #include <ext/atomicity.h> > #include <ext/concurrence.h> > #include <assert.h> > static int count0 = 0; > struct X > { > X() : count1(1), count2(1) { ++count0; } > ~X() { assert( count1==0 ); assert( count2==0 ); --count0; } > void release() > { > if (__gnu_cxx::__exchange_and_add_dispatch(&count1, -1) == 1) > { > if (__gnu_cxx::__exchange_and_add_dispatch(&count2, -1) == 1) > delete this; > } > } > __gnu_cxx::__mutex m; > _Atomic_word count1; > _Atomic_word count2; > }; > int main() > { > for (int i=0; i<100; ++i) > { > X* x = new X; > x->release(); > } > assert( count0 == 0 ); > } [i added a Sleep(100) call after x->release(); for easier diagnostics.] this code produces the same behavior as my test case (increasing number of semaphores, over 100 after 10 seconds) on both vanilla MinGW and mingw64. to summarize: my test case (standard shared_ptr creation) worked fine on mingw64 (which has a __gnu_cxx::__default_lock_policy = 2) but bugged on vanilla MinGW (which has a __gnu_cxx::__default_lock_policy = 1); but this one produces the bug in both cases.
(In reply to comment #9) > > this code produces the same behavior as my test case (increasing number of > semaphores, over 100 after 10 seconds) on both vanilla MinGW and mingw64. > > to summarize: my test case (standard shared_ptr creation) worked fine on > mingw64 (which has a __gnu_cxx::__default_lock_policy = 2) but bugged on > vanilla MinGW (which has a __gnu_cxx::__default_lock_policy = 1); but this one > produces the bug in both cases. That's because my simpler test ignores the default_lock_policy, it always uses a mutex, which is what shared_ptr does when __default_lock_policy=1 I've just looked at __gnu_cxx::__mutex and it doesn't have a destructor, so the problem is probably just that we leak the mutex. This should show the same bug: #include <ext/concurrence.h> int main() { for (int i=0; i<100; ++i) __gnu_cxx::__mutex m; } Could you test that? We should add a destructor to __mutex, which calls __gthread_mutex_destroy. N.B. this could cause a problem on FreeBSD http://www.freebsd.org/cgi/query-pr.cgi?pr=150889 so we might want to only call __gthread_mutex_destroy when we have used __GTHREAD_MUTEX_INIT_FUNCTION
(In reply to comment #10) > That's because my simpler test ignores the default_lock_policy, it always uses > a mutex, which is what shared_ptr does when __default_lock_policy=1 > I've just looked at __gnu_cxx::__mutex and it doesn't have a destructor, so the > problem is probably just that we leak the mutex. This should show the same bug: > #include <ext/concurrence.h> > int main() > { > for (int i=0; i<100; ++i) > __gnu_cxx::__mutex m; > } > Could you test that? > We should add a destructor to __mutex, which calls __gthread_mutex_destroy. > N.B. this could cause a problem on FreeBSD > http://www.freebsd.org/cgi/query-pr.cgi?pr=150889 > so we might want to only call __gthread_mutex_destroy when we have used > __GTHREAD_MUTEX_INIT_FUNCTION this does reproduce the issue on both vanilla MinGW and MinGW64.
Missing destructors: <ext/concurrence.h> __gnu_cxx::__mutex __gnu_cxx::__recursive_mutex __gnu_cxx::__cond <mutex> std::mutex std::recursive_mutex std::timed_mutex std::recursive_timed_mutex I'm not sure why these didn't cause a problem previously, the destructors aren't there in 4.4 either
Thanks a lot Jon for tracking this down! Do you think we can fix it in time for 4.6.0?
yes, it shouldn't be hard to fix
Butenhof's book says you don't need to destroy a mutex/condvar that was statically initialized, so given the FreeBSD bug I will only define the destructor when the __GTHREAD_XXX_INIT macro is not available
Created attachment 22413 [details] add destructors in <ext/concurrence.h> could you try applying this patch to ext/concurrence.h and let me know if it works on Windows? I'm testing on Linux, FreeBSD and OpenBSD but they all use the pthreads model and I don't have a Windows system to test on.
(In reply to comment #16) > Created attachment 22413 [details] > add destructors in <ext/concurrence.h> > could you try applying this patch to ext/concurrence.h and let me know if it > works on Windows? I'm testing on Linux, FreeBSD and OpenBSD but they all use > the pthreads model and I don't have a Windows system to test on. hi, the patch first produced a trivial compiler error: In file included from c:\mingw\bin\../lib/gcc/mingw32/4.5.0/include/c++/memory:7 8:0, from test.cpp:15: c:\mingw\bin\../lib/gcc/mingw32/4.5.0/include/c++/ext/concurrence.h: In destruct or '__gnu_cxx::__recursive_mutex::~__recursive_mutex()': c:\mingw\bin\../lib/gcc/mingw32/4.5.0/include/c++/ext/concurrence.h:224:35: erro r: cannot convert '__gthread_recursive_mutex_t*' to '__gthread_mutex_t*' for arg ument '1' to 'void __gthread_mutex_destroy(__gthread_mutex_t*)' i worked around it by making the pointer cast explicit in the __recursive_mutex destructor: __gthread_mutex_destroy(reinterpret_cast<__gthread_mutex_t*>(&_M_mutex)); it then compiles fine and correctly fixes the issue at hand.
Thanks for testing it. The cast makes me uncomfortable so I'll look into adding a __gthread_recursive_mutex_destroy function...
Jon, sometimes finding a reviewer for those gthr changes takes a bit of time... and we are already in Stage 3... Thus, I would recommend doing our best to figure out first a decent library-only fix.
OK, that will be ugly though. The cast from __gthread_recursive_mutex_t* to __gthread_mutex_t* is not correct, because the "sema" member (the actual Win32 handle) is at a different offset in the two mutex types. We want something like: static inline int __recursive_mutex_destroy(__gthread_recursive_mutex_t* __rmutex) { #ifdef _WIN32 __gthread_mutex_t __tmp = { }; __tmp.counter = __rmutex->counter; __tmp.sema = __rmutex->sema; __ghtread_mutex_t* __mutex = &__tmp; #else __ghtread_mutex_t* __mutex = __rmutex; #endif return ___gthread_mutex_destroy(__mutex); }
Argh, I see. I think we should keep the option open, anyway, together with a huge FIXME in the code, of course. I also think we should try to explain the problem to the people actually maintaining the gthr stuff: if we can convince those people that the change is localized and at the library-level we can only do very ugly things, maybe we can win...
... and when using gthr-mipssde.h / gthr-posix95.h / gthr-solaris.h: static inline int __recursive_mutex_destroy(__gthread_recursive_mutex_t* __rmutex) { return ___gthread_mutex_destroy(__mutex->actual); } Adding the right preprocessor checks to the library will be messy, because configure might have made gthr-default.h a symlink to one of the headers above and we don't have a macro to test. The right place for the destroy function is in the gthr headers where __gthread_recursive_mutex_t is defined. Hmm, I wonder if we could use sfinae, there are three cases: - recursive mutex is the same type as the non-recursive mutex, or - it has an "actual" member, or - it has a "sema" member. I'll try that this evening.
Created attachment 22424 [details] add destructors in <ext/concurrence.h> here's another patch, this one uses SFINAE to select an appropriate overload based on the type of __gthread_recursive_mutex_t and if necessary extracts a __gthread_mutex_t from it to pass to __gthread_mutex_destroy this will be wrong if a recursive mutex type needs more cleanup than just destroying its non-recursive mutex, though I don't think that's a problem for any current systems supported by gthreads. could be improved upon by disabling the third _S_destroy overload when recursive_mutex_type is not the same as mutex_type, but it's ugly enough already. Zouzou, could you test this on mingw? thanks
(In reply to comment #23) > Created attachment 22424 [details] > add destructors in <ext/concurrence.h> > here's another patch, this one uses SFINAE to select an appropriate overload > based on the type of __gthread_recursive_mutex_t and if necessary extracts a > __gthread_mutex_t from it to pass to __gthread_mutex_destroy > this will be wrong if a recursive mutex type needs more cleanup than just > destroying its non-recursive mutex, though I don't think that's a problem for > any current systems supported by gthreads. > could be improved upon by disabling the third _S_destroy overload when > recursive_mutex_type is not the same as mutex_type, but it's ugly enough > already. > Zouzou, could you test this on mingw? thanks same compiler error as for the previous patch: In file included from c:\mingw\bin\../lib/gcc/mingw32/4.5.0/include/c++/memory:78:0, from test.cpp:15: c:\mingw\bin\../lib/gcc/mingw32/4.5.0/include/c++/ext/concurrence.h: In static member function 'static int __gnu_cxx::__recursive_mutex::_S_destroy(_Rm*, ...) [with _Rm = __gthread_recursive_mutex_t]': c:\mingw\bin\../lib/gcc/mingw32/4.5.0/include/c++/ext/concurrence.h:224:22: instantiated from here c:\mingw\bin\../lib/gcc/mingw32/4.5.0/include/c++/ext/concurrence.h:284:44: error: cannot convert '__gthread_recursive_mutex_t*' to '__gthread_mutex_t*' for argument '1' to 'void __gthread_mutex_destroy(__gthread_mutex_t*)' (my line numbers are off by 2 compared to your patches.) apparently the 1st overload doesn't match so it falls back to the 3rd one.
(In reply to comment #24) > apparently the 1st overload doesn't match so it falls back to the 3rd one. bah! ok, thanks, back to the drawing board...
Oops, that patch was broken - could you change the recursive mutex destructor so it calls _S_destroy(&_M_mutex, 0) instead of _S_destroy(&_M_mutex) As I can only test on POSIX systems I didn't notice I'd copied the code wrong from my prototype version which I used to simulate the Win32 case.
I don't think that's right either ... I need to find a better way to test this....
Created attachment 22427 [details] add destructors in <ext/concurrence.h> almost the same as the last patch but the destructor calls: _S_destroy<__gthread_recursive_mutex_t>(&_M_mutex, 0);
(In reply to comment #28) > Created attachment 22427 [details] > add destructors in <ext/concurrence.h> > almost the same as the last patch but the destructor calls: > _S_destroy<__gthread_recursive_mutex_t>(&_M_mutex, 0); different error this time: In file included from c:\mingw\bin\../lib/gcc/mingw32/4.5.0/include/c++/memory:78:0, from test.cpp:15: c:\mingw\bin\../lib/gcc/mingw32/4.5.0/include/c++/ext/concurrence.h: In static member function 'static int __gnu_cxx::__recursive_mutex::_S_destroy(_Rm*, typename __gnu_cxx::__recursive_mutex::_Detect_win32<_Rm, (& _Rm:: sema)>::type*) [with _Rm = __gthread_recursive_mutex_t, typename __gnu_cxx::__recursive_mutex::_Detect_win32<_Rm, (& _Rm:: sema)>::type = __gthread_mutex_t]': c:\mingw\bin\../lib/gcc/mingw32/4.5.0/include/c++/ext/concurrence.h:224:54: instantiated from here c:\mingw\bin\../lib/gcc/mingw32/4.5.0/include/c++/ext/concurrence.h:267:46: error: void value not ignored as it ought to be because __gthread_mutex_destroy returns void. after changing the return types of the _S_destroy functions to void and removing the "return" specifiers, the test case compiles and runs fine without any leak.
(In reply to comment #29) > because __gthread_mutex_destroy returns void. Argh, why is gthr-win32.h different here?! Nevermind ... > after changing the return types of the _S_destroy functions to void and > removing the "return" specifiers, the test case compiles and runs fine without > any leak. Great, thanks very much for testing. I'll get similar changes done to <mutex> and get this checked in.
Created attachment 22436 [details] add destructors in <ext/concurrence.h> a simpler version - could you test this on mingw? thanks
(In reply to comment #31) > Created attachment 22436 [details] > add destructors in <ext/concurrence.h> > a simpler version - could you test this on mingw? thanks great, compiles and fixes the resource leaks.
If we need __is_same / __are_same, maybe we can just include bits/cpp_type_traits.h: isn't that big (and probably something else includes it anyway in most of the cases)
(In reply to comment #33) > If we need __is_same / __are_same, maybe we can just include > bits/cpp_type_traits.h aha, thanks for the pointer to are_same, I was going to move __gnu_debug::__is_same into ext/type_traits.h and use that, I didn't know about are_same. complete patch coming soon ...
patch posted to http://gcc.gnu.org/ml/gcc-patches/2010-11/msg01875.html
Author: redi Date: Thu Nov 18 18:56:29 2010 New Revision: 166917 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=166917 Log: 2010-11-18 Jonathan Wakely <jwakely.gcc@gmail.com> PR libstdc++/46455 * include/std/mutex: Define destructors for mutex types which use an init function. * include/ext/concurrence.h: Likewise. Modified: trunk/libstdc++-v3/ChangeLog trunk/libstdc++-v3/include/ext/concurrence.h trunk/libstdc++-v3/include/std/mutex
fixed for 4.6.0 I think this is a bit risky for the 4.5 branch, and isn't a regression as we've never cleaned up those mutexes (the change in shared_ptr behaviour may be a regression, but seems to be caused by a change in MinGW's port - maybe they can apply the fix to their release if necessary)
*** Bug 48391 has been marked as a duplicate of this bug. ***