From patchwork Thu Jul 11 12:56:08 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 1655 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 2950160C2F for ; Thu, 11 Jul 2019 14:56:42 +0200 (CEST) Received: from pendragon.ideasonboard.com (softbank126163157105.bbtec.net [126.163.157.105]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id A605831C for ; Thu, 11 Jul 2019 14:56:40 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1562849801; bh=tzemjhXXhncskt5OqDuq3eX8HGMa0oJZgFDtzp5ry80=; h=From:To:Subject:Date:From; b=BGeP5OOdURJXJd0ZXtt8b+qSckCCOgNbixHF0g845lro5ISxM4qE1Aig47Ux+0sBv /NugyLACBaHDEfnia2nOb8zSX0zO2qC95QGIL/pJvCHZPvFHkIJnurGJRQk5Hxik/s IIV+WleEEMVe2ffRdMAv8dDNOR2cUHldwDsbOhN4= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Thu, 11 Jul 2019 15:56:08 +0300 Message-Id: <20190711125609.7331-1-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.21.0 MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 1/2] test: signal: Extend Signal test with multi-inheritance reeiver X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Jul 2019 12:56:42 -0000 Add tests that exercises the Object-related signal code paths (in particular automatic disconnection on Signal deletion) when the receiver inherits from multiple base classes, with Object being the second base. This tests the casts to and from Object * in the signal implementation. The new tests segfault due bugs in the signal/slot implementation. Signed-off-by: Laurent Pinchart --- test/signal.cpp | 74 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 71 insertions(+), 3 deletions(-) diff --git a/test/signal.cpp b/test/signal.cpp index 79668b421511..9d8f985d15e5 100644 --- a/test/signal.cpp +++ b/test/signal.cpp @@ -32,6 +32,29 @@ public: } }; +class BaseClass +{ +public: + /* + * A virtual method is required in the base class, otherwise the compiler + * will always store Object before BaseClass in memory. + */ + virtual ~BaseClass() + { + } + + unsigned int data_[32]; +}; + +class SlotMulti : public BaseClass, public Object +{ +public: + void slot() + { + valueStatic_ = 1; + } +}; + class SignalTest : public Test { protected: @@ -69,6 +92,8 @@ protected: int run() { + /* ----------------- Signal -> !Object tests ---------------- */ + /* Test signal emission and reception. */ called_ = false; signalVoid_.connect(this, &SignalTest::slotVoid); @@ -149,6 +174,8 @@ protected: return TestFail; } + /* ----------------- Signal -> Object tests ----------------- */ + /* * Test automatic disconnection on object deletion. Connect the * slot twice to ensure all instances are disconnected. @@ -170,10 +197,10 @@ protected: * Test that signal deletion disconnects objects. This shall * not generate any valgrind warning. */ - Signal<> *signal = new Signal<>(); + Signal<> *dynamicSignal = new Signal<>(); slotObject = new SlotObject(); - signal->connect(slotObject, &SlotObject::slot); - delete signal; + dynamicSignal->connect(slotObject, &SlotObject::slot); + delete dynamicSignal; delete slotObject; /* Exercise the Object slot code paths. */ @@ -188,6 +215,47 @@ protected: delete slotObject; + /* --------- Signal -> Object (multiple inheritance) -------- */ + + /* + * Test automatic disconnection on object deletion. Connect the + * slot twice to ensure all instances are disconnected. + */ + signalVoid_.disconnect(); + + SlotMulti *slotMulti = new SlotMulti(); + signalVoid_.connect(slotMulti, &SlotMulti::slot); + signalVoid_.connect(slotMulti, &SlotMulti::slot); + delete slotMulti; + valueStatic_ = 0; + signalVoid_.emit(); + if (valueStatic_ != 0) { + cout << "Signal disconnection on object deletion test failed" << endl; + return TestFail; + } + + /* + * Test that signal deletion disconnects objects. This shall + * not generate any valgrind warning. + */ + dynamicSignal = new Signal<>(); + slotMulti = new SlotMulti(); + dynamicSignal->connect(slotMulti, &SlotMulti::slot); + delete dynamicSignal; + delete slotMulti; + + /* Exercise the Object slot code paths. */ + slotMulti = new SlotMulti(); + signalVoid_.connect(slotMulti, &SlotMulti::slot); + valueStatic_ = 0; + signalVoid_.emit(); + if (valueStatic_ == 0) { + cout << "Signal delivery for Object test failed" << endl; + return TestFail; + } + + delete slotMulti; + return TestPass; } From patchwork Thu Jul 11 12:56:09 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 1656 Return-Path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id DA01661574 for ; Thu, 11 Jul 2019 14:56:43 +0200 (CEST) Received: from pendragon.ideasonboard.com (softbank126163157105.bbtec.net [126.163.157.105]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 7963531C for ; Thu, 11 Jul 2019 14:56:42 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1562849803; bh=ygvYERfLsYBwdYMhEybRmX/HKMd2chZAHc/fVD7txAA=; h=From:To:Subject:Date:In-Reply-To:References:From; b=UnLLaQ24CRXujkj0c0CHfkoypXs2rKxyWlnCYhl1BwtFRqserC2cZ8cCKtylPAlad csSEOZEUvdrocSrKSBFeYGSCw2kMhdk9tN3ThjsNmfOgQio+kQsDTBXTU8zXsUgqBP q1tfFAvCvK0l79OxPvkVE+nOlDtx6P9CbMQYQeU0= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Thu, 11 Jul 2019 15:56:09 +0300 Message-Id: <20190711125609.7331-2-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.21.0 In-Reply-To: <20190711125609.7331-1-laurent.pinchart@ideasonboard.com> References: <20190711125609.7331-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 2/2] libcamera: signal: Fix Object handling in multiple inheritance cases X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Jul 2019 12:56:44 -0000 The SlotBase implementation stores the receiver object pointer as a void pointer internally. The pointer is then cast back to an Object pointer when the receiver object class derives from Object. When the receiver is an object that inherits from both the Object class and other classes, the Object data members may not be stored at the beginning of the object memory. The cast back to an Object pointer is thus incorrect. Fix this by casting the receiver object pointer to an Object pointer where the type of the receiver object is known, and pass it along with the receiver void pointer to the SlotBase class. The SlotBase class stores both pointers internally, and doesn't need the isObject_ field anymore as the same information is obtained from checking if the Object pointer is null. To avoid confusing the two pointers, use the same naming scheme through the whole implementation: "obj" points to a receiver object as an unknown type, and "object" to the receiver object cast to an Object. The latter is null when the receiver object doesn't inherit from the Object class. To further clarify the code, remove direct access to the SlotBase "obj" and "object" fields as much as possible. They are replaced by two new methods : - SlotBase::disconnect() to disconnect a signal from the slot's receiver object - SlotBase::match() to test if an object pointer matches the slot The match() method is a template method with a specialisation for the Object type, to compare either the obj or the object pointer depending on the type of the parameter. This is required as the Object destructor calls the SignalBase::disconnect() method for signal connected to the object, and passes a pointer to Object to that method, while the actual object may have a different address due to the issue explained above. The pointer must thus be compared with the stored Object pointer in that case, not to the pointer to the receiver object. Signed-off-by: Laurent Pinchart --- include/libcamera/object.h | 4 ++- include/libcamera/signal.h | 55 +++++++++++++++++++++----------------- src/libcamera/signal.cpp | 8 +++++- 3 files changed, 40 insertions(+), 27 deletions(-) diff --git a/include/libcamera/object.h b/include/libcamera/object.h index d61dfb1ebaef..5c251a822d9a 100644 --- a/include/libcamera/object.h +++ b/include/libcamera/object.h @@ -13,9 +13,10 @@ namespace libcamera { class Message; -class SignalBase; template class Signal; +class SignalBase; +class SlotBase; class Thread; class Object @@ -33,6 +34,7 @@ public: private: template friend class Signal; + friend class SlotBase; friend class Thread; void connect(SignalBase *signal); diff --git a/include/libcamera/signal.h b/include/libcamera/signal.h index 11ffb857a59c..d3be362637f8 100644 --- a/include/libcamera/signal.h +++ b/include/libcamera/signal.h @@ -18,23 +18,28 @@ namespace libcamera { template class Signal; +class SignalBase; class SlotBase { public: - SlotBase(void *obj, bool isObject) - : obj_(obj), isObject_(isObject) {} + SlotBase(void *obj, Object *object) + : obj_(obj), object_(object) {} virtual ~SlotBase() {} - void *obj() { return obj_; } - bool isObject() const { return isObject_; } + template + bool match(T *obj) { return obj == obj_; } + template<> + bool match(Object *object) { return object == object_; } + + void disconnect(SignalBase *signal); void activatePack(void *pack); virtual void invokePack(void *pack) = 0; protected: void *obj_; - bool isObject_; + Object *object_; }; template @@ -71,8 +76,8 @@ private: } public: - SlotArgs(void *obj, bool isObject) - : SlotBase(obj, isObject) {} + SlotArgs(void *obj, Object *object) + : SlotBase(obj, object) {} void invokePack(void *pack) override { @@ -89,12 +94,12 @@ class SlotMember : public SlotArgs public: using PackType = std::tuple::type...>; - SlotMember(T *obj, bool isObject, void (T::*func)(Args...)) - : SlotArgs(obj, isObject), func_(func) {} + SlotMember(T *obj, Object *object, void (T::*func)(Args...)) + : SlotArgs(obj, object), func_(func) {} void activate(Args... args) { - if (this->isObject_) + if (this->object_) SlotBase::activatePack(new PackType{ args... }); else (static_cast(this->obj_)->*func_)(args...); @@ -115,7 +120,7 @@ class SlotStatic : public SlotArgs { public: SlotStatic(void (*func)(Args...)) - : SlotArgs(nullptr, false), func_(func) {} + : SlotArgs(nullptr, nullptr), func_(func) {} void activate(Args... args) { (*func_)(args...); } void invoke(Args... args) {} @@ -129,11 +134,11 @@ class SignalBase { public: template - void disconnect(T *object) + void disconnect(T *obj) { for (auto iter = slots_.begin(); iter != slots_.end(); ) { SlotBase *slot = *iter; - if (slot->obj() == object) { + if (slot->match(obj)) { iter = slots_.erase(iter); delete slot; } else { @@ -155,27 +160,27 @@ public: ~Signal() { for (SlotBase *slot : slots_) { - if (slot->isObject()) - static_cast(slot->obj())->disconnect(this); + slot->disconnect(this); delete slot; } } #ifndef __DOXYGEN__ template::value>::type * = nullptr> - void connect(T *object, void (T::*func)(Args...)) + void connect(T *obj, void (T::*func)(Args...)) { + Object *object = static_cast(obj); object->connect(this); - slots_.push_back(new SlotMember(object, true, func)); + slots_.push_back(new SlotMember(obj, object, func)); } template::value>::type * = nullptr> #else template #endif - void connect(T *object, void (T::*func)(Args...)) + void connect(T *obj, void (T::*func)(Args...)) { - slots_.push_back(new SlotMember(object, false, func)); + slots_.push_back(new SlotMember(obj, nullptr, func)); } void connect(void (*func)(Args...)) @@ -191,23 +196,23 @@ public: } template - void disconnect(T *object) + void disconnect(T *obj) { - SignalBase::disconnect(object); + SignalBase::disconnect(obj); } template - void disconnect(T *object, void (T::*func)(Args...)) + void disconnect(T *obj, void (T::*func)(Args...)) { for (auto iter = slots_.begin(); iter != slots_.end(); ) { SlotArgs *slot = static_cast *>(*iter); /* - * If the obj() pointer matches the object, the slot is + * If the object matches the slot, the slot is * guaranteed to be a member slot, so we can safely * cast it to SlotMember and access its * func_ member. */ - if (slot->obj() == object && + if (slot->match(obj) && static_cast *>(slot)->func_ == func) { iter = slots_.erase(iter); delete slot; @@ -221,7 +226,7 @@ public: { for (auto iter = slots_.begin(); iter != slots_.end(); ) { SlotArgs *slot = *iter; - if (slot->obj() == nullptr && + if (slot->match(nullptr) && static_cast *>(slot)->func_ == func) { iter = slots_.erase(iter); delete slot; diff --git a/src/libcamera/signal.cpp b/src/libcamera/signal.cpp index 53c18535fee3..ab7dba508dfb 100644 --- a/src/libcamera/signal.cpp +++ b/src/libcamera/signal.cpp @@ -57,9 +57,15 @@ namespace libcamera { * passed through the signal will remain valid after the signal is emitted. */ +void SlotBase::disconnect(SignalBase *signal) +{ + if (object_) + object_->disconnect(signal); +} + void SlotBase::activatePack(void *pack) { - Object *obj = static_cast(obj_); + Object *obj = static_cast(object_); if (Thread::current() == obj->thread()) { invokePack(pack);