GNOME Bugzilla – Bug 764935
3.0: Basic thread safety / simplified tracking
Last modified: 2018-07-10 22:47:51 UTC
Created attachment 325774 [details] [review] 0001-Add-test_signal_shared_by_threads.patch It might be wise for the libsigc++ classes to offer at least a basic level of thread safety, much as std::shared_ptr<> does. This could appropriately lock the private parts of the classes that the application cannot lock itself, as suggested here: https://herbsutter.com/2014/01/06/gotw-95-thread-safety-and-synchronization/ For instance, this patch adds a test that does signal connection in multiple threads, though it doesn't actually emit the signal in separate threads. This is meant to exercise at least the signal_base::ref_count_, though that's just the start of this. Using clang++'s -fsanitize=thread option, we get this: ================== WARNING: ThreadSanitizer: data race (pid=5824) Write of size 8 at 0x7d080000efd0 by thread T2: #0 std::__cxx11::_List_base<sigc::slot_base, std::allocator<sigc::slot_base> >::_M_inc_size(unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/stl_list.h:363 (lt-test_signal_shared_by_threads+0x0000004ba44e) #1 std::_List_iterator<sigc::slot_base> std::__cxx11::list<sigc::slot_base, std::allocator<sigc::slot_base> >::emplace<sigc::slot_base>(std::_List_const_iterator<sigc::slot_base>, sigc::slot_base&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/list.tcc:92 (lt-test_signal_shared_by_threads+0x0000004ba202) #2 std::__cxx11::list<sigc::slot_base, std::allocator<sigc::slot_base> >::insert(std::_List_const_iterator<sigc::slot_base>, sigc::slot_base&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/stl_list.h:1177 (lt-test_signal_shared_by_threads+0x0000004ba076) #3 sigc::internal::signal_impl::insert(std::_List_iterator<sigc::slot_base>, sigc::slot_base&&) /home/murrayc/checkout/gnome/libsigcplusplus-master/sigc++/signal_base.cc:154 (libsigc-3.0.so.0+0x0000000081a1) #4 operator() /home/murrayc/checkout/gnome/libsigcplusplus-master/tests/test_signal_shared_by_threads.cc:41 (lt-test_signal_shared_by_threads+0x0000004b6400) #5 void std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>::_M_invoke<>(std::_Index_tuple<>) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:1530 (lt-test_signal_shared_by_threads+0x0000004b6358) #6 std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>::operator()() /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:1520 (lt-test_signal_shared_by_threads+0x0000004b6308) #7 std::__future_base::_Task_setter<std::unique_ptr<std::__future_base::_Result<void>, std::__future_base::_Result_base::_Deleter>, std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>::operator()() const /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/future:1342 (lt-test_signal_shared_by_threads+0x0000004b616b) #8 std::_Function_handler<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> (), std::__future_base::_Task_setter<std::unique_ptr<std::__future_base::_Result<void>, std::__future_base::_Result_base::_Deleter>, std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void> >::_M_invoke(std::_Any_data const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:1856 (lt-test_signal_shared_by_threads+0x0000004b5f3f) #9 std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>::operator()() const /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:2271 (lt-test_signal_shared_by_threads+0x0000004bf63b) #10 std::__future_base::_State_baseV2::_M_do_set(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/future:527 (lt-test_signal_shared_by_threads+0x0000004be4cc) #11 void std::_Mem_fn_base<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), true>::operator()<std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*, void>(std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*&&, bool*&&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:600 (lt-test_signal_shared_by_threads+0x0000004bf1ed) #12 void std::_Bind_simple<std::_Mem_fn<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*)> (std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*)>::_M_invoke<0ul, 1ul, 2ul>(std::_Index_tuple<0ul, 1ul, 2ul>) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:1530 (lt-test_signal_shared_by_threads+0x0000004bf06a) #13 std::_Bind_simple<std::_Mem_fn<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*)> (std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*)>::operator()() /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:1520 (lt-test_signal_shared_by_threads+0x0000004befb8) #14 void std::__once_call_impl<std::_Bind_simple<std::_Mem_fn<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*)> (std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*)> >() /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/mutex:697 (lt-test_signal_shared_by_threads+0x0000004be7fc) #15 pthread_once <null> (lt-test_signal_shared_by_threads+0x00000042a3bf) #16 __gthread_once(int*, void (*)()) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/x86_64-linux-gnu/c++/5.2.1/bits/gthr-default.h:699 (lt-test_signal_shared_by_threads+0x0000004b5db6) #17 void std::call_once<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*>(std::once_flag&, void (std::__future_base::_State_baseV2::*&&)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*&&, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*&&, bool*&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/mutex:729 (lt-test_signal_shared_by_threads+0x0000004be45a) #18 std::__future_base::_State_baseV2::_M_set_result(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>, bool) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/future:386 (lt-test_signal_shared_by_threads+0x0000004bddf5) #19 operator() /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/future:1658 (lt-test_signal_shared_by_threads+0x0000004b5ab3) #20 void std::_Bind_simple<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>::_Async_state_impl(std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>&&)::{lambda()#1} ()>::_M_invoke<>(std::_Index_tuple<>) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:1530 (lt-test_signal_shared_by_threads+0x0000004b59c8) #21 std::_Bind_simple<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>::_Async_state_impl(std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>&&)::{lambda()#1} ()>::operator()() /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:1520 (lt-test_signal_shared_by_threads+0x0000004b5978) #22 std::thread::_Impl<std::_Bind_simple<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>::_Async_state_impl(std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>&&)::{lambda()#1} ()> >::_M_run() /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/thread:115 (lt-test_signal_shared_by_threads+0x0000004b578c) #23 std::this_thread::__sleep_for(std::chrono::duration<long, std::ratio<1l, 1l> >, std::chrono::duration<long, std::ratio<1l, 1000000000l> >) <null> (libstdc++.so.6+0x0000000b902f) Previous write of size 8 at 0x7d080000efd0 by thread T1: #0 std::__cxx11::_List_base<sigc::slot_base, std::allocator<sigc::slot_base> >::_M_inc_size(unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/stl_list.h:363 (lt-test_signal_shared_by_threads+0x0000004ba44e) #1 std::_List_iterator<sigc::slot_base> std::__cxx11::list<sigc::slot_base, std::allocator<sigc::slot_base> >::emplace<sigc::slot_base>(std::_List_const_iterator<sigc::slot_base>, sigc::slot_base&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/list.tcc:92 (lt-test_signal_shared_by_threads+0x0000004ba202) #2 std::__cxx11::list<sigc::slot_base, std::allocator<sigc::slot_base> >::insert(std::_List_const_iterator<sigc::slot_base>, sigc::slot_base&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/stl_list.h:1177 (lt-test_signal_shared_by_threads+0x0000004ba076) #3 sigc::internal::signal_impl::insert(std::_List_iterator<sigc::slot_base>, sigc::slot_base&&) /home/murrayc/checkout/gnome/libsigcplusplus-master/sigc++/signal_base.cc:154 (libsigc-3.0.so.0+0x0000000081a1) #4 operator() /home/murrayc/checkout/gnome/libsigcplusplus-master/tests/test_signal_shared_by_threads.cc:41 (lt-test_signal_shared_by_threads+0x0000004b6400) #5 void std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>::_M_invoke<>(std::_Index_tuple<>) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:1530 (lt-test_signal_shared_by_threads+0x0000004b6358) #6 std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>::operator()() /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:1520 (lt-test_signal_shared_by_threads+0x0000004b6308) #7 std::__future_base::_Task_setter<std::unique_ptr<std::__future_base::_Result<void>, std::__future_base::_Result_base::_Deleter>, std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>::operator()() const /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/future:1342 (lt-test_signal_shared_by_threads+0x0000004b616b) #8 std::_Function_handler<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> (), std::__future_base::_Task_setter<std::unique_ptr<std::__future_base::_Result<void>, std::__future_base::_Result_base::_Deleter>, std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void> >::_M_invoke(std::_Any_data const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:1856 (lt-test_signal_shared_by_threads+0x0000004b5f3f) #9 std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>::operator()() const /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:2271 (lt-test_signal_shared_by_threads+0x0000004bf63b) #10 std::__future_base::_State_baseV2::_M_do_set(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/future:527 (lt-test_signal_shared_by_threads+0x0000004be4cc) #11 void std::_Mem_fn_base<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), true>::operator()<std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*, void>(std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*&&, bool*&&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:600 (lt-test_signal_shared_by_threads+0x0000004bf1ed) #12 void std::_Bind_simple<std::_Mem_fn<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*)> (std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*)>::_M_invoke<0ul, 1ul, 2ul>(std::_Index_tuple<0ul, 1ul, 2ul>) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:1530 (lt-test_signal_shared_by_threads+0x0000004bf06a) #13 std::_Bind_simple<std::_Mem_fn<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*)> (std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*)>::operator()() /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:1520 (lt-test_signal_shared_by_threads+0x0000004befb8) #14 void std::__once_call_impl<std::_Bind_simple<std::_Mem_fn<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*)> (std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*)> >() /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/mutex:697 (lt-test_signal_shared_by_threads+0x0000004be7fc) #15 pthread_once <null> (lt-test_signal_shared_by_threads+0x00000042a3bf) #16 __gthread_once(int*, void (*)()) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/x86_64-linux-gnu/c++/5.2.1/bits/gthr-default.h:699 (lt-test_signal_shared_by_threads+0x0000004b5db6) #17 void std::call_once<void (std::__future_base::_State_baseV2::*)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*>(std::once_flag&, void (std::__future_base::_State_baseV2::*&&)(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*), std::__future_base::_State_baseV2*&&, std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*&&, bool*&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/mutex:729 (lt-test_signal_shared_by_threads+0x0000004be45a) #18 std::__future_base::_State_baseV2::_M_set_result(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>, bool) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/future:386 (lt-test_signal_shared_by_threads+0x0000004bddf5) #19 operator() /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/future:1658 (lt-test_signal_shared_by_threads+0x0000004b5ab3) #20 void std::_Bind_simple<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>::_Async_state_impl(std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>&&)::{lambda()#1} ()>::_M_invoke<>(std::_Index_tuple<>) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:1530 (lt-test_signal_shared_by_threads+0x0000004b59c8) #21 std::_Bind_simple<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>::_Async_state_impl(std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>&&)::{lambda()#1} ()>::operator()() /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/functional:1520 (lt-test_signal_shared_by_threads+0x0000004b5978) #22 std::thread::_Impl<std::_Bind_simple<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>::_Async_state_impl(std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>&&)::{lambda()#1} ()> >::_M_run() /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/thread:115 (lt-test_signal_shared_by_threads+0x0000004b578c) #23 std::this_thread::__sleep_for(std::chrono::duration<long, std::ratio<1l, 1l> >, std::chrono::duration<long, std::ratio<1l, 1000000000l> >) <null> (libstdc++.so.6+0x0000000b902f) Location is heap block of size 32 at 0x7d080000efc0 allocated by main thread: #0 operator new(unsigned long) <null> (lt-test_signal_shared_by_threads+0x0000004afee3) #1 sigc::signal_base::impl() const /home/murrayc/checkout/gnome/libsigcplusplus-master/sigc++/signal_base.cc:329 (libsigc-3.0.so.0+0x00000000856a) #2 signal /home/murrayc/checkout/gnome/libsigcplusplus-master/tests/../sigc++/signal.h:1070 (lt-test_signal_shared_by_threads+0x0000004b8d7b) #3 test_connect_many_signals_in_many_threads() /home/murrayc/checkout/gnome/libsigcplusplus-master/tests/test_signal_shared_by_threads.cc:40 (lt-test_signal_shared_by_threads+0x0000004b2483) #4 main /home/murrayc/checkout/gnome/libsigcplusplus-master/tests/test_signal_shared_by_threads.cc:72 (lt-test_signal_shared_by_threads+0x0000004b29a4) Thread T2 (tid=5840, running) created by main thread at: #0 pthread_create <null> (lt-test_signal_shared_by_threads+0x00000042783b) #1 std::thread::_M_start_thread(std::shared_ptr<std::thread::_Impl_base>, void (*)()) <null> (libstdc++.so.6+0x0000000b9172) #2 _Async_state_impl /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/future:1655 (lt-test_signal_shared_by_threads+0x0000004b3ec3) #3 void __gnu_cxx::new_allocator<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void> >::construct<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>, std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()> >(std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>*, std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/ext/new_allocator.h:120 (lt-test_signal_shared_by_threads+0x0000004b3db7) #4 _ZNSt16allocator_traitsISaINSt13__future_base17_Async_state_implISt12_Bind_simpleIFZ41test_connect_many_signals_in_many_threadsvE3$_0vEEvEEEE12_S_constructIS6_JS5_EEENSt9enable_ifIXsr6__and_INS8_18__construct_helperIT_JDpT0_EE4typeEEE5valueEvE4typeERS7_PSC_DpOSD_ /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/alloc_traits.h:256 (lt-test_signal_shared_by_threads+0x0000004b3d53) #5 _ZNSt16allocator_traitsISaINSt13__future_base17_Async_state_implISt12_Bind_simpleIFZ41test_connect_many_signals_in_many_threadsvE3$_0vEEvEEEE9constructIS6_JS5_EEEDTcl12_S_constructfp_fp0_spclsr3stdE7forwardIT0_Efp1_EEERS7_PT_DpOSA_ /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/alloc_traits.h:402 (lt-test_signal_shared_by_threads+0x0000004b39f3) #6 _Sp_counted_ptr_inplace<std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()> > /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/shared_ptr_base.h:522 (lt-test_signal_shared_by_threads+0x0000004b3542) #7 __shared_count<std::__future_base::_Async_state_impl<std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()>, void>, std::allocator<std::__future_base::_Async_state_impl<std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()>, void> >, std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()> > /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/shared_ptr_base.h:617 (lt-test_signal_shared_by_threads+0x0000004b322b) #8 __shared_ptr<std::allocator<std::__future_base::_Async_state_impl<std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()>, void> >, std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()> > /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/shared_ptr_base.h:1096 (lt-test_signal_shared_by_threads+0x0000004b30a7) #9 shared_ptr<std::allocator<std::__future_base::_Async_state_impl<std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()>, void> >, std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()> > /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/shared_ptr.h:319 (lt-test_signal_shared_by_threads+0x0000004b3013) #10 std::shared_ptr<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void> > std::allocate_shared<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>, std::allocator<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void> >, std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()> >(std::allocator<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void> > const&, std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/shared_ptr.h:613 (lt-test_signal_shared_by_threads+0x0000004b2ed7) #11 std::shared_ptr<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void> > std::make_shared<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>, std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()> >(std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/shared_ptr.h:629 (lt-test_signal_shared_by_threads+0x0000004b2d63) #12 std::shared_ptr<std::__future_base::_State_baseV2> std::__future_base::_S_make_async_state<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()> >(std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/future:1696 (lt-test_signal_shared_by_threads+0x0000004b2b8f) #13 std::future<std::result_of<test_connect_many_signals_in_many_threads()::$_0& ()>::type> std::async<test_connect_many_signals_in_many_threads()::$_0&>(std::launch, test_connect_many_signals_in_many_threads()::$_0&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/future:1709 (lt-test_signal_shared_by_threads+0x0000004b27a6) #14 test_connect_many_signals_in_many_threads() /home/murrayc/checkout/gnome/libsigcplusplus-master/tests/test_signal_shared_by_threads.cc:50 (lt-test_signal_shared_by_threads+0x0000004b24d6) #15 main /home/murrayc/checkout/gnome/libsigcplusplus-master/tests/test_signal_shared_by_threads.cc:72 (lt-test_signal_shared_by_threads+0x0000004b29a4) Thread T1 (tid=5839, finished) created by main thread at: #0 pthread_create <null> (lt-test_signal_shared_by_threads+0x00000042783b) #1 std::thread::_M_start_thread(std::shared_ptr<std::thread::_Impl_base>, void (*)()) <null> (libstdc++.so.6+0x0000000b9172) #2 _Async_state_impl /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/future:1655 (lt-test_signal_shared_by_threads+0x0000004b3ec3) #3 void __gnu_cxx::new_allocator<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void> >::construct<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>, std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()> >(std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>*, std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/ext/new_allocator.h:120 (lt-test_signal_shared_by_threads+0x0000004b3db7) #4 _ZNSt16allocator_traitsISaINSt13__future_base17_Async_state_implISt12_Bind_simpleIFZ41test_connect_many_signals_in_many_threadsvE3$_0vEEvEEEE12_S_constructIS6_JS5_EEENSt9enable_ifIXsr6__and_INS8_18__construct_helperIT_JDpT0_EE4typeEEE5valueEvE4typeERS7_PSC_DpOSD_ /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/alloc_traits.h:256 (lt-test_signal_shared_by_threads+0x0000004b3d53) #5 _ZNSt16allocator_traitsISaINSt13__future_base17_Async_state_implISt12_Bind_simpleIFZ41test_connect_many_signals_in_many_threadsvE3$_0vEEvEEEE9constructIS6_JS5_EEEDTcl12_S_constructfp_fp0_spclsr3stdE7forwardIT0_Efp1_EEERS7_PT_DpOSA_ /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/alloc_traits.h:402 (lt-test_signal_shared_by_threads+0x0000004b39f3) #6 _Sp_counted_ptr_inplace<std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()> > /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/shared_ptr_base.h:522 (lt-test_signal_shared_by_threads+0x0000004b3542) #7 __shared_count<std::__future_base::_Async_state_impl<std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()>, void>, std::allocator<std::__future_base::_Async_state_impl<std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()>, void> >, std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()> > /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/shared_ptr_base.h:617 (lt-test_signal_shared_by_threads+0x0000004b322b) #8 __shared_ptr<std::allocator<std::__future_base::_Async_state_impl<std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()>, void> >, std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()> > /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/shared_ptr_base.h:1096 (lt-test_signal_shared_by_threads+0x0000004b30a7) #9 shared_ptr<std::allocator<std::__future_base::_Async_state_impl<std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()>, void> >, std::_Bind_simple<(lambda at test_signal_shared_by_threads.cc:40:5) ()> > /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/shared_ptr.h:319 (lt-test_signal_shared_by_threads+0x0000004b3013) #10 std::shared_ptr<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void> > std::allocate_shared<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>, std::allocator<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void> >, std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()> >(std::allocator<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void> > const&, std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/shared_ptr.h:613 (lt-test_signal_shared_by_threads+0x0000004b2ed7) #11 std::shared_ptr<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void> > std::make_shared<std::__future_base::_Async_state_impl<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>, void>, std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()> >(std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/shared_ptr.h:629 (lt-test_signal_shared_by_threads+0x0000004b2d63) #12 std::shared_ptr<std::__future_base::_State_baseV2> std::__future_base::_S_make_async_state<std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()> >(std::_Bind_simple<test_connect_many_signals_in_many_threads()::$_0 ()>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/future:1696 (lt-test_signal_shared_by_threads+0x0000004b2b8f) #13 std::future<std::result_of<test_connect_many_signals_in_many_threads()::$_0& ()>::type> std::async<test_connect_many_signals_in_many_threads()::$_0&>(std::launch, test_connect_many_signals_in_many_threads()::$_0&) /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/future:1709 (lt-test_signal_shared_by_threads+0x0000004b27a6) #14 test_connect_many_signals_in_many_threads() /home/murrayc/checkout/gnome/libsigcplusplus-master/tests/test_signal_shared_by_threads.cc:50 (lt-test_signal_shared_by_threads+0x0000004b24d6) #15 main /home/murrayc/checkout/gnome/libsigcplusplus-master/tests/test_signal_shared_by_threads.cc:72 (lt-test_signal_shared_by_threads+0x0000004b29a4) SUMMARY: ThreadSanitizer: data race /usr/bin/../lib/gcc/x86_64-linux-gnu/5.2.1/../../../../include/c++/5.2.1/bits/stl_list.h:363 in std::__cxx11::_List_base<sigc::slot_base, std::allocator<sigc::slot_base> >::_M_inc_size(unsigned long) ================== ThreadSanitizer: reported 1 warnings
Created attachment 325776 [details] [review] 0001-signal_impl-Lock-slots_-with-a-mutex.patch This is a presumably-naive attempt to fix the problem with multi-threaded access to the signal_impl::slots_ std::list. I hope we can make this cleaner and simpler.
I'm also wondering if we can replace the reference-counting and tracking by some appropriate uses of std::shared_ptr<> and std::weak_ptr<>.
Created attachment 326110 [details] [review] 0001-signal_impl-Trying-to-do-the-ref-counting-with-std-s.patch Here is a simple attempt to replace the signal_impl::ref_count and manual reference/unreference with just using signal_impl via std::shared_ptr<>. This means having to do std::shared_ptr<signal_impl>(this), with std::enable_shared_from_this<signal_impl>, but that should theoretically be OK. This currently causes an infinite loop in the signal_impl destructor when it calls clear(). It looks like the destructor (of the same instance) is being repeatedly re-called. For instance, when running tests/test_signal:
+ Trace 236181
I can dig in this topic next week, but for now just quickly reviewed your patch. signal_exec exec(std::shared_ptr<signal_impl>(this)); It's not a way of creating shared_ptr of *this* pointer. You should use shared_from_this() instead (http://en.cppreference.com/w/cpp/memory/enable_shared_from_this/shared_from_this). If you change a way of creating shared_ptr, it'll probably solve problem from comment 3
Created attachment 326137 [details] [review] 0001-signal_impl-Trying-to-do-the-ref-counting-with-std-s2.patch Thanks. Yes, that seems to improve things. Many tests still fail, however. For instance, with test_signal: Program received signal SIGABRT, Aborted. 0x00007ffff7290267 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:55 55 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. (gdb) bt
+ Trace 236182
That is indeed triggered by the attempt to get a shared_ptr to this object during its own destruction. If we instead take a copy of the slots_ list (of slot_base) and iterate over it instead of the original, that seems to fix it. But I'm not sure yet whether slot_base shares anything between copies. So maybe disconnecting the copies does nothing useful. However, the tests also all pass when signal_impl::clear() is empty, in either git master or the old libsigc++-2.0 version, so maybe we need some test to check that it really does whatever it should do.
Created attachment 326162 [details] [review] 0001-signal_exec-Rename-to-signal_exec_holder.patch
Created attachment 326163 [details] [review] 0002-signal_impl_holder-Split-into-this-and-signal_exec_h.patch This changes clear() to just increment/decrement the exec_count, instead of trying to get a shared_ptr to this during destruction. That might be the original intention.
Created attachment 326394 [details] [review] 0001-signal_impl-Trying-to-do-the-ref-counting-with-std-s.patch Updated for git master.
Created attachment 326395 [details] [review] 0002-signal_exec-Rename-to-signal_exec_holder.patch Updated for git master.
Created attachment 326396 [details] [review] 0003-signal_impl_holder-Split-into-this-and-signal_exec_h.patch Updated for git master.
By the way, git master (and the libsigc++-2.10 branch) now have an --enable-benchmark configure option so we can do some comparisons. It shows no noticeably significant performance difference with this use of std::shared_ptr<>, using CXXFLAGS="-O2". [murrayc@murrayc-ThinkPad-X220 libsigcplusplus-master (master)]$ ./tests/benchmark elapsed time for calling a slot 10000000 times: 0.036584s wall, 0.040000s user + 0.000000s system = 0.040000s CPU (109.3%) elapsed time for 10000000 emissions (0 slots): 0.018969s wall, 0.010000s user + 0.000000s system = 0.010000s CPU (52.7%) elapsed time for 10000000 emissions (1 slot): 0.459183s wall, 0.460000s user + 0.000000s system = 0.460000s CPU (100.2%) elapsed time for 10000000 emissions (5 slots): 0.570006s wall, 0.570000s user + 0.000000s system = 0.570000s CPU (100.0%) elapsed time for 10000000 connections/disconnections: 1.664515s wall, 1.670000s user + 0.000000s system = 1.670000s CPU (100.3%) [murrayc@murrayc-ThinkPad-X220 libsigcplusplus-master-sharedptr (master)]$ ./tests/benchmark elapsed time for calling a slot 10000000 times: 0.036507s wall, 0.040000s user + 0.000000s system = 0.040000s CPU (109.6%) elapsed time for 10000000 emissions (0 slots): 0.018628s wall, 0.010000s user + 0.000000s system = 0.010000s CPU (53.7%) elapsed time for 10000000 emissions (1 slot): 0.548716s wall, 0.550000s user + 0.000000s system = 0.550000s CPU (100.2%) elapsed time for 10000000 emissions (5 slots): 0.651529s wall, 0.660000s user + 0.000000s system = 0.660000s CPU (101.3%) elapsed time for 10000000 connections/disconnections: 1.912694s wall, 1.910000s user + 0.000000s system = 1.910000s CPU (99.9%)
I am troubled by this direct call of a destructor in typed_slot_rep::destroy(). Can anyone figure out why we are doing that? https://git.gnome.org/browse/libsigcplusplus/tree/sigc++/functors/slot.h#n92 static void destroy(notifiable* data) { auto self_ = static_cast<self*>(data); self_->call_ = nullptr; self_->destroy_ = nullptr; sigc::visit_each_trackable(slot_do_unbind(self_), self_->functor_); self_->functor_.~adaptor_type(); /* don't call disconnect() here: destroy() is either called * a) from the parent itself (in which case disconnect() leads to a segfault) or * b) from a parentless slot (in which case disconnect() does nothing) */ }
I encountered that while exploring the idea of replacing the destroy_ vfunc with a regular virtual destructor: https://git.gnome.org/browse/libsigcplusplus/tree/sigc++/functors/slot_base.h#n151 though I haven't had much success with that yet.
I think calling destructor on member objects is the only(or at least, the easiest) way to destroy, but not delete the object.
Yes, but that seems very strange to me. I don't like leaving an object in this strange destroyed but not deleted state. I have tried to change that functor_ to a pointer (or unique_ptr), using new to initialize it, so we can just delete it and set it to null, but haven't managed that yet. Maybe you'd like to try.
Created attachment 326564 [details] [review] 0001-slot-use-pointer-for-functor_-member.patch Please review. If looks ok, I'll push it.
Excellent. Yes, please push that. When I tried it I was missing the * in std::make_unique<adaptor_type>(*cl.functor_)) because I didn't see that it was actually a copy constructor. I think I will make source that they all use src. I'm thinking of pushing the shared_ptr<> patches. It only partly improves the thread safety, but it seems good to replace our custom reference-counting. Does anyone have an opinion about that?
(In reply to Murray Cumming from comment #18) > I'm thinking of pushing the shared_ptr<> patches. It only partly improves > the thread safety, but it seems good to replace our custom > reference-counting. Does anyone have an opinion about that? Which patches do you mean? All attached to this bug? I could review them before.
I mean: 0001-signal_impl-Trying-to-do-the-ref-counting-with-std-s.patch 0002-signal_exec-Rename-to-signal_exec_holder.patch 0003-signal_impl_holder-Split-into-this-and-signal_exec_h.patch
I'm a bit concerned about this inheriting from enable_shared_from_this. From documentation [1]: "Note that prior to calling shared_from_this on an object t, there must be a std::shared_ptr that owns t." and I can't find a place in the code where the shared_ptr<signal_impl> is created. Did I miss something? Anyway, good practice is to provide method public: static shared_ptr<> create(); to a enable_shared_from_this-based class, and make all constructors private/protected, so you're 100% sure that shared_ptr object exists. [1] http://en.cppreference.com/w/cpp/memory/enable_shared_from_this
(In reply to Marcin Kolny (IRC: loganek) from comment #21) > I'm a bit concerned about this inheriting from enable_shared_from_this. From > documentation [1]: > "Note that prior to calling shared_from_this on an object t, there must be a > std::shared_ptr that owns t." > and I can't find a place in the code where the shared_ptr<signal_impl> is > created. Did I miss something? There's a call to std::make_shared<>() in the first patch, resulting in this code: std::shared_ptr<internal::signal_impl> signal_base::impl() const { if (!impl_) { impl_ = std::make_shared<internal::signal_impl>(); } return impl_; } > Anyway, good practice is to provide method > > public: static shared_ptr<> create(); > > to a enable_shared_from_this-based class, and make all constructors > private/protected, so you're 100% sure that shared_ptr object exists. Doesn't that stop us from using std::make_shared(), because it cannot use the private constructor?
(In reply to Murray Cumming from comment #22) > There's a call to std::make_shared<>() in the first patch, resulting in this > code: Ok, I missed it. > Doesn't that stop us from using std::make_shared(), because it cannot use > the private constructor? Yes. But you don't need std::make_shared anymore, if you already have create() method.
However, it is generally best to use std::make_shared() for efficiency (one allocation instead of two, to include the control block) and safety (no leaks when there are exceptions), so I'd like to do that.
I agree, we cannot avoid two allocations. Safety is only a problem, when you create shared_ptr inline in function's parameter list, and perform also other functions in this parameter list. So template<typename... T> static shared_ptr<ThisClass> create() create(T&&... p) { return shared_ptr<ThisClass>(new ThisClass(std::forward<T>(p)...)); } is safe. Moreover, it's also recommended pattern in Scott Meyers book [1]. From the other hand, it's internal class, so nobody else should use it directly. I think your patches look ok. [1] http://www.amazon.com/Effective-Modern-Specific-Ways-Improve/dp/1491903996
Comment on attachment 326394 [details] [review] 0001-signal_impl-Trying-to-do-the-ref-counting-with-std-s.patch >From 9782c59815f29dd20d19cbaaf61ddd24860d492c Mon Sep 17 00:00:00 2001 >From: Murray Cumming <murrayc@murrayc.com> >Date: Fri, 15 Apr 2016 17:16:15 +0200 >Subject: [PATCH 1/3] signal_impl: Trying to do the ref-counting with > std::shared_ptr. > >Bug #764935 >--- > sigc++/signal.h | 16 ++++++++-------- > sigc++/signal_base.cc | 44 ++++++++++---------------------------------- > sigc++/signal_base.h | 36 ++++++++++-------------------------- > 3 files changed, 28 insertions(+), 68 deletions(-) > >diff --git a/sigc++/signal.h b/sigc++/signal.h >index 6208a70..3318151 100644 >--- a/sigc++/signal.h >+++ b/sigc++/signal.h >@@ -176,7 +176,7 @@ struct slot_list > > slot_list() : sig_impl_(nullptr) {} > >- explicit slot_list(internal::signal_impl* sig_impl) : sig_impl_(sig_impl) {} >+ explicit slot_list(const std::shared_ptr<internal::signal_impl>& sig_impl) : sig_impl_(sig_impl) {} > > iterator begin() { return iterator(sig_impl_->slots_.begin()); } > >@@ -238,7 +238,7 @@ struct slot_list > } > > private: >- internal::signal_impl* sig_impl_; >+ std::shared_ptr<internal::signal_impl> sig_impl_; > }; > > namespace internal >@@ -623,7 +623,7 @@ struct signal_emit > * @param a Arguments to be passed on to the slots. > * @return The accumulated return values of the slot invocations as processed by the accumulator. > */ >- static decltype(auto) emit(signal_impl* impl, type_trait_take_t<T_arg>... a) >+ static decltype(auto) emit(const std::shared_ptr<internal::signal_impl>& impl, type_trait_take_t<T_arg>... a) > { > T_accumulator accumulator; > >@@ -643,7 +643,7 @@ struct signal_emit > * @param a Arguments to be passed on to the slots. > * @return The accumulated return values of the slot invocations as processed by the accumulator. > */ >- static decltype(auto) emit_reverse(signal_impl* impl, type_trait_take_t<T_arg>... a) >+ static decltype(auto) emit_reverse(const std::shared_ptr<internal::signal_impl>& impl, type_trait_take_t<T_arg>... a) > { > T_accumulator accumulator; > >@@ -692,7 +692,7 @@ public: > * passed on to the slots. > * @return The return value of the last slot invoked. > */ >- static decltype(auto) emit(signal_impl* impl, type_trait_take_t<T_arg>... a) >+ static decltype(auto) emit(const std::shared_ptr<internal::signal_impl>& impl, type_trait_take_t<T_arg>... a) > { > if (!impl || impl->slots_.empty()) > return T_return(); >@@ -735,7 +735,7 @@ public: > * @param a Arguments to be passed on to the slots. > * @return The return value of the last slot invoked. > */ >- static decltype(auto) emit_reverse(signal_impl* impl, type_trait_take_t<T_arg>... a) >+ static decltype(auto) emit_reverse(const std::shared_ptr<internal::signal_impl>& impl, type_trait_take_t<T_arg>... a) > { > if (!impl || impl->slots_.empty()) > return T_return(); >@@ -794,7 +794,7 @@ public: > * passed directly on to the slots. > * @param a Arguments to be passed on to the slots. > */ >- static decltype(auto) emit(signal_impl* impl, type_trait_take_t<T_arg>... a) >+ static decltype(auto) emit(const std::shared_ptr<internal::signal_impl>& impl, type_trait_take_t<T_arg>... a) > { > if (!impl || impl->slots_.empty()) > return; >@@ -816,7 +816,7 @@ public: > * @param last An iterator pointing to the last slot in the list. * @param a Arguments to be > * passed on to the slots. > */ >- static decltype(auto) emit_reverse(signal_impl* impl, type_trait_take_t<T_arg>... a) >+ static decltype(auto) emit_reverse(const std::shared_ptr<internal::signal_impl>& impl, type_trait_take_t<T_arg>... a) > { > if (!impl || impl->slots_.empty()) > return; >diff --git a/sigc++/signal_base.cc b/sigc++/signal_base.cc >index cde3d99..bb2fc8d 100644 >--- a/sigc++/signal_base.cc >+++ b/sigc++/signal_base.cc >@@ -28,27 +28,20 @@ namespace internal > // when the slot is disconnected. Bug 167714. > struct self_and_iter : public notifiable > { >- signal_impl* self_; >+ const std::shared_ptr<signal_impl> self_; > const signal_impl::iterator_type iter_; > >- self_and_iter(signal_impl* self, const signal_impl::iterator_type& iter) : self_(self), iter_(iter) {} >+ self_and_iter(const std::shared_ptr<signal_impl>& self, const signal_impl::iterator_type& iter) : self_(self), iter_(iter) {} > }; > >-signal_impl::signal_impl() : ref_count_(0), exec_count_(0), deferred_(false) >+signal_impl::signal_impl() : exec_count_(0), deferred_(false) > { > } > > signal_impl::~signal_impl() > { >- // unreference() must not call ~signal_impl() while clear() is executing. >- ++ref_count_; >- > // Disconnect all slots before *this is deleted. > clear(); >- >- // Now ref_count_ can be cleared again (not really necessary), but don't do it >- // with a call to unreference(). That would invoke ~signal_impl() recursively. >- --ref_count_; > } > > // only MSVC needs this to guarantee that all new/delete are executed from the DLL module >@@ -72,7 +65,7 @@ signal_impl::clear() > // Don't let signal_impl::notify() erase the slots. It would invalidate the > // iterator in the following loop. > const bool saved_deferred = deferred_; >- signal_exec exec(this); >+ signal_exec exec(shared_from_this()); > > // Disconnect all connected slots before they are deleted. > // signal_impl::notify() will be called and delete the self_and_iter structs. >@@ -128,7 +121,7 @@ signal_impl::erase(iterator_type i) > // Don't let signal_impl::notify() erase the slot. It would be more > // difficult to get the correct return value from signal_impl::erase(). > const bool saved_deferred = deferred_; >- signal_exec exec(this); >+ signal_exec exec(shared_from_this()); > > // Disconnect the slot before it is deleted. > // signal_impl::notify() will be called and delete the self_and_iter struct. >@@ -143,7 +136,7 @@ signal_impl::iterator_type > signal_impl::insert(signal_impl::iterator_type i, const slot_base& slot_) > { > auto temp = slots_.insert(i, slot_); >- auto si = new self_and_iter(this, temp); >+ auto si = new self_and_iter(shared_from_this(), temp); > temp->set_parent(si, ¬ify); > return temp; > } >@@ -152,7 +145,7 @@ signal_impl::iterator_type > signal_impl::insert(signal_impl::iterator_type i, slot_base&& slot_) > { > auto temp = slots_.insert(i, std::move(slot_)); >- auto si = new self_and_iter(this, temp); >+ auto si = new self_and_iter(shared_from_this(), temp); > temp->set_parent(si, ¬ify); > return temp; > } >@@ -163,7 +156,7 @@ signal_impl::sweep() > // The deletion of a slot may cause the deletion of a signal_base, > // a decrementation of ref_count_, and the deletion of this. > // In that case, the deletion of this is deferred to ~signal_exec(). >- signal_exec exec(this); >+ signal_exec exec(shared_from_this()); > > deferred_ = false; > auto i = slots_.begin(); >@@ -208,7 +201,6 @@ signal_base::signal_base() noexcept : impl_(nullptr) > > signal_base::signal_base(const signal_base& src) noexcept : trackable(), impl_(src.impl()) > { >- impl_->reference(); > } > > signal_base::signal_base(signal_base&& src) : trackable(std::move(src)), impl_(std::move(src.impl_)) >@@ -218,10 +210,6 @@ signal_base::signal_base(signal_base&& src) : trackable(std::move(src)), impl_(s > > signal_base::~signal_base() > { >- if (impl_) >- { >- impl_->unreference(); >- } > } > > void >@@ -293,13 +281,7 @@ signal_base::operator=(const signal_base& src) > if (src.impl_ == impl_) > return *this; > >- if (impl_) >- { >- impl_->unreference(); >- } >- > impl_ = src.impl(); >- impl_->reference(); > return *this; > } > >@@ -309,11 +291,6 @@ signal_base::operator=(signal_base&& src) > if (src.impl_ == impl_) > return *this; > >- if (impl_) >- { >- impl_->unreference(); >- } >- > src.notify_callbacks(); > impl_ = src.impl_; > src.impl_ = nullptr; >@@ -321,13 +298,12 @@ signal_base::operator=(signal_base&& src) > return *this; > } > >-internal::signal_impl* >+std::shared_ptr<internal::signal_impl> > signal_base::impl() const > { > if (!impl_) > { >- impl_ = new internal::signal_impl; >- impl_->reference(); // start with a reference count of 1 >+ impl_ = std::make_shared<internal::signal_impl>(); > } > return impl_; > } >diff --git a/sigc++/signal_base.h b/sigc++/signal_base.h >index 10d3623..0a5f3ab 100644 >--- a/sigc++/signal_base.h >+++ b/sigc++/signal_base.h >@@ -21,6 +21,7 @@ > > #include <cstddef> > #include <list> >+#include <memory> //For std::shared_ptr<> > #include <sigc++config.h> > #include <sigc++/type_traits.h> > #include <sigc++/trackable.h> >@@ -42,7 +43,9 @@ namespace internal > * erase() to sweep() when the signal is being emitted. sweep() removes all > * invalid slots from the list. > */ >-struct SIGC_API signal_impl : public notifiable >+struct SIGC_API signal_impl >+ : public notifiable, >+ public std::enable_shared_from_this<signal_impl> > { > using size_type = std::size_t; > using slot_list = std::list<slot_base>; >@@ -64,34 +67,19 @@ struct SIGC_API signal_impl : public notifiable > void operator delete(void* p); > #endif > >- /// Increments the reference counter. >- inline void reference() noexcept { ++ref_count_; } >- > /// Increments the reference and execution counter. > inline void reference_exec() noexcept > { >- ++ref_count_; > ++exec_count_; > } > >- /** Decrements the reference counter. >- * The object is deleted when the reference counter reaches zero. >- */ >- inline void unreference() >- { >- if (!(--ref_count_)) >- delete this; >- } >- > /** Decrements the reference and execution counter. > * Invokes sweep() if the execution counter reaches zero and the > * removal of one or more slots has been deferred. > */ > inline void unreference_exec() > { >- if (!(--ref_count_)) >- delete this; >- else if (!(--exec_count_) && deferred_) >+ if (!(--exec_count_) && deferred_) > sweep(); > } > >@@ -181,11 +169,6 @@ public: > std::list<slot_base> slots_; > > private: >- /** Reference counter. >- * The object is destroyed when @em ref_count_ reaches zero. >- */ >- short ref_count_; >- > /** Execution counter. > * Indicates whether the signal is being emitted. > */ >@@ -201,7 +184,8 @@ struct SIGC_API signal_exec > /** Increments the reference and execution counter of the parent sigc::signal_impl object. > * @param sig The parent sigc::signal_impl object. > */ >- inline signal_exec(const signal_impl* sig) noexcept : sig_(const_cast<signal_impl*>(sig)) >+ inline signal_exec(const std::shared_ptr<signal_impl>& sig) noexcept >+ : sig_(sig) > { > sig_->reference_exec(); > } >@@ -217,7 +201,7 @@ struct SIGC_API signal_exec > > protected: > /// The parent sigc::signal_impl object. >- signal_impl* sig_; >+ const std::shared_ptr<signal_impl> sig_; > }; > > } /* namespace internal */ >@@ -385,10 +369,10 @@ protected: > /** Returns the signal_impl object encapsulating the list of slots. > * @return The signal_impl object encapsulating the list of slots. > */ >- internal::signal_impl* impl() const; >+ std::shared_ptr<internal::signal_impl> impl() const; > > /// The signal_impl object encapsulating the slot list. >- mutable internal::signal_impl* impl_; >+ mutable std::shared_ptr<internal::signal_impl> impl_; > }; > > } // namespace sigc >-- >2.5.0 >
Was there anything for me in that last comment, or did something just go wrong with the review system?
No, my comment was unintended, probably I clicked something wrong. All your patches look ok for me.
Now I'm wondering if trackable's notification (of trackable's invalidation) callback can be replaced with weakptr, of course without requiring any trackable (such as slot) to be used via shared_ptr/weak_ptr.
About make_shared - maybe this would be of help? http://stackoverflow.com/questions/8147027/how-do-i-call-stdmake-shared-on-a-class-with-only-protected-or-private-const
I don't see anything particularly suitable there, but did you mean a particular technique? For this case, it's not so important, but it will become an issue when we one day use std::shared_ptr<> instead of Glib::RefPtr in glibmm: https://bugzilla.gnome.org/show_bug.cgi?id=755037
Created attachment 326891 [details] [review] 0001-Experiment-Use-weak_ptr-instead-of-add_destroy_notif.patch I haven't found a good way to replace our notifiable/trackable system with std::weak_ptr<> or std::shared_ptr<>'s Deleter. This hacky little patch shows one idea, which doesn't really seem suitable. It changes Connection to keep a std::weak_ptr<> of the slot_base's child slot_rep, just so it can have a weak_ptr<> to something that will become invalid when the slot_base is deleted. We can't have a weak_ptr<> to the slot_base itself because we don't want to force slots to be used only via shared_ptr. It then checks that weak_ptr before every use of the slot_base, so we can invalidate the slot_base pointer too. That's obviously not as good as invalidating the slot_base pointer as soon as the weak_ptr first becomes invalid (expired), but the weak_ptr doesn't give us any way to react immediately. Amazingly, all the tests seem to pass with this. If we instead used the shared_pr<>'s Deleter, that Deleter would then call some callbacks, just like we do now, with no obvious improvements.
Created attachment 326926 [details] [review] 0001-Added-sigc-internal-weak_raw_ptr.patch
Created attachment 326927 [details] [review] 0002-connection-Use-weak_raw_ptr-for-slot_base.patch This seems like a big improvement on my hacky use of std::weak_ptr<>, though it requires our own custom std::internal::weak_raw_ptr<T>, where T must be a sigc::trackable. I think it nicely simplifies the connection code for nulling the slot_base*. Does anyone see a problem with this?
Created attachment 326929 [details] [review] 0003-slot_base-Use-weak_raw_ptr-instead-of-destroy_notify.patch
I have pushed that weak_raw_ptr<> and the 2 uses of it, though I'd still welcome any review. Unfortunately, the other uses of trackable::add_destroy_notify_callback() cannot be wrapped so easily because the notify callback handlers do more than just null a pointer. For instance: https://git.gnome.org/browse/libsigcplusplus/tree/sigc++/functors/slot_base.cc#n61 https://git.gnome.org/browse/libsigcplusplus/tree/sigc++/signal_base.cc#n177
According to https://libsigcplusplus.github.io/libsigcplusplus/devel.html the issue tracker of libsigc++ is located at https://github.com/libsigcplusplus/libsigcplusplus/issues nowadays. If the problem in this bug report still exists in a recent version of libsigc++ please file this bug report in GitHub. Closing this report as WONTFIX as part of Bugzilla Housekeeping, as we plan to shut down GNOME Bugzilla in favor of GNOME Gitlab.