[GR-75391] [GR-75392] Fix XDecrefPointerNode and PyGILState_Ensure/Release.

PullRequest: graalpython/4466

Author: fangerer

graalpython/com.oracle.graal.python.cext/src/capi.c

@@ -100,7 +100,7 @@ typedef struct {
 // defined in 'unicodeobject.c'
 void unicode_dealloc(PyObject *unicode);
 
-NO_INLINE void
+Py_LOCAL_SYMBOL NO_INLINE void
 graalpy_dealloc_stack_grow(PyThreadState *tstate)
 {
     size_t old_capacity;
@@ -119,7 +119,7 @@ graalpy_dealloc_stack_grow(PyThreadState *tstate)
     }
 }
 
-void
+Py_LOCAL_SYMBOL void
 graalpy_dealloc_stack_push(PyThreadState *tstate, PyObject *op)
 {
     assert(tstate != NULL);
@@ -129,7 +129,7 @@ graalpy_dealloc_stack_push(PyThreadState *tstate, PyObject *op)
     tstate->graalpy_deallocating.items[tstate->graalpy_deallocating.len++] = op;
 }
 
-void
+Py_LOCAL_SYMBOL void
 graalpy_dealloc_stack_pop(PyThreadState *tstate, PyObject *op)
 {
     assert(tstate != NULL);
@@ -574,26 +574,12 @@ void initialize_hashes();
 void _PyFloat_InitState(PyInterpreterState* state);
 
 /*
- * This is used to allow Truffle to enter/leave the context on native threads
- * that were not created by NFI/Truffle/Java and thus not previously attached
- * to the context. See e.g. PyGILState_Ensure. This is used by some C
- * extensions to allow calling Python APIs from natively created threads. This
- * poses a problem if multiple contexts use the same library, since we cannot
- * know which context should be entered. CPython has the same problem (see
- * https://docs.python.org/3/c-api/init.html#bugs-and-caveats), in particular
- * the following quote:
- *
- *   Furthermore, extensions (such as ctypes) using these APIs to allow calling
- *   of Python code from non-Python created threads will probably be broken
- *   when using sub-interpreters.
- *
- * If we try to use the same libpython for multiple contexts, we can only
- * behave in a similar (likely broken) way as CPython: natively created threads
- * that use the PyGIL_* APIs to allow calling into Python will attach to the
- * first interpreter that initialized the C API (and thus set the
- * TRUFFLE_CONTEXT pointer) only.
+ * These functions allow Truffle to enter/leave the context on native threads
+ * that were not created by Truffle/Java and thus do not have a previously
+ * entered polyglot context. See e.g. PyGILState_Ensure.
  */
-Py_LOCAL_SYMBOL TruffleContext* TRUFFLE_CONTEXT;
+Py_LOCAL_SYMBOL graalpy_attach_native_thread_func graalpy_attach_native_thread = NULL;
+Py_LOCAL_SYMBOL graalpy_detach_native_thread_func graalpy_detach_native_thread = NULL;
 
 /*
  * This is only set during VM shutdown, so on the native side can only be used
@@ -603,10 +589,14 @@ Py_LOCAL_SYMBOL TruffleContext* TRUFFLE_CONTEXT;
  */
 Py_LOCAL_SYMBOL int8_t *_graalpy_finalizing = NULL;
 
-PyAPI_FUNC(PyThreadState **) initialize_graal_capi(void **builtin_closures, GCState *gc, PyThreadState *tstate) {
+PyAPI_FUNC(PyThreadState **) initialize_graal_capi(void **builtin_closures, GCState *gc,
+                PyThreadState *tstate, graalpy_attach_native_thread_func attach_native_thread,
+                graalpy_detach_native_thread_func detach_native_thread) {
     clock_t t = clock();
 
     _PyGC_InitState(gc);
+    graalpy_attach_native_thread = attach_native_thread;
+    graalpy_detach_native_thread = detach_native_thread;
 
     /*
      * Initializing all these global fields with pointers to different contexts

graalpython/com.oracle.graal.python.cext/src/capi.h

@@ -65,6 +65,16 @@
 #error "don't know how to declare thread local variable"
 #endif
 
+typedef int (*graalpy_attach_native_thread_func)(void);
+typedef void (*graalpy_detach_native_thread_func)(void);
+
+#define GRAALPY_ATTACH_NATIVE_FAILED (-1)
+#define GRAALPY_ATTACH_NATIVE_OWNED 1
+#define GRAALPY_ATTACH_NATIVE_FOREIGN 2
+
+extern graalpy_attach_native_thread_func graalpy_attach_native_thread;
+extern graalpy_detach_native_thread_func graalpy_detach_native_thread;
+
 #ifdef MS_WINDOWS
 // define the below, otherwise windows' sdk defines complex to _complex and breaks us
 #define _COMPLEX_DEFINED
@@ -168,9 +178,9 @@ extern THREAD_LOCAL Py_LOCAL_SYMBOL PyThreadState *tstate_current;
 extern Py_LOCAL_SYMBOL int8_t *_graalpy_finalizing;
 #define graalpy_finalizing (_graalpy_finalizing != NULL && *_graalpy_finalizing)
 
-void graalpy_dealloc_stack_grow(PyThreadState *tstate);
-void graalpy_dealloc_stack_push(PyThreadState *tstate, PyObject *op);
-void graalpy_dealloc_stack_pop(PyThreadState *tstate, PyObject *op);
+Py_LOCAL_SYMBOL void graalpy_dealloc_stack_grow(PyThreadState *tstate);
+Py_LOCAL_SYMBOL void graalpy_dealloc_stack_push(PyThreadState *tstate, PyObject *op);
+Py_LOCAL_SYMBOL void graalpy_dealloc_stack_pop(PyThreadState *tstate, PyObject *op);
 
 #if (__linux__ && __GNU_LIBRARY__)
 #include <stdlib.h>

graalpython/com.oracle.graal.python.cext/src/pystate.c

@@ -31,7 +31,6 @@
 #include "capi.h"
 #include <trufflenfi.h>
 
-extern TruffleContext* TRUFFLE_CONTEXT;
 static THREAD_LOCAL int graalpy_attached_thread = 0;
 static THREAD_LOCAL int graalpy_gilstate_counter = 0;
 
@@ -2290,22 +2289,22 @@ PyGILState_Check(void)
 
     return (tstate == gilstate_tss_get(runtime));
 #else // GraalPy change
-    int attached = 0;
     /*
      * PyGILState_Check is allowed to be called from a new thread that didn't yet setup the GIL.
-     * If we don't attach the thread ourselves, the upcall will work because NFI will attach
-     * the thread automatically, but it won't create the context which would then break
-     * subsequent PyGILState_Ensure.
+     * We must attach it before calling into Java so the upcall has an entered polyglot context.
      */
-    if (TRUFFLE_CONTEXT) {
-        if ((*TRUFFLE_CONTEXT)->getTruffleEnv(TRUFFLE_CONTEXT) == NULL) {
-            (*TRUFFLE_CONTEXT)->attachCurrentThread(TRUFFLE_CONTEXT);
-            attached = 1;
+    int attached = 0;
+    if (graalpy_attach_native_thread) {
+        attached = graalpy_attach_native_thread();
+        if (attached == GRAALPY_ATTACH_NATIVE_FAILED) {
+            return 0;
         }
+    } else {
+        return 0;
     }
     int ret = GraalPyPrivate_GILState_Check();
-    if (attached) {
-        (*TRUFFLE_CONTEXT)->detachCurrentThread(TRUFFLE_CONTEXT);
+    if (attached == GRAALPY_ATTACH_NATIVE_OWNED && graalpy_detach_native_thread) {
+        graalpy_detach_native_thread();
     }
     return ret;
 #endif // GraalPy change
@@ -2362,13 +2361,18 @@ PyGILState_Ensure(void)
 
     return has_gil ? PyGILState_LOCKED : PyGILState_UNLOCKED;
 #else // GraalPy change
-    if (TRUFFLE_CONTEXT) {
-        if ((*TRUFFLE_CONTEXT)->getTruffleEnv(TRUFFLE_CONTEXT) == NULL) {
-            (*TRUFFLE_CONTEXT)->attachCurrentThread(TRUFFLE_CONTEXT);
+    if (!graalpy_attach_native_thread) {
+        Py_FatalError("PyGILState_Ensure called before GraalPy C API initialization");
+    }
+    if (!graalpy_attached_thread) {
+        int attached = graalpy_attach_native_thread();
+        if (attached == GRAALPY_ATTACH_NATIVE_FAILED) {
+            Py_FatalError("Could not attach native thread to the polyglot context");
+        } else if (attached == GRAALPY_ATTACH_NATIVE_OWNED) {
             graalpy_attached_thread = 1;
         }
-        graalpy_gilstate_counter++;
     }
+    graalpy_gilstate_counter++;
     return GraalPyPrivate_GILState_Ensure() ? PyGILState_UNLOCKED : PyGILState_LOCKED;
 #endif // GraalPy change
 }
@@ -2428,20 +2432,20 @@ PyGILState_Release(PyGILState_STATE oldstate)
     if (oldstate == PyGILState_UNLOCKED) {
         GraalPyPrivate_GILState_Release();
     }
-    if (TRUFFLE_CONTEXT) {
-        graalpy_gilstate_counter--;
-        if (graalpy_gilstate_counter == 0 && graalpy_attached_thread) {
-            GraalPyPrivate_BeforeThreadDetach();
-            (*TRUFFLE_CONTEXT)->detachCurrentThread(TRUFFLE_CONTEXT);
-            graalpy_attached_thread = 0;
-            /*
-             * The thread state on the Java-side is cleared in GraalPyPrivate_BeforeThreadDetach.
-             * As part of that the tstate_current pointer should have been set to NULL to make
-             * sure to fetch a fresh pointer the next time we attach. Just to be sure, we clear
-             * it here too:
-             */
-            tstate_current = NULL;
+    graalpy_gilstate_counter--;
+    if (graalpy_gilstate_counter == 0 && graalpy_attached_thread) {
+        GraalPyPrivate_BeforeThreadDetach();
+        if (graalpy_detach_native_thread) {
+            graalpy_detach_native_thread();
         }
+        graalpy_attached_thread = 0;
+        /*
+         * The thread state on the Java-side is cleared in GraalPyPrivate_BeforeThreadDetach.
+         * As part of that the tstate_current pointer should have been set to NULL to make
+         * sure to fetch a fresh pointer the next time we attach. Just to be sure, we clear
+         * it here too:
+         */
+        tstate_current = NULL;
     }
 #endif // GraalPy change
 }

graalpython/com.oracle.graal.python.test/src/tests/cpyext/test_list.py

@@ -1,4 +1,4 @@
-# Copyright (c) 2018, 2025, Oracle and/or its affiliates. All rights reserved.
+# Copyright (c) 2018, 2026, Oracle and/or its affiliates. All rights reserved.
 # DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 #
 # The Universal Permissive License (UPL), Version 1.0
@@ -167,36 +167,64 @@ class TestPyList(CPyExtTestCase):
     test_PyList_SetItem = CPyExtFunction(
         _reference_setitem,
         lambda: (
-            (4, 0, 0),
-            (4, 3, 5),
+            (4, 0, 0, False),
+            (4, 3, 5, False),
+            (4, 0, 0, True),
+            (4, 3, 5, True),
         ),
-        code='''PyObject* wrap_PyList_SetItem(Py_ssize_t capacity, Py_ssize_t idx, PyObject* new_item) {
+        code='''PyObject* wrap_PyList_SetItem(Py_ssize_t capacity, Py_ssize_t idx, PyObject* new_item, int init_with_none) {
             PyObject *newList = PyList_New(capacity);
             Py_ssize_t i;
             for (i = 0; i < capacity; i++) {
-                if (i == idx) {
-                    Py_INCREF(new_item);
-                    PyList_SetItem(newList, i, new_item);
-                } else {
+                if (init_with_none || i != idx) {
                     Py_INCREF(Py_None);
                     PyList_SetItem(newList, i, Py_None);
                 }
             }
+            Py_INCREF(new_item);
+            PyList_SetItem(newList, idx, new_item);
             return newList;
         }
         ''',
         resultspec="O",
-        argspec='nnO',
-        arguments=["Py_ssize_t capacity", "Py_ssize_t size", "PyObject* new_item"],
+        argspec='nnOp',
+        arguments=["Py_ssize_t capacity", "Py_ssize_t size", "PyObject* new_item", "int init_with_none"],
         callfunction="wrap_PyList_SetItem",
         cmpfunc=unhandled_error_compare
     )
 
+    test_native_list_setitem_decrefs_none = CPyExtFunction(
+        lambda args: [args[0]],
+        lambda: (
+            ("new item",),
+        ),
+        code='''PyObject* wrap_native_list_setitem_decrefs_none(PyObject* new_item) {
+            PyObject *list = PyList_New(1);
+            if (list == NULL) {
+                return NULL;
+            }
+            Py_INCREF(Py_None);
+            PyList_SET_ITEM(list, 0, Py_None);
+            if (PySequence_SetItem(list, 0, new_item) < 0) {
+                Py_DECREF(list);
+                return NULL;
+            }
+            return list;
+        }
+        ''',
+        resultspec="O",
+        argspec='O',
+        arguments=["PyObject* new_item"],
+        callfunction="wrap_native_list_setitem_decrefs_none",
+        cmpfunc=unhandled_error_compare
+    )
+
     test_PyList_SET_ITEM = CPyExtFunction(
         _wrap_list_fun(_reference_SET_ITEM),
         lambda: (
             ([1,2,3,4], 0, _reference_SET_ITEM),
             ([1,2,3,4], 3, _reference_SET_ITEM),
+            ([None], 0, 0),
         ),
         code='''PyObject* wrap_PyList_SET_ITEM(PyObject* op, Py_ssize_t idx, PyObject* newitem) {
             Py_INCREF(newitem);

graalpython/com.oracle.graal.python.test/src/tests/cpyext/test_thread.py

@@ -82,8 +82,7 @@ class TestPyThread(CPyExtTestCase):
     )
 
 
-# TODO(native-access) support ENV - thread attach/detach
-@unittest.skipIf(True, "Needs pthread")
+@unittest.skipIf(sys.platform == 'win32', "Needs pthread")
 class TestNativeThread(unittest.TestCase):
     def test_register_new_thread(self):
         TestThread = CPyExtType(