RE: [boost] Re: [shared_ptr] Interlocked* - possible bug?
John Torjo <john.lists@torjo.com> wrote:
Ben Hutchings wrote: <snip>
expired() may read the count as still being 1 and so return false, but I believe the wnd_shared_ptr constructor will catch the fact that the pointer really has expired. Unfortunately I can't yet see the code to confirm this!
By looking at the code, I would think not. But I may be wrong. Anyway, I've posted the code in my other post.
Thanks for that. Supposing that expired() wrongly returns false because use_count is really 0 but it gets an old non-zero value. Then lock() attempts to construct a shared_ptr: calls shared_ptr<T>::shared_ptr<T>(weak_ptr<T> const &) calls detail::shared_count::shared_count(weak_count const &) calls detail::sp_counted_base::add_ref_lock() calls detail::atomic_conditional_increment( long volatile &) either reads the count as 0 or calls _InterlockedCompareExchange fails and returns 0 then loops around and reads the count as 0 returns 0 throws bad_weak_ptr catches bad_weak_ptr returns 0 OK?
Ben Hutchings wrote:
John Torjo <john.lists@torjo.com> wrote:
Ben Hutchings wrote: <snip>
expired() may read the count as still being 1 and so return false, but I believe the wnd_shared_ptr constructor will catch the fact that the pointer really has expired. Unfortunately I can't yet see the code to confirm this!
By looking at the code, I would think not. But I may be wrong. Anyway, I've posted the code in my other post.
Thanks for that. Supposing that expired() wrongly returns false because use_count is really 0 but it gets an old non-zero value. Then lock() attempts to construct a shared_ptr: [...]
It is also worth noting that no matter how you fiddle with the implementation of atomic_read() that is indirectly invoked by expired(), it would still be possible for the use_count to drop to zero _after_ expired() has returned.
Ben Hutchings wrote:
John Torjo <john.lists@torjo.com> wrote:
Ben Hutchings wrote:
<snip>
expired() may read the count as still being 1 and so return false, but I believe the wnd_shared_ptr constructor will catch the fact that the pointer really has expired. Unfortunately I can't yet see the code to confirm this!
By looking at the code, I would think not. But I may be wrong. Anyway, I've posted the code in my other post.
Thanks for that. Supposing that expired() wrongly returns false because use_count is really 0 but it gets an old non-zero value. Then lock() attempts to construct a shared_ptr:
calls shared_ptr<T>::shared_ptr<T>(weak_ptr<T> const &) calls detail::shared_count::shared_count(weak_count const &) calls detail::sp_counted_base::add_ref_lock() calls detail::atomic_conditional_increment( long volatile &) either reads the count as 0 or calls _InterlockedCompareExchange fails and returns 0 then loops around and reads the count as 0 returns 0 throws bad_weak_ptr catches bad_weak_ptr returns 0
Yes indeed. It was my mistake. I failed to see the flow when shared_ptr gets constructed from weak_ptr. So, I was wrong from the beginning. Sorry. But now, by looking again at atomic_conditional_increment. I'm not a threading expert, but it seems pretty costly as it is implemented now. Could it not go multiple times (n>=2) thorughout the for? Current implementation: inline long atomic_conditional_increment(long volatile & value) { for(;;) { long tmp = value; if(tmp == 0) return 0; if(InterlockedCompareExchange(&value, tmp + 1, tmp) == tmp) return tmp + 1; } } Couldn't it become: inline long atomic_conditional_increment(long volatile & value) { long tmp = value; if(tmp == 0) return 0; long new_val = InterlockedCompareExchange(&value, tmp + 1, tmp); if(new_val == tmp) return tmp + 1; else return InterlockedIncrement(&value); } Best, John -- John Torjo -- john@torjo.com Contributing editor, C/C++ Users Journal -- "Win32 GUI Generics" -- generics & GUI do mix, after all -- http://www.torjo.com/win32gui/ -- v1.4 - save_dlg - true binding of your data to UI controls! + easily add validation rules (win32gui/examples/smart_dlg)
participants (3)
-
Ben Hutchings -
John Torjo -
Peter Dimov