aboutsummaryrefslogtreecommitdiffstats
path: root/sources/shiboken2/ApiExtractor
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/ApiExtractor
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/ApiExtractor')
-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
8 files changed, 159 insertions, 20 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,