From 7b4e4e7fc1583ac91df576a1969bf2ee548467b8 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 13 Feb 2024 22:31:52 +0000 Subject: [PATCH 1/4] gh-115432: Add critical section variant that handles a NULL object This adds `Py_BEGIN_CRITICAL_SECTION_OPT` and `Py_END_CRITICAL_SECTION_OPT`, which accept a possibly NULL object as an argument. If the argument is NULL, then nothing is locked or unlocked. Otherwise, they behave like `Py_BEGIN/END_CRITICAL_SECTION`. --- Include/internal/pycore_critical_section.h | 29 +++++++++++++++++++ .../test_critical_sections.c | 10 +++++++ 2 files changed, 39 insertions(+) diff --git a/Include/internal/pycore_critical_section.h b/Include/internal/pycore_critical_section.h index 38ed8cd69804ba..a68856f180e0fb 100644 --- a/Include/internal/pycore_critical_section.h +++ b/Include/internal/pycore_critical_section.h @@ -96,6 +96,15 @@ extern "C" { _PyCriticalSection_End(&_cs); \ } +# define Py_BEGIN_CRITICAL_SECTION_OPT(op) \ + { \ + _PyCriticalSection _cs_opt = {0}; \ + _PyCriticalSection_BeginOpt(&_cs_opt, _PyObject_CAST(op)) + +# define Py_END_CRITICAL_SECTION_OPT() \ + _PyCriticalSection_EndOpt(&_cs_opt); \ + } + # define Py_BEGIN_CRITICAL_SECTION2(a, b) \ { \ _PyCriticalSection2 _cs2; \ @@ -131,6 +140,8 @@ extern "C" { // The critical section APIs are no-ops with the GIL. # define Py_BEGIN_CRITICAL_SECTION(op) # define Py_END_CRITICAL_SECTION() +# define Py_BEGIN_CRITICAL_SECTION_OPT(op) +# define Py_END_CRITICAL_SECTION_OPT() # define Py_BEGIN_CRITICAL_SECTION2(a, b) # define Py_END_CRITICAL_SECTION2() # define _Py_CRITICAL_SECTION_ASSERT_MUTEX_LOCKED(mutex) @@ -187,6 +198,16 @@ _PyCriticalSection_Begin(_PyCriticalSection *c, PyMutex *m) } } +static inline void +_PyCriticalSection_BeginOpt(_PyCriticalSection *c, PyObject *op) +{ +#ifdef Py_GIL_DISABLED + if (op != NULL) { + _PyCriticalSection_Begin(c, &_PyObject_CAST(op)->ob_mutex); + } +#endif +} + // Removes the top-most critical section from the thread's stack of critical // sections. If the new top-most critical section is inactive, then it is // resumed. @@ -209,6 +230,14 @@ _PyCriticalSection_End(_PyCriticalSection *c) _PyCriticalSection_Pop(c); } +static inline void +_PyCriticalSection_EndOpt(_PyCriticalSection *c) +{ + if (c->mutex) { + _PyCriticalSection_End(c); + } +} + static inline void _PyCriticalSection2_Begin(_PyCriticalSection2 *c, PyMutex *m1, PyMutex *m2) { diff --git a/Modules/_testinternalcapi/test_critical_sections.c b/Modules/_testinternalcapi/test_critical_sections.c index 1f7e311558b27c..2bc6c7b8fc4c4a 100644 --- a/Modules/_testinternalcapi/test_critical_sections.c +++ b/Modules/_testinternalcapi/test_critical_sections.c @@ -49,6 +49,16 @@ test_critical_sections(PyObject *self, PyObject *Py_UNUSED(args)) Py_END_CRITICAL_SECTION2(); assert_nogil(!PyMutex_IsLocked(&d2->ob_mutex)); + // Optional variant behaves the same if the object is non-NULL + Py_BEGIN_CRITICAL_SECTION_OPT(d1); + assert_nogil(PyMutex_IsLocked(&d1->ob_mutex)); + Py_END_CRITICAL_SECTION_OPT(); + + // No-op + PyObject *null_object = NULL; + Py_BEGIN_CRITICAL_SECTION_OPT(null_object); + Py_END_CRITICAL_SECTION_OPT(); + Py_DECREF(d2); Py_DECREF(d1); Py_RETURN_NONE; From 54491d7f8a52333bbd0ca684d44e8bf289e36fc0 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 13 Feb 2024 22:39:08 +0000 Subject: [PATCH 2/4] Avoid warning in default build --- Modules/_testinternalcapi/test_critical_sections.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Modules/_testinternalcapi/test_critical_sections.c b/Modules/_testinternalcapi/test_critical_sections.c index 2bc6c7b8fc4c4a..6260a414cadde3 100644 --- a/Modules/_testinternalcapi/test_critical_sections.c +++ b/Modules/_testinternalcapi/test_critical_sections.c @@ -55,8 +55,7 @@ test_critical_sections(PyObject *self, PyObject *Py_UNUSED(args)) Py_END_CRITICAL_SECTION_OPT(); // No-op - PyObject *null_object = NULL; - Py_BEGIN_CRITICAL_SECTION_OPT(null_object); + Py_BEGIN_CRITICAL_SECTION_OPT(NULL); Py_END_CRITICAL_SECTION_OPT(); Py_DECREF(d2); From f6ba3b931723f37247fdb34cf67efae0bf26007c Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 14 Feb 2024 00:00:53 +0000 Subject: [PATCH 3/4] Use 'X' prefix for consistency with Py_XDECREF, Py_XNewRef, etc. --- Include/internal/pycore_critical_section.h | 12 ++++++------ Modules/_testinternalcapi/test_critical_sections.c | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Include/internal/pycore_critical_section.h b/Include/internal/pycore_critical_section.h index a68856f180e0fb..7f345b84931790 100644 --- a/Include/internal/pycore_critical_section.h +++ b/Include/internal/pycore_critical_section.h @@ -96,13 +96,13 @@ extern "C" { _PyCriticalSection_End(&_cs); \ } -# define Py_BEGIN_CRITICAL_SECTION_OPT(op) \ +# define Py_XBEGIN_CRITICAL_SECTION(op) \ { \ _PyCriticalSection _cs_opt = {0}; \ - _PyCriticalSection_BeginOpt(&_cs_opt, _PyObject_CAST(op)) + _PyCriticalSection_XBegin(&_cs_opt, _PyObject_CAST(op)) -# define Py_END_CRITICAL_SECTION_OPT() \ - _PyCriticalSection_EndOpt(&_cs_opt); \ +# define Py_XEND_CRITICAL_SECTION() \ + _PyCriticalSection_XEnd(&_cs_opt); \ } # define Py_BEGIN_CRITICAL_SECTION2(a, b) \ @@ -199,7 +199,7 @@ _PyCriticalSection_Begin(_PyCriticalSection *c, PyMutex *m) } static inline void -_PyCriticalSection_BeginOpt(_PyCriticalSection *c, PyObject *op) +_PyCriticalSection_XBegin(_PyCriticalSection *c, PyObject *op) { #ifdef Py_GIL_DISABLED if (op != NULL) { @@ -231,7 +231,7 @@ _PyCriticalSection_End(_PyCriticalSection *c) } static inline void -_PyCriticalSection_EndOpt(_PyCriticalSection *c) +_PyCriticalSection_XEnd(_PyCriticalSection *c) { if (c->mutex) { _PyCriticalSection_End(c); diff --git a/Modules/_testinternalcapi/test_critical_sections.c b/Modules/_testinternalcapi/test_critical_sections.c index 6260a414cadde3..94da0468fcf149 100644 --- a/Modules/_testinternalcapi/test_critical_sections.c +++ b/Modules/_testinternalcapi/test_critical_sections.c @@ -50,13 +50,13 @@ test_critical_sections(PyObject *self, PyObject *Py_UNUSED(args)) assert_nogil(!PyMutex_IsLocked(&d2->ob_mutex)); // Optional variant behaves the same if the object is non-NULL - Py_BEGIN_CRITICAL_SECTION_OPT(d1); + Py_XBEGIN_CRITICAL_SECTION(d1); assert_nogil(PyMutex_IsLocked(&d1->ob_mutex)); - Py_END_CRITICAL_SECTION_OPT(); + Py_XEND_CRITICAL_SECTION(); // No-op - Py_BEGIN_CRITICAL_SECTION_OPT(NULL); - Py_END_CRITICAL_SECTION_OPT(); + Py_XBEGIN_CRITICAL_SECTION(NULL); + Py_XEND_CRITICAL_SECTION(); Py_DECREF(d2); Py_DECREF(d1); From 59a8b740403d3522bb2f9118a3acac2f9c5d4c1f Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 14 Feb 2024 00:10:39 +0000 Subject: [PATCH 4/4] Fix definition for default build --- Include/internal/pycore_critical_section.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Include/internal/pycore_critical_section.h b/Include/internal/pycore_critical_section.h index 7f345b84931790..30820a24f5bb64 100644 --- a/Include/internal/pycore_critical_section.h +++ b/Include/internal/pycore_critical_section.h @@ -140,8 +140,8 @@ extern "C" { // The critical section APIs are no-ops with the GIL. # define Py_BEGIN_CRITICAL_SECTION(op) # define Py_END_CRITICAL_SECTION() -# define Py_BEGIN_CRITICAL_SECTION_OPT(op) -# define Py_END_CRITICAL_SECTION_OPT() +# define Py_XBEGIN_CRITICAL_SECTION(op) +# define Py_XEND_CRITICAL_SECTION() # define Py_BEGIN_CRITICAL_SECTION2(a, b) # define Py_END_CRITICAL_SECTION2() # define _Py_CRITICAL_SECTION_ASSERT_MUTEX_LOCKED(mutex)