From 8d99ee1f6af693ad45dcd03fcae6a86f8f86302a Mon Sep 17 00:00:00 2001 From: Pankaj Pandey Date: Tue, 27 May 2014 12:21:39 +0530 Subject: Improve the 'Value' type wrapper registration This commit works around some bugs where multiple/incorrect wrappers were registered for some types: - In cases where the first field of a class was itself a Value type instance (instead of pointer), both the parent and child wrappers had same cptr address, causing confusion in retrieveWrapper. Previously, this was worked around by always creating a new wrapper for all Value type fields on every access, causing leaks. We now check for colocated child wrappers and return that instead of creating new wrapper, so each Value type subfield need only have one wrapper. - Some cases of incorrect wrapper registration due to an existing wrapper which shiboken could not figure being deleted are fixed, specifically cases where the newly registered wrapper is from object created in python or owns its wrapper. - Do not release incorrect wrapper in case of address reuse by checking that the registered wrapper is indeed the wrapper being released. Task-number: PYSIDE-217 Task-number: PYSIDE-224 Change-Id: I019c078566c4b9b90e63c5d991e2e372d39c632a Reviewed-by: Alexandru Croitor Reviewed-by: Christian Tismer --- generator/shiboken2/cppgenerator.cpp | 36 ++++++++++++++- libshiboken/basewrapper.cpp | 77 ++++++++++++++++++++++++++++--- libshiboken/basewrapper.h | 6 +++ libshiboken/bindingmanager.cpp | 16 +++++-- tests/libsample/protected.h | 12 +++-- tests/samplebinding/protected_test.py | 20 +++++++- tests/samplebinding/typesystem_sample.xml | 2 + 7 files changed, 152 insertions(+), 17 deletions(-) diff --git a/generator/shiboken2/cppgenerator.cpp b/generator/shiboken2/cppgenerator.cpp index c8e919c..f0c2fe4 100644 --- a/generator/shiboken2/cppgenerator.cpp +++ b/generator/shiboken2/cppgenerator.cpp @@ -1577,6 +1577,14 @@ void CppGenerator::writeConstructorWrapper(QTextStream &s, const AbstractMetaFun // Python owns it and C++ wrapper is false. if (shouldGenerateCppWrapper(overloads.first()->ownerClass())) s << INDENT << "Shiboken::Object::setHasCppWrapper(sbkSelf, true);" << endl; + // Need to check if a wrapper for same pointer is already registered + // Caused by bug PYSIDE-217, where deleted objects' wrappers are not released + s << INDENT << "if (Shiboken::BindingManager::instance().hasWrapper(cptr)) {" << endl; + { + Indentation indent(INDENT); + s << INDENT << "Shiboken::BindingManager::instance().releaseWrapper(Shiboken::BindingManager::instance().retrieveWrapper(cptr));" << endl; + } + s << INDENT << "}" << endl; s << INDENT << "Shiboken::BindingManager::instance().registerWrapper(sbkSelf, cptr);" << endl; // Create metaObject and register signal/slot @@ -4011,12 +4019,38 @@ void CppGenerator::writeGetterFunction(QTextStream &s, cppField = QLatin1String("fieldValue"); } - s << INDENT << "PyObject* pyOut = "; + s << INDENT << "PyObject* pyOut = 0;\n"; if (newWrapperSameObject) { + // Special case colocated field with same address (first field in a struct) + s << INDENT << "if (reinterpret_cast(" + << cppField + << ") == reinterpret_cast(" + << CPP_SELF_VAR << ")) {\n"; + { + Indentation indent(INDENT); + s << INDENT << "pyOut = (PyObject*)Shiboken::Object::findColocatedChild(" + << "(SbkObject*)self , (SbkObjectType*)" + << cpythonTypeNameExt(fieldType) + << ");" << "\n"; + s << INDENT << "if (pyOut) {Py_IncRef(pyOut); return pyOut;}\n"; + } + s << INDENT << "}\n"; + // Check if field wrapper has already been created. + s << INDENT << "else if (Shiboken::BindingManager::instance().hasWrapper(" << cppField << ")) {" << "\n"; + { + Indentation indent(INDENT); + s << INDENT << "pyOut = (PyObject*)Shiboken::BindingManager::instance().retrieveWrapper(" << cppField << ");" << "\n"; + s << INDENT << "Py_IncRef(pyOut);" << "\n"; + s << INDENT << "return pyOut;" << "\n"; + } + s << INDENT << "}\n"; + // Create and register new wrapper + s << INDENT << "pyOut = "; s << "Shiboken::Object::newObject((SbkObjectType*)" << cpythonTypeNameExt(fieldType); s << ", " << cppField << ", false, true);" << endl; s << INDENT << "Shiboken::Object::setParent(" PYTHON_SELF_VAR ", pyOut)"; } else { + s << INDENT << "pyOut = "; writeToPythonConversion(s, fieldType, metaField->enclosingClass(), cppField); } s << ';' << endl; diff --git a/libshiboken/basewrapper.cpp b/libshiboken/basewrapper.cpp index 71abbb6..d82297d 100644 --- a/libshiboken/basewrapper.cpp +++ b/libshiboken/basewrapper.cpp @@ -1105,7 +1105,38 @@ bool isValid(PyObject* pyObj, bool throwPyError) return isValid(reinterpret_cast(pyObj), throwPyError); } -PyObject* newObject(SbkObjectType* instanceType, +SbkObject *findColocatedChild(SbkObject* wrapper, + SbkObjectType* instanceType) +{ + // Degenerate case, wrapper is the correct wrapper. + if (reinterpret_cast(Py_TYPE(wrapper)) == reinterpret_cast(instanceType)) + return wrapper; + + if (!(wrapper->d && wrapper->d->cptr)) + return 0; + + ParentInfo* pInfo = wrapper->d->parentInfo; + if (!pInfo) + return 0; + + ChildrenList& children = pInfo->children; + + ChildrenList::iterator childrenEnd = children.end(); + for (ChildrenList::iterator iChild = children.begin(); iChild != childrenEnd; ++iChild) { + if (!((*iChild)->d && (*iChild)->d->cptr)) + continue; + if ((*iChild)->d->cptr[0] == wrapper->d->cptr[0]) { + if (reinterpret_cast(Py_TYPE(*iChild)) == reinterpret_cast(instanceType)) + return const_cast((*iChild)); + else + return findColocatedChild(const_cast(*iChild), instanceType); + } + } + return 0; +} + + +PyObject *newObject(SbkObjectType* instanceType, void* cptr, bool hasOwnership, bool isExactType, @@ -1123,11 +1154,45 @@ PyObject* newObject(SbkObjectType* instanceType, instanceType = BindingManager::instance().resolveType(&cptr, instanceType); } - SbkObject* self = reinterpret_cast(SbkObjectTpNew(reinterpret_cast(instanceType), 0, 0)); - self->d->cptr[0] = cptr; - self->d->hasOwnership = hasOwnership; - self->d->validCppObject = 1; - BindingManager::instance().registerWrapper(self, cptr); + bool shouldCreate = true; + bool shouldRegister = true; + SbkObject* self = 0; + + // Some logic to ensure that colocated child field does not overwrite the parent + if (BindingManager::instance().hasWrapper(cptr)) { + SbkObject* existingWrapper = BindingManager::instance().retrieveWrapper(cptr); + + self = findColocatedChild(existingWrapper, instanceType); + if (self) { + // Wrapper already registered for cptr. + // This should not ideally happen, binding code should know when a wrapper + // already exists and retrieve it instead. + shouldRegister = shouldCreate = false; + } else if (hasOwnership && + (!(Shiboken::Object::hasCppWrapper(existingWrapper) || + Shiboken::Object::hasOwnership(existingWrapper)))) { + // Old wrapper is likely junk, since we have ownership and it doesn't. + BindingManager::instance().releaseWrapper(existingWrapper); + } else { + // Old wrapper may be junk caused by some bug in identifying object deletion + // but it may not be junk when a colocated field is accessed for an + // object which was not created by python (returned from c++ factory function). + // Hence we cannot release the wrapper confidently so we do not register. + shouldRegister = false; + } + } + + if (shouldCreate) { + self = reinterpret_cast(SbkObjectTpNew(reinterpret_cast(instanceType), 0, 0)); + self->d->cptr[0] = cptr; + self->d->hasOwnership = hasOwnership; + self->d->validCppObject = 1; + if (shouldRegister) { + BindingManager::instance().registerWrapper(self, cptr); + } + } else { + Py_IncRef(reinterpret_cast(self)); + } return reinterpret_cast(self); } diff --git a/libshiboken/basewrapper.h b/libshiboken/basewrapper.h index 8152f98..3404198 100644 --- a/libshiboken/basewrapper.h +++ b/libshiboken/basewrapper.h @@ -251,6 +251,12 @@ LIBSHIBOKEN_API bool isUserType(PyObject* pyObj); */ LIBSHIBOKEN_API Py_hash_t hash(PyObject* pyObj); +/** + * Find a child of given wrapper having same address having the specified type. + */ +LIBSHIBOKEN_API SbkObject *findColocatedChild(SbkObject* wrapper, + SbkObjectType* instanceType); + /** * Bind a C++ object to Python. * \param instanceType equivalent Python type for the C++ object. diff --git a/libshiboken/bindingmanager.cpp b/libshiboken/bindingmanager.cpp index 5fbd11d..d7e122c 100644 --- a/libshiboken/bindingmanager.cpp +++ b/libshiboken/bindingmanager.cpp @@ -143,16 +143,22 @@ struct BindingManager::BindingManagerPrivate { bool destroying; BindingManagerPrivate() : destroying(false) {} - void releaseWrapper(void* cptr); + bool releaseWrapper(void* cptr, SbkObject* wrapper); void assignWrapper(SbkObject* wrapper, const void* cptr); }; -void BindingManager::BindingManagerPrivate::releaseWrapper(void* cptr) +bool BindingManager::BindingManagerPrivate::releaseWrapper(void* cptr, SbkObject* wrapper) { + // The wrapper argument is checked to ensure that the correct wrapper is released. + // Returns true if the correct wrapper is found and released. + // If wrapper argument is NULL, no such check is performed. WrapperMap::iterator iter = wrapperMapper.find(cptr); - if (iter != wrapperMapper.end()) + if (iter != wrapperMapper.end() && (wrapper == 0 || iter->second == wrapper)) { wrapperMapper.erase(iter); + return true; + } + return false; } void BindingManager::BindingManagerPrivate::assignWrapper(SbkObject* wrapper, const void* cptr) @@ -234,12 +240,12 @@ void BindingManager::releaseWrapper(SbkObject* sbkObj) void** cptrs = reinterpret_cast(sbkObj)->d->cptr; for (int i = 0; i < numBases; ++i) { unsigned char *cptr = reinterpret_cast(cptrs[i]); - m_d->releaseWrapper(cptr); + m_d->releaseWrapper(cptr, sbkObj); if (d && d->mi_offsets) { int* offset = d->mi_offsets; while (*offset != -1) { if (*offset > 0) - m_d->releaseWrapper(cptr + *offset); + m_d->releaseWrapper(reinterpret_cast((std::size_t) cptr + (*offset)), sbkObj); offset++; } } diff --git a/tests/libsample/protected.h b/tests/libsample/protected.h index fe1d40f..c33bdf4 100644 --- a/tests/libsample/protected.h +++ b/tests/libsample/protected.h @@ -127,19 +127,25 @@ class LIBSAMPLE_API ProtectedProperty { public: ProtectedProperty() - : protectedProperty(0), + : protectedValueTypeProperty(Point(0, 0)), + protectedProperty(0), protectedEnumProperty(Event::NO_EVENT), - protectedValueTypeProperty(Point(0, 0)), protectedValueTypePointerProperty(0), protectedObjectTypeProperty(0) {} protected: + // This is deliberately the first member to test wrapper registration + // for value type members sharing the same memory address. + Point protectedValueTypeProperty; int protectedProperty; std::list protectedContainerProperty; Event::EventType protectedEnumProperty; - Point protectedValueTypeProperty; Point* protectedValueTypePointerProperty; ObjectType* protectedObjectTypeProperty; }; +LIBSAMPLE_API inline ProtectedProperty* createProtectedProperty() { + return new ProtectedProperty; +} + #endif // PROTECTED_H diff --git a/tests/samplebinding/protected_test.py b/tests/samplebinding/protected_test.py index 43b7640..9201a63 100644 --- a/tests/samplebinding/protected_test.py +++ b/tests/samplebinding/protected_test.py @@ -36,7 +36,7 @@ import unittest from sample import cacheSize from sample import ProtectedNonPolymorphic, ProtectedVirtualDestructor from sample import ProtectedPolymorphic, ProtectedPolymorphicDaughter, ProtectedPolymorphicGrandDaughter -from sample import ProtectedProperty, ProtectedEnumClass +from sample import createProtectedProperty, ProtectedProperty, ProtectedEnumClass from sample import PrivateDtor from sample import Event, ObjectType, Point @@ -300,7 +300,23 @@ class ProtectedPropertyTest(unittest.TestCase): self.assertEqual(self.obj.protectedValueTypeProperty, point) self.assertFalse(self.obj.protectedValueTypeProperty is point) pointProperty = self.obj.protectedValueTypeProperty - self.assertFalse(self.obj.protectedValueTypeProperty is pointProperty) + self.assertTrue(self.obj.protectedValueTypeProperty is pointProperty) + + def testProtectedValueTypePropertyWrapperRegistration(self): + '''Access colocated protected value type property.''' + cache_size = cacheSize() + point = Point(12, 34) + obj = createProtectedProperty() + obj.protectedValueTypeProperty + self.assertEqual(obj.protectedValueTypeProperty.copy(), + obj.protectedValueTypeProperty) + obj.protectedValueTypeProperty = point + self.assertEqual(obj.protectedValueTypeProperty, point) + self.assertFalse(obj.protectedValueTypeProperty is point) + pointProperty = obj.protectedValueTypeProperty + self.assertTrue(obj.protectedValueTypeProperty is pointProperty) + del obj, point, pointProperty + self.assertEqual(cacheSize(), cache_size) def testProtectedValueTypePointerProperty(self): '''Writes and reads a protected value type pointer property.''' diff --git a/tests/samplebinding/typesystem_sample.xml b/tests/samplebinding/typesystem_sample.xml index 601b4b0..089f835 100644 --- a/tests/samplebinding/typesystem_sample.xml +++ b/tests/samplebinding/typesystem_sample.xml @@ -878,6 +878,8 @@ + +