From 3653eb74d58242cb027f11a858c23b20a2cd2fc9 Mon Sep 17 00:00:00 2001 From: Ronald Oussoren Date: Fri, 20 Dec 2024 23:13:36 +0100 Subject: [PATCH] Issue #608: Enable free-threading This changeset fixes a number of small issues in the free-threading support, and enables GIL-less operation by default. This is the first point where tests in pyobjc-core pass with the free-threaded build. Next up: actually testing threaded usage. --- .../Modules/objc/OC_PythonDictionary.m | 21 ++++++--- pyobjc-core/Modules/objc/class-builder.m | 6 ++- pyobjc-core/Modules/objc/file_wrapper.m | 44 ++++++++++++------- pyobjc-core/Modules/objc/module.m | 18 ++++---- pyobjc-core/Modules/objc/objc-class.m | 4 +- pyobjc-core/Modules/objc/proxy-registry.m | 8 ++-- pyobjc-core/Modules/objc/pyobjc-compat.h | 4 +- pyobjc-core/Modules/objc/selector.m | 4 +- pyobjc-core/Modules/objc/super-call.m | 19 +++----- 9 files changed, 73 insertions(+), 55 deletions(-) diff --git a/pyobjc-core/Modules/objc/OC_PythonDictionary.m b/pyobjc-core/Modules/objc/OC_PythonDictionary.m index 1012cf354..6cfb1c077 100644 --- a/pyobjc-core/Modules/objc/OC_PythonDictionary.m +++ b/pyobjc-core/Modules/objc/OC_PythonDictionary.m @@ -52,6 +52,7 @@ - (void)dealloc - (id _Nullable)nextObject { id key = nil; + int rv; PyObject* pykey = NULL; PyObjC_BEGIN_WITH_GIL @@ -59,24 +60,32 @@ - (id _Nullable)nextObject Py_BEGIN_CRITICAL_SECTION(dct); - if (unlikely(!PyDict_Next(dct, &pos, &pykey, NULL))) { + rv = PyDict_Next(dct, &pos, &pykey, NULL); + if (rv) { + Py_XINCREF(pykey); + valid = YES; + } else { + valid = NO; + } + + Py_END_CRITICAL_SECTION(); + + if (!rv) { key = nil; } else if (pykey == Py_None) { key = [NSNull null]; + Py_DECREF(pykey); } else { if (depythonify_c_value(@encode(id), pykey, &key) == -1) { Py_DECREF(dct); - Py_EXIT_CRITICAL_SECTION(); + Py_DECREF(pykey); PyObjC_GIL_FORWARD_EXC(); } + Py_DECREF(pykey); } - valid = (key != nil) ? YES : NO; - - Py_END_CRITICAL_SECTION(); - Py_DECREF(dct); PyObjC_END_WITH_GIL diff --git a/pyobjc-core/Modules/objc/class-builder.m b/pyobjc-core/Modules/objc/class-builder.m index a518b8a53..012835a27 100644 --- a/pyobjc-core/Modules/objc/class-builder.m +++ b/pyobjc-core/Modules/objc/class-builder.m @@ -119,8 +119,8 @@ static Class _Nullable build_intermediate_class(Class base_class, char* name) #ifdef Py_GIL_DISABLED PyMutex_Lock(&intermediate_mutex); - #endif + /* * The naming style for the intermediate class ensures that the class * we've found has the correct parent, so no need to check for this in @@ -128,7 +128,11 @@ static Class _Nullable build_intermediate_class(Class base_class, char* name) */ intermediate_class = objc_lookUpClass(name); if (intermediate_class != Nil) { +#ifdef Py_GIL_DISABLED + PyMutex_Unlock(&intermediate_mutex); +#endif PyObjC_Assert(class_getSuperclass(intermediate_class) == base_class, Nil); + return intermediate_class; } diff --git a/pyobjc-core/Modules/objc/file_wrapper.m b/pyobjc-core/Modules/objc/file_wrapper.m index 410e067a6..5f910fbd7 100644 --- a/pyobjc-core/Modules/objc/file_wrapper.m +++ b/pyobjc-core/Modules/objc/file_wrapper.m @@ -59,21 +59,26 @@ static PyObject* _Nullable file_close(PyObject* _self) { struct file_object* self = (struct file_object*)_self; + FILE* fp; Py_BEGIN_CRITICAL_SECTION(_self); + fp = self->fp; + self->fp = NULL; + Py_END_CRITICAL_SECTION(); - if (self->fp == NULL) { + if (fp == NULL) { PyErr_SetString(PyExc_ValueError, "Closing closed file"); return NULL; } - if (fclose(self->fp) < 0) { + if (fclose(fp) < 0) { + /* This is very unlikely, restore previous value */ + Py_BEGIN_CRITICAL_SECTION(_self); + self->fp = fp; + Py_END_CRITICAL_SECTION(); return PyErr_SetFromErrno(PyExc_OSError); } - self->fp = NULL; - - Py_END_CRITICAL_SECTION(); Py_RETURN_NONE; } @@ -111,18 +116,20 @@ if (self->fp == NULL) { PyErr_SetString(PyExc_ValueError, "Using closed file"); - Py_EXIT_CRITICAL_SECTION(); - return NULL; + result = -1; } else { - result = ferror(self->fp); + result = ferror(self->fp) != 0; } Py_END_CRITICAL_SECTION(); - if (result) { - Py_RETURN_TRUE; - } else { + switch (result) { + case -1: + return NULL; + case 0: Py_RETURN_FALSE; + default: + Py_RETURN_TRUE; } } @@ -135,17 +142,19 @@ if (self->fp == NULL) { PyErr_SetString(PyExc_ValueError, "Using closed file"); - Py_EXIT_CRITICAL_SECTION(); - return NULL; + result = -1; } else { - result = feof(self->fp); + result = feof(self->fp) != 0; } Py_END_CRITICAL_SECTION(); - if (result) { - Py_RETURN_TRUE; - } else { + switch (result) { + case -1: + return NULL; + case 0: Py_RETURN_FALSE; + default: + Py_RETURN_TRUE; } } @@ -257,6 +266,7 @@ Py_BEGIN_CRITICAL_SECTION(_self); if (self->fp == NULL) { PyErr_SetString(PyExc_ValueError, "Using closed file"); + Py_EXIT_CRITICAL_SECTION(); return NULL; } else { result = fgets(buffer, 2048, self->fp); diff --git a/pyobjc-core/Modules/objc/module.m b/pyobjc-core/Modules/objc/module.m index 3dff45347..54f31e595 100644 --- a/pyobjc-core/Modules/objc/module.m +++ b/pyobjc-core/Modules/objc/module.m @@ -370,12 +370,12 @@ + (void)targetForBecomingMultiThreaded:(id)sender * there's not much we can do if releasing raises. */ #ifdef Py_GIL_DISABLED - PyMutex_Lock(&global_release_pool_lock); + PyMutex_Lock(&global_release_pool_lock); #endif pool = global_release_pool; global_release_pool = nil; #ifdef Py_GIL_DISABLED - PyMutex_Unlock(&global_release_pool_lock); + PyMutex_Unlock(&global_release_pool_lock); #endif [pool release]; @@ -399,17 +399,20 @@ + (void)targetForBecomingMultiThreaded:(id)sender __attribute__((__unused__))) { NSAutoreleasePool* pool; -#ifdef Py_GIL_DISABLED - PyMutex_Lock(&global_release_pool_lock); -#endif Py_BEGIN_ALLOW_THREADS @try { /* Unconditionally set global_release_pool to nil * before calling release. There's not much we can * do if draining fails with an exception. */ +#ifdef Py_GIL_DISABLED + PyMutex_Lock(&global_release_pool_lock); +#endif pool = global_release_pool; global_release_pool = nil; +#ifdef Py_GIL_DISABLED + PyMutex_Unlock(&global_release_pool_lock); +#endif [pool release]; } @catch (NSObject* localException) { @@ -421,9 +424,6 @@ + (void)targetForBecomingMultiThreaded:(id)sender */ [OC_NSAutoreleasePoolCollector newAutoreleasePool]; Py_END_ALLOW_THREADS -#ifdef Py_GIL_DISABLED - PyMutex_Unlock(&global_release_pool_lock); -#endif if (PyErr_Occurred()) return NULL; @@ -2388,7 +2388,7 @@ static int mod_exec_module(PyObject* m) #if PY_VERSION_HEX >= 0x030d0000 { .slot = Py_mod_gil, - .value = Py_MOD_GIL_USED, + .value = Py_MOD_GIL_NOT_USED, }, #endif { /* Sentinel */ diff --git a/pyobjc-core/Modules/objc/objc-class.m b/pyobjc-core/Modules/objc/objc-class.m index e092ceae6..833584389 100644 --- a/pyobjc-core/Modules/objc/objc-class.m +++ b/pyobjc-core/Modules/objc/objc-class.m @@ -1315,7 +1315,7 @@ static Class _Nullable objc_metaclass_locate(PyObject* meta_class) } else { should_call = 0; } - Py_END_CRITICAL_SECTION(info); + Py_END_CRITICAL_SECTION(); if (should_call) { #endif @@ -3219,9 +3219,7 @@ Class _Nullable PyObjCClass_GetClass(PyObject* cls) if (PyObjCClass_Check(cls)) { Class result = Nil; - Py_BEGIN_CRITICAL_SECTION(cls); result = ((PyObjCClassObject*)cls)->class; - Py_END_CRITICAL_SECTION(); if (result == Nil) { /* XXX: Audit callers, the field can be Nil */ diff --git a/pyobjc-core/Modules/objc/proxy-registry.m b/pyobjc-core/Modules/objc/proxy-registry.m index 72383c64f..f1fa1c03b 100644 --- a/pyobjc-core/Modules/objc/proxy-registry.m +++ b/pyobjc-core/Modules/objc/proxy-registry.m @@ -242,6 +242,8 @@ static void weak_value_release(NSMapTable* table __attribute__((__unused__)), vo id _Nullable NS_RETURNS_RETAINED PyObjC_FindObjCProxy(PyObject* original) { + id result; + if (original == Py_None) // LCOV_BR_EXCL_LINE PyObjCErr_InternalError(); // LCOV_EXCL_LINE @@ -251,10 +253,10 @@ id _Nullable NS_RETURNS_RETAINED PyObjC_FindObjCProxy(PyObject* original) struct weak_value* record = NSMapGet(objc_proxies, original); if (record == NULL) { - return nil; + result = nil ; + } else { + result = objc_loadWeakRetained(&record->value); } - - id result = objc_loadWeakRetained(&record->value); #ifdef Py_GIL_DISABLED PyMutex_Unlock(&proxy_mutex); #endif diff --git a/pyobjc-core/Modules/objc/pyobjc-compat.h b/pyobjc-core/Modules/objc/pyobjc-compat.h index c5c47b5ed..4b8a2165f 100644 --- a/pyobjc-core/Modules/objc/pyobjc-compat.h +++ b/pyobjc-core/Modules/objc/pyobjc-compat.h @@ -235,11 +235,11 @@ _PyObjCTuple_GetItem(PyObject* tuple, Py_ssize_t idx) } while (0) #if PY_VERSION_HEX < 0x030d0000 -#define Py_BEGIN_CRITICAL_SECTION(value) { +#define Py_BEGIN_CRITICAL_SECTION(value) { (void)(value); #define Py_END_CRITICAL_SECTION() } #define Py_EXIT_CRITICAL_SECTION() ((void)0) -#define Py_BEGIN_CRITICAL_SECTION2(value1, value2) { +#define Py_BEGIN_CRITICAL_SECTION2(value1, value2) { (void)((value1),(value2)); #define Py_END_CRITICAL_SECTION2() } #define Py_EXIT_CRITICAL_SECTION2() ((void)0) diff --git a/pyobjc-core/Modules/objc/selector.m b/pyobjc-core/Modules/objc/selector.m index d100739fc..159713b58 100644 --- a/pyobjc-core/Modules/objc/selector.m +++ b/pyobjc-core/Modules/objc/selector.m @@ -253,11 +253,11 @@ } if (PyObject_IsTrue(newVal)) { - Py_BEGIN_CRITICAL_SECTION(self); + Py_BEGIN_CRITICAL_SECTION(_self); ((PyObjCSelector*)_self)->sel_flags |= PyObjCSelector_kHIDDEN; Py_END_CRITICAL_SECTION(); } else { - Py_BEGIN_CRITICAL_SECTION(self); + Py_BEGIN_CRITICAL_SECTION(_self); ((PyObjCSelector*)_self)->sel_flags &= ~PyObjCSelector_kHIDDEN; Py_END_CRITICAL_SECTION(); } diff --git a/pyobjc-core/Modules/objc/super-call.m b/pyobjc-core/Modules/objc/super-call.m index d19ffc8dc..2bb5c2bb7 100644 --- a/pyobjc-core/Modules/objc/super-call.m +++ b/pyobjc-core/Modules/objc/super-call.m @@ -195,12 +195,13 @@ Py_DECREF(entry); PyObjC_MappingCount += 1; + +exit: #ifdef Py_GIL_DISABLED PyMutex_Unlock(®istry_mutex); #endif - -exit: return retval; + } int @@ -391,16 +392,16 @@ struct registry* rv = PyCapsule_GetPointer(result, "objc.__memblock__"); Py_DECREF(result); #ifdef Py_GIL_DISABLED - PyMutex_Unlock(®istry_mutex); + PyMutex_Unlock(®istry_mutex); #endif return rv; error: - Py_CLEAR(special_class); + Py_CLEAR(special_class); Py_CLEAR(py_selname); Py_CLEAR(search_class); #ifdef Py_GIL_DISABLED - PyMutex_Unlock(®istry_mutex); + PyMutex_Unlock(®istry_mutex); #endif return NULL; } @@ -420,7 +421,7 @@ #endif PyObject* key = PyBytes_FromStringAndSize(NULL, strlen(signature) + 10); if (key == NULL) { // LCOV_BR_EXCL_LINE - return NULL; // LCOV_EXCL_LINE + goto exit; } res = PyObjCRT_SimplifySignature(signature, PyBytes_AS_STRING(key), @@ -456,9 +457,6 @@ PyObjC_CallFunc _Nullable PyObjC_FindCallFunc(Class class, SEL sel, const char* PyObjC_Assert(special_registry != NULL, NULL); -#ifdef Py_GIL_DISABLED - PyMutex_Lock(®istry_mutex); -#endif special = search_special(class, sel); if (special) { result = special->call_to_objc; @@ -475,9 +473,6 @@ PyObjC_CallFunc _Nullable PyObjC_FindCallFunc(Class class, SEL sel, const char* } } -#ifdef Py_GIL_DISABLED - PyMutex_Unlock(®istry_mutex); -#endif return result; }