diff options
10 files changed, 185 insertions, 34 deletions
diff --git a/sources/shiboken2/ApiExtractor/abstractmetalang.cpp b/sources/shiboken2/ApiExtractor/abstractmetalang.cpp index ffc138968..2ef0a9a21 100644 --- a/sources/shiboken2/ApiExtractor/abstractmetalang.cpp +++ b/sources/shiboken2/ApiExtractor/abstractmetalang.cpp @@ -735,17 +735,55 @@ bool AbstractMetaFunction::isDeprecated() const return false; } +// Auto-detect whether a function should be wrapped into +// Py_BEGIN_ALLOW_THREADS/Py_END_ALLOW_THREADS, that is, temporarily release +// the GIL (global interpreter lock). Doing so is required for any thread-wait +// functions, anything that might call a virtual function (potentially +// reimplemented in Python), and recommended for lengthy I/O or similar. +// It has performance costs, though. +bool AbstractMetaFunction::autoDetectAllowThread() const +{ + // Disallow for simple getter functions. + const bool maybeGetter = m_constant != 0 && m_type != nullptr + && m_arguments.isEmpty(); + return !maybeGetter; +} + +static QString msgDisallowThread(const AbstractMetaFunction *f) +{ + QString result; + QTextStream str(&result); + str <<"Disallowing threads for "; + if (auto c = f->declaringClass()) + str << c->name() << "::"; + str << f->name() << "()."; + return result; +} + bool AbstractMetaFunction::allowThread() const { - const FunctionModificationList &modifications = this->modifications(declaringClass()); - for (const FunctionModification &modification : modifications) { - if (modification.allowThread()) - return true; + using AllowThread = TypeSystem::AllowThread; + + if (m_cachedAllowThread < 0) { + AllowThread allowThread = AllowThread::Auto; + // Find a modification that specifies allowThread + const FunctionModificationList &modifications = this->modifications(declaringClass()); + for (const FunctionModification &modification : modifications) { + if (modification.allowThread() != AllowThread::Unspecified) { + allowThread = modification.allowThread(); + break; + } + } + + m_cachedAllowThread = allowThread == AllowThread::Allow + || (allowThread == AllowThread::Auto && autoDetectAllowThread()) ? 1 : 0; + + if (m_cachedAllowThread == 0) + qCDebug(lcShiboken).noquote() << msgDisallowThread(this); } - return false; + return m_cachedAllowThread > 0; } - TypeSystem::Ownership AbstractMetaFunction::ownership(const AbstractMetaClass *cls, TypeSystem::Language language, int key) const { const FunctionModificationList &modifications = this->modifications(cls); diff --git a/sources/shiboken2/ApiExtractor/abstractmetalang.h b/sources/shiboken2/ApiExtractor/abstractmetalang.h index da3b72e1c..f55f61eb4 100644 --- a/sources/shiboken2/ApiExtractor/abstractmetalang.h +++ b/sources/shiboken2/ApiExtractor/abstractmetalang.h @@ -1084,6 +1084,8 @@ public: #endif private: + bool autoDetectAllowThread() const; + QString m_name; QString m_originalName; mutable QString m_cachedMinimalSignature; @@ -1105,6 +1107,7 @@ private: uint m_isNoExcept : 1; uint m_pointerOperator : 1; uint m_isCallOperator : 1; + mutable int m_cachedAllowThread = -1; }; Q_DECLARE_OPERATORS_FOR_FLAGS(AbstractMetaFunction::CompareResult) diff --git a/sources/shiboken2/ApiExtractor/doc/typesystem_manipulating_objects.rst b/sources/shiboken2/ApiExtractor/doc/typesystem_manipulating_objects.rst index ff6ea5317..2d0c40e20 100644 --- a/sources/shiboken2/ApiExtractor/doc/typesystem_manipulating_objects.rst +++ b/sources/shiboken2/ApiExtractor/doc/typesystem_manipulating_objects.rst @@ -74,6 +74,7 @@ modify-function since="..." remove="all | c++" access="public | private | protected" + allow-thread="true | auto | false" rename="..." /> </object-type> @@ -82,6 +83,15 @@ modify-function The ``since`` attribute specify the API version when this function was modified. + The ``allow-thread`` attribute specifies whether a function should be wrapped + into ``Py_BEGIN_ALLOW_THREADS`` and ``Py_END_ALLOW_THREADS``, that is, + temporarily release the GIL (global interpreter lock). Doing so is required + for any thread-related function (wait operations), functions that might call + a virtual function (potentially reimplemented in Python), and recommended for + lengthy I/O operations or similar. It has performance costs, though. + The value ``auto`` means that it will be turned off for functions for which + it is deemed to be safe, for example, simple getters. + The ``remove``, ``access`` and ``rename`` attributes are *optional* attributes for added convenience; they serve the same purpose as the deprecated tags :ref:`remove`, :ref:`access` and :ref:`rename`. diff --git a/sources/shiboken2/ApiExtractor/tests/testmodifyfunction.cpp b/sources/shiboken2/ApiExtractor/tests/testmodifyfunction.cpp index d0a0c9c7a..4743b7167 100644 --- a/sources/shiboken2/ApiExtractor/tests/testmodifyfunction.cpp +++ b/sources/shiboken2/ApiExtractor/tests/testmodifyfunction.cpp @@ -220,6 +220,60 @@ void TestModifyFunction::testWithApiVersion() QVERIFY(func->ownership(func->ownerClass(), TypeSystem::TargetLangCode, 0) != TypeSystem::CppOwnership); } +void TestModifyFunction::testAllowThread() +{ + const char cppCode[] =R"CPP(\ +struct A { + void f1(); + void f2(); + void f3(); + int getter1() const; + int getter2() const; +}; +)CPP"; + + const char xmlCode[] = R"XML( +<typesystem package='Foo'> + <primitive-type name='int'/> + <object-type name='A'> + <modify-function signature='f2()' allow-thread='auto'/> + <modify-function signature='f3()' allow-thread='no'/> + <modify-function signature='getter2()const' allow-thread='yes'/> + </object-type> +</typesystem> +)XML"; + QScopedPointer<AbstractMetaBuilder> builder(TestUtil::parse(cppCode, xmlCode, false, "0.1")); + QVERIFY(!builder.isNull()); + AbstractMetaClassList classes = builder->classes(); + const AbstractMetaClass *classA = AbstractMetaClass::findClass(classes, QLatin1String("A")); + QVERIFY(classA); + + // Nothing specified, true + const AbstractMetaFunction *f1 = classA->findFunction(QLatin1String("f1")); + QVERIFY(f1); + QVERIFY(f1->allowThread()); + + // 'auto' specified, should be true for nontrivial function + const AbstractMetaFunction *f2 = classA->findFunction(QLatin1String("f2")); + QVERIFY(f2); + QVERIFY(f2->allowThread()); + + // 'no' specified, should be false + const AbstractMetaFunction *f3 = classA->findFunction(QLatin1String("f3")); + QVERIFY(f3); + QVERIFY(!f3->allowThread()); + + // Nothing specified, should be false for simple getter + const AbstractMetaFunction *getter1 = classA->findFunction(QLatin1String("getter1")); + QVERIFY(getter1); + QVERIFY(!getter1->allowThread()); + + // Forced to true simple getter + const AbstractMetaFunction *getter2 = classA->findFunction(QLatin1String("getter2")); + QVERIFY(getter2); + QVERIFY(getter2->allowThread()); // Forced to true simple getter +} + void TestModifyFunction::testGlobalFunctionModification() { const char* cppCode ="\ diff --git a/sources/shiboken2/ApiExtractor/tests/testmodifyfunction.h b/sources/shiboken2/ApiExtractor/tests/testmodifyfunction.h index f116b5124..636b3ef34 100644 --- a/sources/shiboken2/ApiExtractor/tests/testmodifyfunction.h +++ b/sources/shiboken2/ApiExtractor/tests/testmodifyfunction.h @@ -37,6 +37,7 @@ class TestModifyFunction : public QObject private slots: void testOwnershipTransfer(); void testWithApiVersion(); + void testAllowThread(); void testRenameArgument_data(); void testRenameArgument(); void invalidateAfterUse(); diff --git a/sources/shiboken2/ApiExtractor/typesystem.cpp b/sources/shiboken2/ApiExtractor/typesystem.cpp index 70563286c..1eab95248 100644 --- a/sources/shiboken2/ApiExtractor/typesystem.cpp +++ b/sources/shiboken2/ApiExtractor/typesystem.cpp @@ -171,6 +171,17 @@ static EnumType functionName(QStringView needle, EnumType defaultValue = default return lb != end && *lb == needleEntry ? lb->value : defaultValue; \ } +ENUM_LOOKUP_BEGIN(TypeSystem::AllowThread, Qt::CaseInsensitive, + allowThreadFromAttribute, TypeSystem::AllowThread::Unspecified) + { + {QStringViewLiteral("yes"), TypeSystem::AllowThread::Allow}, + {QStringViewLiteral("true"), TypeSystem::AllowThread::Allow}, + {QStringViewLiteral("auto"), TypeSystem::AllowThread::Auto}, + {QStringViewLiteral("no"), TypeSystem::AllowThread::Disallow}, + {QStringViewLiteral("false"), TypeSystem::AllowThread::Disallow}, + }; +ENUM_LOOKUP_LINEAR_SEARCH() + ENUM_LOOKUP_BEGIN(TypeSystem::Language, Qt::CaseInsensitive, languageFromAttribute, TypeSystem::NoLanguage) { @@ -363,6 +374,19 @@ static QString msgMissingAttribute(const QString &a) + QLatin1String("' missing."); } +QTextStream &operator<<(QTextStream &str, const QXmlStreamAttribute &attribute) +{ + str << attribute.qualifiedName() << "=\"" << attribute.value() << '"'; + return str; +} + +static QString msgInvalidAttributeValue(const QXmlStreamAttribute &attribute) +{ + QString result; + QTextStream(&result) << "Invalid attribute value:" << attribute; + return result; +} + static QString msgUnusedAttributes(const QStringRef &tag, const QXmlStreamAttributes &attributes) { QString result; @@ -371,7 +395,7 @@ static QString msgUnusedAttributes(const QStringRef &tag, const QXmlStreamAttrib for (int i = 0, size = attributes.size(); i < size; ++i) { if (i) str << ", "; - str << attributes.at(i).qualifiedName() << "=\"" << attributes.at(i).value() << '"'; + str << attributes.at(i); } return result; } @@ -1853,7 +1877,7 @@ bool Handler::parseModifyFunction(const QXmlStreamReader &, QString association; bool deprecated = false; bool isThread = false; - bool allowThread = false; + TypeSystem::AllowThread allowThread = TypeSystem::AllowThread::Unspecified; bool virtualSlot = false; for (int i = attributes->size() - 1; i >= 0; --i) { const QStringRef name = attributes->at(i).qualifiedName(); @@ -1874,8 +1898,12 @@ bool Handler::parseModifyFunction(const QXmlStreamReader &, isThread = convertBoolean(attributes->takeAt(i).value(), threadAttribute(), false); } else if (name == allowThreadAttribute()) { - allowThread = convertBoolean(attributes->takeAt(i).value(), - allowThreadAttribute(), false); + const QXmlStreamAttribute attribute = attributes->takeAt(i); + allowThread = allowThreadFromAttribute(attribute.value()); + if (allowThread == TypeSystem::AllowThread::Unspecified) { + m_error = msgInvalidAttributeValue(attribute); + return false; + } } else if (name == virtualSlotAttribute()) { virtualSlot = convertBoolean(attributes->takeAt(i).value(), virtualSlotAttribute(), false); @@ -1924,7 +1952,8 @@ bool Handler::parseModifyFunction(const QXmlStreamReader &, mod.association = association; mod.setIsThread(isThread); - mod.setAllowThread(allowThread); + if (allowThread != TypeSystem::AllowThread::Unspecified) + mod.setAllowThread(allowThread); if (virtualSlot) mod.modifiers |= Modification::VirtualSlot; diff --git a/sources/shiboken2/ApiExtractor/typesystem.h b/sources/shiboken2/ApiExtractor/typesystem.h index 8eb9dfb5a..df40508a2 100644 --- a/sources/shiboken2/ApiExtractor/typesystem.h +++ b/sources/shiboken2/ApiExtractor/typesystem.h @@ -326,6 +326,8 @@ struct Modification struct FunctionModification: public Modification { + using AllowThread = TypeSystem::AllowThread; + bool isCodeInjection() const { return modifiers & CodeInjection; @@ -338,14 +340,9 @@ struct FunctionModification: public Modification { return m_thread; } - bool allowThread() const - { - return m_allowThread; - } - void setAllowThread(bool allow) - { - m_allowThread = allow; - } + + AllowThread allowThread() const { return m_allowThread; } + void setAllowThread(AllowThread allow) { m_allowThread = allow; } bool matches(const QString &functionSignature) const { @@ -372,7 +369,7 @@ private: QString m_originalSignature; QRegularExpression m_signaturePattern; bool m_thread = false; - bool m_allowThread = false; + AllowThread m_allowThread = AllowThread::Unspecified; }; struct FieldModification: public Modification diff --git a/sources/shiboken2/ApiExtractor/typesystem_enums.h b/sources/shiboken2/ApiExtractor/typesystem_enums.h index 62ae8feb2..f0ebc197d 100644 --- a/sources/shiboken2/ApiExtractor/typesystem_enums.h +++ b/sources/shiboken2/ApiExtractor/typesystem_enums.h @@ -55,6 +55,13 @@ enum Language { TargetLangAndNativeCode = TargetLangCode | NativeCode }; +enum class AllowThread { + Allow, + Disallow, + Auto, + Unspecified +}; + enum Ownership { InvalidOwnership, DefaultOwnership, diff --git a/sources/shiboken2/generator/shiboken2/cppgenerator.cpp b/sources/shiboken2/generator/shiboken2/cppgenerator.cpp index 8bddef700..f230782d1 100644 --- a/sources/shiboken2/generator/shiboken2/cppgenerator.cpp +++ b/sources/shiboken2/generator/shiboken2/cppgenerator.cpp @@ -186,18 +186,19 @@ QVector<AbstractMetaFunctionList> CppGenerator::filterGroupedOperatorFunctions(c return result; } -bool CppGenerator::hasBoolCast(const AbstractMetaClass* metaClass) const +const AbstractMetaFunction *CppGenerator::boolCast(const AbstractMetaClass* metaClass) const { if (!useIsNullAsNbNonZero()) - return false; + return nullptr; // TODO: This could be configurable someday const AbstractMetaFunction* func = metaClass->findFunction(QLatin1String("isNull")); if (!func || !func->type() || !func->type()->typeEntry()->isPrimitive() || !func->isPublic()) - return false; + return nullptr; const PrimitiveTypeEntry* pte = static_cast<const PrimitiveTypeEntry*>(func->type()->typeEntry()); while (pte->referencedTypeEntry()) pte = pte->referencedTypeEntry(); - return func && func->isConstant() && pte->name() == QLatin1String("bool") && func->arguments().isEmpty(); + return func && func->isConstant() && pte->name() == QLatin1String("bool") + && func->arguments().isEmpty() ? func : nullptr; } typedef QMap<QString, AbstractMetaFunctionList> FunctionGroupMap; @@ -490,16 +491,20 @@ void CppGenerator::generateClass(QTextStream &s, GeneratorContext &classContext) } } - if (hasBoolCast(metaClass)) { + if (const AbstractMetaFunction *f = boolCast(metaClass)) { ErrorCode errorCode(-1); s << "static int " << cpythonBaseName(metaClass) << "___nb_bool(PyObject* " PYTHON_SELF_VAR ")" << endl; s << '{' << endl; writeCppSelfDefinition(s, classContext); - s << INDENT << "int result;" << endl; - s << INDENT << BEGIN_ALLOW_THREADS << endl; - s << INDENT << "result = !" CPP_SELF_VAR "->isNull();" << endl; - s << INDENT << END_ALLOW_THREADS << endl; - s << INDENT << "return result;" << endl; + if (f->allowThread()) { + s << INDENT << "int result;" << endl; + s << INDENT << BEGIN_ALLOW_THREADS << endl; + s << INDENT << "result = !" CPP_SELF_VAR "->isNull();" << endl; + s << INDENT << END_ALLOW_THREADS << endl; + s << INDENT << "return result;" << endl; + } else { + s << INDENT << "return !" << CPP_SELF_VAR "->isNull();" << endl; + } s << '}' << endl << endl; } @@ -3270,7 +3275,10 @@ void CppGenerator::writeMethodCall(QTextStream &s, const AbstractMetaFunction *f } if (!injectedCodeCallsCppFunction(func)) { - s << INDENT << BEGIN_ALLOW_THREADS << endl << INDENT; + const bool allowThread = func->allowThread(); + if (allowThread) + s << INDENT << BEGIN_ALLOW_THREADS << endl; + s << INDENT; if (isCtor) { s << (useVAddr.isEmpty() ? QString::fromLatin1("cptr = %1;").arg(methodCall) : useVAddr) << endl; @@ -3303,7 +3311,8 @@ void CppGenerator::writeMethodCall(QTextStream &s, const AbstractMetaFunction *f } else { s << methodCall << ';' << endl; } - s << INDENT << END_ALLOW_THREADS << endl; + if (allowThread) + s << INDENT << END_ALLOW_THREADS << endl; if (!func->conversionRule(TypeSystem::TargetLangCode, 0).isEmpty()) { writeConversionRule(s, func, TypeSystem::TargetLangCode, QLatin1String(PYTHON_RETURN_VAR)); @@ -4010,7 +4019,8 @@ void CppGenerator::writeTypeAsNumberDefinition(QTextStream& s, const AbstractMet QString baseName = cpythonBaseName(metaClass); - nb[QLatin1String("bool")] = hasBoolCast(metaClass) ? baseName + QLatin1String("___nb_bool") : QString(); + if (hasBoolCast(metaClass)) + nb.insert(QLatin1String("bool"), baseName + QLatin1String("___nb_bool")); for (QHash<QString, QString>::const_iterator it = m_nbFuncs.cbegin(), end = m_nbFuncs.cend(); it != end; ++it) { const QString &nbName = it.key(); diff --git a/sources/shiboken2/generator/shiboken2/cppgenerator.h b/sources/shiboken2/generator/shiboken2/cppgenerator.h index 49a1e1835..d810665e9 100644 --- a/sources/shiboken2/generator/shiboken2/cppgenerator.h +++ b/sources/shiboken2/generator/shiboken2/cppgenerator.h @@ -328,7 +328,9 @@ private: QString writeReprFunction(QTextStream &s, GeneratorContext &context); - bool hasBoolCast(const AbstractMetaClass* metaClass) const; + const AbstractMetaFunction *boolCast(const AbstractMetaClass* metaClass) const; + bool hasBoolCast(const AbstractMetaClass* metaClass) const + { return boolCast(metaClass) != nullptr; } // Number protocol structure members names. static QHash<QString, QString> m_nbFuncs; |