From 201471612c5b1bc9634393d5f2fcc7db2b6f9193 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Tue, 15 Jul 2014 23:39:20 -0700 Subject: [PATCH 01/12] Simultaneous acquisition of locks avoids possible deadlock. --- autowiring/atomic_object.h | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/autowiring/atomic_object.h b/autowiring/atomic_object.h index ffdd2034a..eeea71c4a 100644 --- a/autowiring/atomic_object.h +++ b/autowiring/atomic_object.h @@ -42,7 +42,7 @@ class atomic_object { /// atomic_object target(*unlock_object(source)); /// atomic_object(const atomic_object& source) { - std::lock_guard lock_this(source.m_lock); + std::lock_guard lock_source(source.m_lock); m_object = source.m_object; m_initialized = source.m_initialized; } @@ -64,10 +64,15 @@ class atomic_object { atomic_object& operator = (const atomic_object& source) { if (this == &source) return *this; - std::lock_guard lock_this(m_lock); - std::lock_guard locksource(source.m_lock); + //IMPORTANT: Aquisition of both locks must be atomic. + //The following code: + // m_initialized = source.initialized(m_object); + //could deadlock with its counterpart in source. + std::lock(m_lock, source.m_lock); m_object = source.m_object; m_initialized = source.m_initialized; + m_lock.unlock(); + source.m_lock.unlock(); return *this; } @@ -96,7 +101,7 @@ class atomic_object { } /// - ///Atomic copy of this location to argument location, only if this has location. + ///Atomic copy of this object to target, only if initialized() == true. /// ///True if the object was not assigned default values bool initialized(object& target) { From 737bab3b9237ee0b3f3c9fa253c5138559460598 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Wed, 16 Jul 2014 00:18:02 -0700 Subject: [PATCH 02/12] Clarifying shared_object comments. --- autowiring/shared_object.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/autowiring/shared_object.h b/autowiring/shared_object.h index 3a73944c5..490e73b40 100644 --- a/autowiring/shared_object.h +++ b/autowiring/shared_object.h @@ -48,7 +48,8 @@ class shared_object { /// ///To copy the atomic_object referenced by source use: /// shared_object target(*unlock_object(source)); - /// + ///This method does NOT copy the atomic_object referenced by source. + /// shared_object(const shared_object& source): m_share(source.m_share) {} @@ -65,6 +66,7 @@ class shared_object { /// ///This method is equivalent to: /// target = *unlock_object(source); + ///This method does NOT redirect the atomic_object reference to the reference of source. /// shared_object& operator = (const shared_object& source) { *m_share = *source.m_share; @@ -92,7 +94,7 @@ class shared_object { } /// - ///Atomic copy of this location to argument location, only if this has location. + ///Atomic copy of target to this object, only if initialized() == false. /// ///True if the object was not assigned default values bool initialized(object& target) { From 44f9d19f43cca62a47193862681c34ab8bf96397 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Wed, 16 Jul 2014 00:19:40 -0700 Subject: [PATCH 03/12] Renaming unlock_object methods for clarity. --- autowiring/unlock_object.h | 13 +++++++------ src/autowiring/test/GuardObjectTest.cpp | 4 ++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/autowiring/unlock_object.h b/autowiring/unlock_object.h index c1a1da04d..d2de9e780 100644 --- a/autowiring/unlock_object.h +++ b/autowiring/unlock_object.h @@ -9,7 +9,7 @@ /// /// ///An unlock_object cannot be copied by construction or assignment since it maintains access. -///An unlock_object and cannot be used to instantiate new shared_object instances, +///An unlock_object cannot be used to instantiate new shared_object instances, ///because it is not guaranteed to reference a shared_object. ///An unlock_object cannot be applied to an atomic_object, since continued existence of the ///referenced object would not be guaranteed. @@ -18,6 +18,7 @@ template class unlock_object { unlock_object(unlock_object& source) = delete; unlock_object& operator = (unlock_object& _rhs) = delete; + unlock_object* operator & () = delete; std::shared_ptr> m_share; @@ -61,12 +62,12 @@ class unlock_object { } /// - ///Reset releases reference and lock, yielding bool(this) == false; + ///Releases reference and lock, yielding bool(this) == false; /// /// ///This method is idempotent. /// - void reset() { + void release() { if (m_share) { m_share->m_lock.unlock(); m_share.reset(); @@ -74,16 +75,16 @@ class unlock_object { } /// - ///Reset re-targets reference and lock to source, yielding bool(this) == false; + ///Aquires a lock on and reference to source, yielding unlock_object::operator bool(this) == true; /// ///If true, the returned unlock_object might hold no reference or lock /// ///This method is idempotent, including when called repeatedly with the same argument. ///However, reset(source) always releases the any currently held lock. /// - void reset(shared_object& source, bool should_try = false) { + void acquire(shared_object& source, bool should_try = false) { if (m_share) - reset(); + release(); if(should_try) try_unlock(source); else diff --git a/src/autowiring/test/GuardObjectTest.cpp b/src/autowiring/test/GuardObjectTest.cpp index 046d8ac79..3b807575f 100644 --- a/src/autowiring/test/GuardObjectTest.cpp +++ b/src/autowiring/test/GuardObjectTest.cpp @@ -64,11 +64,11 @@ TEST_F(GuardObjectTest, UnlockTests) { ASSERT_TRUE(static_cast(so_unlock3)); ASSERT_TRUE(*so_unlock3 == 1); - so_unlock3.reset(so2); //reset with argument + so_unlock3.acquire(so2); //reset with argument ASSERT_TRUE(static_cast(so_unlock3)); ASSERT_TRUE(*so_unlock3 == 2); - so_unlock3.reset(so2); //reset is idempotent + so_unlock3.acquire(so2); //reset is idempotent ASSERT_TRUE(static_cast(so_unlock3)); ASSERT_TRUE(*so_unlock3 == 2); } From 43879a2d7309e6dd75ae9c743c84e41384087da7 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Wed, 16 Jul 2014 00:26:35 -0700 Subject: [PATCH 04/12] Adding reset(target) and transfer(target) methods to atomic_object, and by extension to shared_object. NOTE: It is not clear that these conditional transfer methods are fundamental to atomic_object. Instead, write a class that provides conditional assignment and transfer methods... which seems like a separate project. NOTE: It will then be necessary to extend the unlock_object functionality to an atomic array of unlocks. --- autowiring/atomic_object.h | 70 +++++++++++++++++++++++++++++++++++--- autowiring/shared_object.h | 50 +++++++++++++++++++++++---- 2 files changed, 108 insertions(+), 12 deletions(-) diff --git a/autowiring/atomic_object.h b/autowiring/atomic_object.h index eeea71c4a..f0d507413 100644 --- a/autowiring/atomic_object.h +++ b/autowiring/atomic_object.h @@ -94,6 +94,31 @@ class atomic_object { return m_object; } + /// + ///Reset using default constructor yielding initialized() == false. + /// + ///True if the object was not assigned default values + bool reset() { + std::lock_guard lock_this(m_lock); + bool was_initialized = m_initialized; + m_initialized = false; + m_object = object(); + return was_initialized; + } + + /// + ///Atomic copy of target to this object, only if initialized() == false. + /// + ///True if the object was not assigned default values + bool reset(const object& target) { + std::lock_guard lock_this(m_lock); + bool was_initialized = m_initialized; + if (!m_initialized) + m_object = target; + m_initialized = true; + return was_initialized; + } + ///True if the object was not assigned default values bool initialized() const { std::lock_guard lock_this(m_lock); @@ -104,7 +129,7 @@ class atomic_object { ///Atomic copy of this object to target, only if initialized() == true. /// ///True if the object was not assigned default values - bool initialized(object& target) { + bool initialized(object& target) const { std::lock_guard lock_this(m_lock); if (m_initialized) target = m_object; @@ -112,11 +137,46 @@ class atomic_object { } /// - ///Reset using default constructor yielding initialized() == false. + ///If uninitialized uses target for initialization. + ///If initialized assigns current value to target. /// - void reset() { + /// Returns +1 for transfer from target to this, -1 for transfer from this to target + int transfer(object& target) { std::lock_guard lock_this(m_lock); - m_initialized = false; - m_object = object(); + int val = 0; + if (m_initialized) { + target = m_object; + val = +1; + } else { + m_object = target; + m_initialized = true; + val = -1; + } + return val; + } + + /// + ///If neither this nor target are uninitialized, no transfer occurs. + ///If this is uninitialized and target is not, then this is initialized by target. + ///If target is uninitialized and this is, then target is initialized by this. + ///If both this and target are initialized, no transfer occurs. + /// + /// Returns +1 for transfer from target to this, -1 for transfer from this to target, else 0 + int transfer(atomic_object& target) { + std::lock(m_lock, target.m_lock); + int val = 0; + if (m_initialized && !target.m_initialized) { + target.m_object = m_object; + target.m_initialized = true; + val = -1; + } + if (!m_initialized && target.m_initialized) { + m_object = target.m_object; + m_initialized = true; + val = +1; + } + m_lock.unlock(); + target.m_lock.unlock(); + return val; } }; diff --git a/autowiring/shared_object.h b/autowiring/shared_object.h index 490e73b40..b32e5ac8a 100644 --- a/autowiring/shared_object.h +++ b/autowiring/shared_object.h @@ -88,6 +88,14 @@ class shared_object { return *m_share; } + /// + ///Reset using default constructor yielding initialized() == false. + /// + ///True if the object was not assigned default values + bool reset() { + return m_share->reset(); + } + ///True if the object was not assigned default values bool initialized() const { return m_share->initialized(); @@ -97,18 +105,46 @@ class shared_object { ///Atomic copy of target to this object, only if initialized() == false. /// ///True if the object was not assigned default values + bool reset(const object& target) { + return m_share->reset(target); + } + + /// + ///Atomic copy of this object to target, only if initialized() == true. + /// + ///True if the object was not assigned default values bool initialized(object& target) { return m_share->initialized(target); } /// - ///Reset using default constructor yielding initialized() == false. + ///If uninitialized uses target for initialization. + ///If initialized assigns current value to target. /// - /// - ///This method is equivalent to: - /// unlock_object(source)->reset(); - /// - void reset() { - m_share->reset(); + /// Returns +1 for transfer from target to this, -1 for transfer from this to target + int transfer(object& target) { + return m_share->transfer(target); + } + + /// + ///If neither this nor target are uninitialized, no transfer occurs. + ///If this is uninitialized and target is not, then this is initialized by target. + ///If target is uninitialized and this is, then target is initialized by this. + ///If both this and target are initialized, no transforer occurs. + /// + /// Returns +1 for transfer from target to this, -1 for transfer from this to target, else 0 + int transfer(atomic_object& target) { + return m_share->transfer(target); + } + + /// + ///If neither this nor target are uninitialized, no transfer occurs. + ///If this is uninitialized and target is not, then this is initialized by target. + ///If target is uninitialized and this is, then target is initialized by this. + ///If both this and target are initialized, no transforer occurs. + /// + /// Returns +1 for transfer from target to this, -1 for transfer from this to target, else 0 + int transfer(shared_object& target) { + return m_share->transfer(target.m_share); } }; From eeb315fa720647e588220f9de5e0cbcaf3d7a5d7 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Mon, 4 Aug 2014 15:53:43 -0700 Subject: [PATCH 05/12] Finished implementation of AutoSelfUpdate, which is used to provide prior data. --- autowiring/AutoSelfUpdate.h | 37 ++++++++++++++++++++++++++++++------- autowiring/shared_object.h | 6 ++++++ 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/autowiring/AutoSelfUpdate.h b/autowiring/AutoSelfUpdate.h index 1882f57e3..4eb7e24f8 100644 --- a/autowiring/AutoSelfUpdate.h +++ b/autowiring/AutoSelfUpdate.h @@ -1,7 +1,7 @@ // Copyright (C) 2012-2014 Leap Motion, Inc. All rights reserved. #pragma once #include "Autowiring/NewAutoFilter.h" -#include "Autowiring/atomic_object.h" +#include "Autowiring/shared_object.h" /// ///Enables an automatic self-update when a packet is decorated with the object type. @@ -9,20 +9,43 @@ /// /// ///In order to ensure that this method will be consistent with any other AutoFilter calls, -///the object inherits from atomic_object, which implements basic locking functionality. +///the object inherits from shared_object, which implements basic locking functionality. /// template class AutoSelfUpdate: -public atomic_object { +public shared_object { +protected: + using shared_object::get_lock; + using shared_object::get_object; + public: + AutoSelfUpdate() {} + AutoSelfUpdate(const shared_object& source) : shared_object(source) {} + AutoSelfUpdate(const object& source) : shared_object(source) {} + using shared_object::operator =; + using shared_object::operator object; + + //The distinct type assigned to the prior value of the object + class prior_object: public object { + public: + prior_object(const object& source): object(source) {} + }; + + //Avoid intermediate copy by defining an explicit cast + operator prior_object() const { + std::lock_guard lock_this(get_lock()); + return prior_object(get_object()); + } + //Decorates all packets with instances of prior_object void AutoFilter(AutoPacket& packet) { - //TODO: Decorate with prior + packet.Decorate(this->operator prior_object()); } - void SelfUpdate(object& update) { - atomic_object::operator = (update); + //Updates this object + void AutoGather(const object& update) { + shared_object::operator = (update); } - NewAutoFilter::SelfUpdate), &AutoSelfUpdate::SelfUpdate> AutoFilter_SelfUpdate; + NewAutoFilter::AutoGather), &AutoSelfUpdate::AutoGather> SelfUpdate; }; diff --git a/autowiring/shared_object.h b/autowiring/shared_object.h index b32e5ac8a..e14f7af7c 100644 --- a/autowiring/shared_object.h +++ b/autowiring/shared_object.h @@ -29,9 +29,15 @@ class shared_object { object& get_object() { return m_share->m_object; } + const object& get_object() const { + return m_share->m_object; + } bool& get_initialized() { return m_share->m_initialized; } + const bool& get_initialized() const { + return m_share->m_initialized; + } public: typedef unlock_object unlock; From adee2508606f1ce334b4f4a265fcc323d7a0a8d7 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Mon, 4 Aug 2014 17:15:02 -0700 Subject: [PATCH 06/12] Adding unit test for AutoSelfUpdate functionality. --- src/autowiring/test/AutoFilterTest.cpp | 41 +++++++++++--------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/src/autowiring/test/AutoFilterTest.cpp b/src/autowiring/test/AutoFilterTest.cpp index 305e84266..5296d499f 100644 --- a/src/autowiring/test/AutoFilterTest.cpp +++ b/src/autowiring/test/AutoFilterTest.cpp @@ -5,6 +5,7 @@ #include #include #include +#include "AutoSelfUpdate.h" #include THREAD_HEADER class AutoFilterTest: @@ -1005,32 +1006,26 @@ TEST_F(AutoFilterTest, SharedPointerAliasingRules) { ASSERT_EQ(1UL, genFilter2->m_called) << "AutoFilter accepting a decorated type was not called as expected"; } -TEST_F(AutoFilterTest, GetSharedPointer) { - // Attach a simple decoration +TEST_F(AutoFilterTest, AutoSelfUpdateTest) { + AutoCurrentContext()->Initiate(); AutoRequired factory; - auto packet = factory->NewPacket(); - packet->Decorate(Decoration<0>()); + AutoRequired>> filter; - // Verify we can get this decoration back - const std::shared_ptr>* dec; - packet->Get(dec); - ASSERT_TRUE(dec != nullptr) << "Failed to obtain a shared pointer for a decoration that was just added"; + { + auto packet = factory->NewPacket(); + ASSERT_TRUE(packet->Has>::prior_object>()) << "Missing status update from AutoSelfUpdate"; + Decoration<0> mod_deco; + mod_deco.i = 1; + packet->Decorate(mod_deco); + Decoration<0> get_deco = *filter; //Implicit cast from AutoSelfUpdate + ASSERT_EQ(1, get_deco.i) << "AutoSelfUpdate did not update"; + } - // Now add a bunch of other decorations: - packet->Decorate(Decoration<1>()); - packet->Decorate(Decoration<2>()); - packet->Decorate(Decoration<3>()); - packet->Decorate(Decoration<4>()); - packet->Decorate(Decoration<5>()); - packet->Decorate(Decoration<6>()); - packet->Decorate(Decoration<7>()); - packet->Decorate(Decoration<8>()); - packet->Decorate(Decoration<9>()); - - // Verify nothing moved: - const std::shared_ptr>* dec2; - packet->Get(dec2); - ASSERT_EQ(dec, dec2) << "Decoration was moved incorrectly after updates were made"; + { + auto packet = factory->NewPacket(); + Decoration<0> get_deco = packet->Get>::prior_object>(); //Implicit cast from prior_object + ASSERT_EQ(1, get_deco.i) << "Status updated yielded incorrect prior"; + } } TEST_F(AutoFilterTest, WaitWhilePacketOutstanding) { From 165f475ca098e6d1e3fe13bbbd00bed0a51f6a52 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Mon, 4 Aug 2014 17:29:30 -0700 Subject: [PATCH 07/12] Introduction of AutoSelfUpdate in multiple sub-contexts yields an error... but should be allowed. --- src/autowiring/test/AutoFilterTest.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/autowiring/test/AutoFilterTest.cpp b/src/autowiring/test/AutoFilterTest.cpp index 5296d499f..e97017ff0 100644 --- a/src/autowiring/test/AutoFilterTest.cpp +++ b/src/autowiring/test/AutoFilterTest.cpp @@ -1063,3 +1063,25 @@ TEST_F(AutoFilterTest, DeferredDecorateOnly) { ASSERT_TRUE(packet->Get(dec)) << "Deferred decorator didn't attach a decoration to an issued packet"; ASSERT_EQ(105, dec->i) << "Deferred decorate-only AutoFilter did not properly attach before context termination"; } + +TEST_F(AutoFilterTest, AutoSelfUpdateTwoContexts) { + AutoCreateContext contextA; + { + CurrentContextPusher pusher(contextA); + try { + AutoRequired>> filter; + } catch (...) { + FAIL() << "Failed to create AutoSelfUpdate in contextA"; + } + } + + AutoCreateContext contextB; + { + CurrentContextPusher pusher(contextB); + try { + AutoRequired>> filter; + } catch (...) { + FAIL() << "Failed to create AutoSelfUpdate in contextB"; + } + } +} From 99fc0322ba0b7948154918559c05ccea619820fb Mon Sep 17 00:00:00 2001 From: Jason Lokerson Date: Mon, 4 Aug 2014 18:13:19 -0700 Subject: [PATCH 08/12] Making use of dedicated ASSERT_NO_THROW macro --- src/autowiring/test/AutoFilterTest.cpp | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/autowiring/test/AutoFilterTest.cpp b/src/autowiring/test/AutoFilterTest.cpp index e97017ff0..0133e5f39 100644 --- a/src/autowiring/test/AutoFilterTest.cpp +++ b/src/autowiring/test/AutoFilterTest.cpp @@ -1068,20 +1068,12 @@ TEST_F(AutoFilterTest, AutoSelfUpdateTwoContexts) { AutoCreateContext contextA; { CurrentContextPusher pusher(contextA); - try { - AutoRequired>> filter; - } catch (...) { - FAIL() << "Failed to create AutoSelfUpdate in contextA"; - } + ASSERT_NO_THROW(AutoRequired>>()) << "Failed to create AutoSelfUpdate in contextA"; } AutoCreateContext contextB; { CurrentContextPusher pusher(contextB); - try { - AutoRequired>> filter; - } catch (...) { - FAIL() << "Failed to create AutoSelfUpdate in contextB"; - } + ASSERT_NO_THROW(AutoRequired>>()) << "Failed to create AutoSelfUpdate in contextB"; } } From d487ad7ec41fb10df10f0651783991dcda9f2b07 Mon Sep 17 00:00:00 2001 From: Jason Lokerson Date: Mon, 4 Aug 2014 20:07:27 -0700 Subject: [PATCH 09/12] SlotInformation should always store position and extent, even when already initialized --- autowiring/SlotInformation.h | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/autowiring/SlotInformation.h b/autowiring/SlotInformation.h index 61edc1e01..cd94cd43b 100644 --- a/autowiring/SlotInformation.h +++ b/autowiring/SlotInformation.h @@ -123,8 +123,6 @@ class SlotInformationStackLocation { // Current slot information: SlotInformation* m_pCur; - // If this type has additional AutoFilter routines, - // Information about the object being constructed while this stack location is valid: const void* m_pObj; size_t m_extent; @@ -150,18 +148,11 @@ class SlotInformationStackLocation { /// The pointer to the base of the space about to be constructed template static SlotInformationStackLocation PushStackLocation(T* pSpace) { - auto* pStump = &SlotInformationStump::s_stump; - - // If we're already initialized, then we have nothing to do. This line is an optimization; if there - // is a race here, then the worst-case scenario is an unneeded sequence of memory allocations that - // will only ever be referenced by this stack location. - if(pStump->bInitialized) - return SlotInformationStackLocation(pStump); - - // New stack location to enclose this stump. This stack location may be concurrent with respect - // to other threads, but only one thread will succeed in colonizing this stump with a chain of - // slot entries - return SlotInformationStackLocation(pStump, pSpace, sizeof(T)); + return SlotInformationStackLocation( + &SlotInformationStump::s_stump, + pSpace, + sizeof(T) + ); } /// From 9f482e05bb44e7732c4782d5af26811d870e0e92 Mon Sep 17 00:00:00 2001 From: Jason Lokerson Date: Thu, 7 Aug 2014 11:21:54 -0700 Subject: [PATCH 10/12] Clarifying some comments --- src/autowiring/SlotInformation.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/autowiring/SlotInformation.cpp b/src/autowiring/SlotInformation.cpp index d6f4c96b5..65d4bd02d 100644 --- a/src/autowiring/SlotInformation.cpp +++ b/src/autowiring/SlotInformation.cpp @@ -28,8 +28,8 @@ SlotInformationStackLocation::~SlotInformationStackLocation(void) { tss.reset(m_pPrior); if( - // Do not attempt the update if this stump is initialized already, it could - // mean that our value is presently invalid + // Do not attempt the update if this stump is initialized already, because it + // means that someone else has already filled this in. !m_pStump->bInitialized && // We can only end here if the swap succeeds, otherwise we're going to have @@ -42,7 +42,8 @@ SlotInformationStackLocation::~SlotInformationStackLocation(void) { return; } - // Stump already filled in, prevent leaks by destroying this stump entry: + // Stump already filled in by someone else before we could get to it. We don't want + // to leak memory, so we need to destroy our linked list of slot informations. std::unique_ptr prior; for(const auto* cur = m_pCur; cur; cur = cur->pFlink) prior.reset(cur); From 157ccb1fd1f944c2d7c96d38d2d463a19f55324d Mon Sep 17 00:00:00 2001 From: Jason Lokerson Date: Thu, 7 Aug 2014 12:47:52 -0700 Subject: [PATCH 11/12] Corrected invalid use-after-free --- autowiring/NewAutoFilter.h | 7 ----- autowiring/SlotInformation.h | 23 +++++++++++++-- src/autowiring/AutoPacketFactory.cpp | 2 -- src/autowiring/CoreContext.cpp | 2 +- src/autowiring/NewAutoFilter.cpp | 11 ++----- src/autowiring/SlotInformation.cpp | 43 ++++++++++++++++------------ 6 files changed, 47 insertions(+), 41 deletions(-) diff --git a/autowiring/NewAutoFilter.h b/autowiring/NewAutoFilter.h index 9e780aceb..9b3220519 100644 --- a/autowiring/NewAutoFilter.h +++ b/autowiring/NewAutoFilter.h @@ -8,13 +8,6 @@ class NewAutoFilterBase { protected: NewAutoFilterBase(const AutoFilterDescriptorStub& stub); - -public: - // The actual descriptor stub - const AutoFilterDescriptorStub& m_stub; - - // The next filter AutoFilter entry in the series - NewAutoFilterBase* pFlink; }; template diff --git a/autowiring/SlotInformation.h b/autowiring/SlotInformation.h index cd94cd43b..3f2f021e5 100644 --- a/autowiring/SlotInformation.h +++ b/autowiring/SlotInformation.h @@ -4,8 +4,18 @@ #include #include MEMORY_HEADER +struct AutoFilterDescriptorStub; class DeferrableAutowiring; -class NewAutoFilterBase; + +struct AutoFilterDescriptorStubLink { + AutoFilterDescriptorStubLink(const AutoFilterDescriptorStub& stub, const AutoFilterDescriptorStubLink* pFlink) : + stub(stub), + pFlink(pFlink) + {} + + const AutoFilterDescriptorStub& stub; + const AutoFilterDescriptorStubLink* const pFlink; +}; /// /// Represents information about a single slot detected as having been declared in a context member @@ -50,7 +60,7 @@ struct SlotInformationStumpBase { // If there are any custom AutoFilter fields defined, this is the first of them // Note that these custom fields -only- include fields registered via the AutoFilter // registration type - NewAutoFilterBase* pFirstAutoFilter; + const AutoFilterDescriptorStubLink* pFirstAutoFilter; }; /// @@ -123,6 +133,9 @@ class SlotInformationStackLocation { // Current slot information: SlotInformation* m_pCur; + // Most recent AutoFilter descriptor link: + AutoFilterDescriptorStubLink* m_pLastLink; + // Information about the object being constructed while this stack location is valid: const void* m_pObj; size_t m_extent; @@ -168,9 +181,13 @@ class SlotInformationStackLocation { /// /// Registers the named slot with the current stack location /// - /// static void RegisterSlot(DeferrableAutowiring* pAutowiring); + /// + /// Registers a NewAutoFilter with this SlotInformation + /// + static void RegisterSlot(const AutoFilterDescriptorStub& stub); + // Operator overloads: void operator=(SlotInformationStackLocation&& rhs) = delete; void operator=(const SlotInformationStackLocation& rhs) = delete; diff --git a/src/autowiring/AutoPacketFactory.cpp b/src/autowiring/AutoPacketFactory.cpp index 13fb1312c..f41cb1587 100644 --- a/src/autowiring/AutoPacketFactory.cpp +++ b/src/autowiring/AutoPacketFactory.cpp @@ -4,8 +4,6 @@ #include "AutoPacket.h" #include "thread_specific_ptr.h" -static autowiring::thread_specific_ptr pAFB; - AutoPacketFactory::AutoPacketFactory(void): ContextMember("AutoPacketFactory"), m_parent(GetContext()->GetParentContext()), diff --git a/src/autowiring/CoreContext.cpp b/src/autowiring/CoreContext.cpp index 6e4a02625..e2dfc687d 100644 --- a/src/autowiring/CoreContext.cpp +++ b/src/autowiring/CoreContext.cpp @@ -239,7 +239,7 @@ void CoreContext::AddInternal(const AddInternalTraits& traits) { // Ancilliary subscribers, if present: for(const auto* pCur = stump.pFirstAutoFilter; pCur; pCur = pCur->pFlink) { - AutoFilterDescriptor subscriber(traits.subscriber.GetAutoFilter(), pCur->m_stub); + AutoFilterDescriptor subscriber(traits.subscriber.GetAutoFilter(), pCur->stub); AddPacketSubscriber(subscriber); } } diff --git a/src/autowiring/NewAutoFilter.cpp b/src/autowiring/NewAutoFilter.cpp index bfdd37888..e0dbbb18b 100644 --- a/src/autowiring/NewAutoFilter.cpp +++ b/src/autowiring/NewAutoFilter.cpp @@ -4,8 +4,7 @@ #include "SlotInformation.h" #include "autowiring_error.h" -NewAutoFilterBase::NewAutoFilterBase(const AutoFilterDescriptorStub& stub) : - m_stub(stub) +NewAutoFilterBase::NewAutoFilterBase(const AutoFilterDescriptorStub& stub) { auto location = SlotInformationStackLocation::CurrentStackLocation(); @@ -13,11 +12,5 @@ NewAutoFilterBase::NewAutoFilterBase(const AutoFilterDescriptorStub& stub) : if(!location) throw autowiring_error("Attempted to make use of an AutoFilter entry without creating a type"); - // Verify that our created slot is in the body of the enclosing class - if(!location->Encloses(this)) - throw autowiring_error("AutoFilter entries may only be constructed precisely in the _body_ of a class, never on the stack"); - - auto stump = location->GetStump(); - pFlink = stump->pFirstAutoFilter; - stump->pFirstAutoFilter = this; + location->RegisterSlot(stub); } diff --git a/src/autowiring/SlotInformation.cpp b/src/autowiring/SlotInformation.cpp index 65d4bd02d..7926941c6 100644 --- a/src/autowiring/SlotInformation.cpp +++ b/src/autowiring/SlotInformation.cpp @@ -13,12 +13,25 @@ SlotInformationStackLocation::SlotInformationStackLocation(SlotInformationStumpB m_pPrior(tss.get()), m_pStump(pStump), m_pCur(nullptr), + m_pLastLink(nullptr), m_pObj(pObj), m_extent(extent) { tss.reset(this); } +template +void UpdateOrCascadeDelete(T* ptr, const T*& dest) { + if(!compare_exchange(&dest, ptr, nullptr)) + // Exchange passed, the destination now owns this pointer + return; + + // Failed the exchange, return here + std::unique_ptr prior; + for(const auto* cur = ptr; cur; cur = cur->pFlink) + prior.reset(cur); +} + SlotInformationStackLocation::~SlotInformationStackLocation(void) { if(!m_pStump) // Rvalue moved, end here @@ -27,26 +40,11 @@ SlotInformationStackLocation::~SlotInformationStackLocation(void) { // Replace the prior stack location, we were pushed tss.reset(m_pPrior); - if( - // Do not attempt the update if this stump is initialized already, because it - // means that someone else has already filled this in. - !m_pStump->bInitialized && - - // We can only end here if the swap succeeds, otherwise we're going to have - // to leave the value that's here already and tear down our list - !compare_exchange(&m_pStump->pHead, m_pCur, nullptr) - ) { - // No reason to CAS this boolean field--we're going to unconditionally make - // this spot initialized. - m_pStump->bInitialized = true; - return; - } + UpdateOrCascadeDelete(m_pCur, m_pStump->pHead); + UpdateOrCascadeDelete(m_pLastLink, m_pStump->pFirstAutoFilter); - // Stump already filled in by someone else before we could get to it. We don't want - // to leak memory, so we need to destroy our linked list of slot informations. - std::unique_ptr prior; - for(const auto* cur = m_pCur; cur; cur = cur->pFlink) - prior.reset(cur); + // Unconditionally update to true, no CAS needed + m_pStump->bInitialized = true; } SlotInformationStackLocation* SlotInformationStackLocation::CurrentStackLocation(void) { @@ -79,3 +77,10 @@ void SlotInformationStackLocation::RegisterSlot(DeferrableAutowiring* pDeferrabl false ); } + +void SlotInformationStackLocation::RegisterSlot(const AutoFilterDescriptorStub& stub) { + if(!tss.get() || tss->m_pStump->bInitialized) + return; + + tss->m_pLastLink = new AutoFilterDescriptorStubLink(stub, tss->m_pLastLink); +} From 415c7b90204e0835601fc96c090bd129da051103 Mon Sep 17 00:00:00 2001 From: Gabriel Hare Date: Thu, 7 Aug 2014 14:56:14 -0700 Subject: [PATCH 12/12] Cleaning up inclusion path specifications. --- autowiring/AutoSelfUpdate.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/autowiring/AutoSelfUpdate.h b/autowiring/AutoSelfUpdate.h index 4eb7e24f8..11cc47fb7 100644 --- a/autowiring/AutoSelfUpdate.h +++ b/autowiring/AutoSelfUpdate.h @@ -1,7 +1,7 @@ // Copyright (C) 2012-2014 Leap Motion, Inc. All rights reserved. #pragma once -#include "Autowiring/NewAutoFilter.h" -#include "Autowiring/shared_object.h" +#include "NewAutoFilter.h" +#include "shared_object.h" /// ///Enables an automatic self-update when a packet is decorated with the object type.