aboutsummaryrefslogtreecommitdiffstats
path: root/sources/shiboken2
diff options
context:
space:
mode:
authorFriedemann Kleint <Friedemann.Kleint@qt.io>2018-07-25 11:42:39 +0200
committerFriedemann Kleint <Friedemann.Kleint@qt.io>2018-07-25 12:16:31 +0000
commitbe202bd1baf600c7b422c96cc6b47a327e7a9d23 (patch)
tree187183f6636901b36c1e0c87708040c653b7d0fd /sources/shiboken2
parentfdae2fce386cb4d72c4528b7e868c61860c43069 (diff)
shiboken: Fix the allow-thread attribute to actually have an effect
Previously, calls to BEGIN_ALLOW_THREADS/END_ALLOW_THREADS were always generated since the value of XML attribute was not used. Fix it to actually use the value. Since having it default to "no" caused a number of deadlocks (related to thread functions or functions calling a virtual function (potentially reimplemented in Python), introduce "auto" as default value. "auto" turns off BEGIN_ALLOW_THREADS/END_ALLOW_THREADS for simple getter functions. Task-number: PYSIDE-743 Change-Id: I4833afef14f2552c75b3424417c2702ce25cb379 Reviewed-by: Christian Tismer <tismer@stackless.com> Reviewed-by: Cristian Maureira-Fredes <cristian.maureira-fredes@qt.io>
Diffstat (limited to 'sources/shiboken2')
-rw-r--r--sources/shiboken2/ApiExtractor/abstractmetalang.cpp50
-rw-r--r--sources/shiboken2/ApiExtractor/abstractmetalang.h3
-rw-r--r--sources/shiboken2/ApiExtractor/doc/typesystem_manipulating_objects.rst10
-rw-r--r--sources/shiboken2/ApiExtractor/tests/testmodifyfunction.cpp54
-rw-r--r--sources/shiboken2/ApiExtractor/tests/testmodifyfunction.h1
-rw-r--r--sources/shiboken2/ApiExtractor/typesystem.cpp39
-rw-r--r--sources/shiboken2/ApiExtractor/typesystem.h15
-rw-r--r--sources/shiboken2/ApiExtractor/typesystem_enums.h7
-rw-r--r--sources/shiboken2/generator/shiboken2/cppgenerator.cpp36
-rw-r--r--sources/shiboken2/generator/shiboken2/cppgenerator.h4
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;