gh-125286: Bug fix for trace-refs logic and shared objects

Lib/test/support/singlephase_helper.py

@@ -0,0 +1,32 @@
+"""Helper module for _testembed.c test_subinterpreter_finalize test.
+"""
+
+import sys
+
+PREFIX = "shared_string_"
+SUFFIX = "DByGMRJRSEDp29PkiZQNHA"
+
+def init_sub1():
+    import _testsinglephase
+    # Create global object to be shared when imported a second time.
+    _testsinglephase._shared_list = []
+    # Create a new interned string, to be shared with the main interpreter.
+    _testsinglephase._shared_string = sys.intern(PREFIX + SUFFIX)
+
+
+def init_sub2():
+    # This sub-interpreter will share a reference to _shared_list with the
+    # first interpreter, since importing _testsinglephase will not initialize
+    # the module a second time but will just copy the global dict.  This
+    # situtation used to trigger a bug like gh-125286 if TraceRefs was enabled
+    # for the build.
+    import _testsinglephase
+
+
+def init_main():
+    global shared_str
+    # The first sub-interpreter has already interned this string value.  The
+    # return value from intern() will be the same string object created in
+    # sub-interpreter 1.  Assign it to a global so it lives until the main
+    # interpreter is shutdown.
+    shared_string = sys.intern(PREFIX + SUFFIX)

Misc/NEWS.d/next/Core_and_Builtins/2024-10-11-02-26-39.gh-issue-125286.2ii5_E.rst

@@ -0,0 +1,3 @@
+Fix the TraceRefs build to handle objects that are shared between
+interpreters (interned strings and globals of single-phase init extension
+modules).

Objects/object.c

@@ -2500,38 +2500,21 @@ _Py_ResurrectReference(PyObject *op)
 
 
 #ifdef Py_TRACE_REFS
-/* Make sure the ref is associated with the right interpreter.
- * This only needs special attention for heap-allocated objects
- * that have been immortalized, and only when the object might
- * outlive the interpreter where it was created.  That means the
- * object was necessarily created using a global allocator
- * (i.e. from the main interpreter).  Thus in that specific case
- * we move the object over to the main interpreter's refchain.
- *
- * This was added for the sake of the immortal interned strings,
- * where legacy subinterpreters share the main interpreter's
- * interned dict (and allocator), and therefore the strings can
- * outlive the subinterpreter.
- *
- * It may make sense to fold this into _Py_SetImmortalUntracked(),
- * but that requires further investigation.  In the meantime, it is
- * up to the caller to know if this is needed.  There should be
- * very few cases.
+/* Make sure the ref is traced in the main interpreter. This is used only by
+ * _PyUnicode_ClearInterned() to ensure interned strings are traced. Since
+ * interned strings can be shared between interpreters, the interpreter that
+ * created the string might not be the main interpreter. We trace it here so
+ * that _Py_ForgetReference() will handle it without error.
  */
 void
-_Py_NormalizeImmortalReference(PyObject *op)
+_Py_NormalizeReference(PyObject *op)
 {
-    assert(_Py_IsImmortal(op));
     PyInterpreterState *interp = _PyInterpreterState_GET();
-    if (!_PyRefchain_IsTraced(interp, op)) {
+    if (_PyRefchain_IsTraced(interp, op)) {
         return;
     }
     PyInterpreterState *main_interp = _PyInterpreterState_Main();
-    if (interp != main_interp
-           && interp->feature_flags & Py_RTFLAGS_USE_MAIN_OBMALLOC)
-    {
-        assert(!_PyRefchain_IsTraced(main_interp, op));
-        _PyRefchain_Remove(interp, op);
+    if (interp == main_interp) {
         _PyRefchain_Trace(main_interp, op);
     }
 }
@@ -2545,6 +2528,25 @@ _Py_ForgetReference(PyObject *op)
 
     PyInterpreterState *interp = _PyInterpreterState_GET();
 
+    if (!_PyRefchain_IsTraced(interp, op)) {
+        if (PyUnicode_CHECK_INTERNED(op)) {
+            // interned strings can be shared between the main interpreter and
+            // between sub-interpreters due to the shared interp dict.  See
+            // init_interned_dict().  In this case, the string was created and
+            // traced by a different sub-interpreter.
+            return;
+        }
+        PyInterpreterState *main_interp = _PyInterpreterState_Main();
+        if (interp != main_interp  &&
+            interp->feature_flags & Py_RTFLAGS_USE_MAIN_OBMALLOC) {
+            // objects stored in the globals of basic single-phase init
+            // (m_size == -1) extension modules can be shared between
+            // sub-interpreters.  In this case, the object was created
+            // and traced by a different sub-interpreter.
+            return;
+        }
+    }
+
 #ifdef SLOW_UNREF_CHECK
     if (!_PyRefchain_Get(interp, op)) {
         /* Not found */

Objects/unicodeobject.c

@@ -15445,7 +15445,7 @@ _PyUnicode_InternStatic(PyInterpreterState *interp, PyObject **p)
 }
 
 #ifdef Py_TRACE_REFS
-extern void _Py_NormalizeImmortalReference(PyObject *);
+extern void _Py_NormalizeReference(PyObject *);
 #endif
 
 static void
@@ -15463,10 +15463,6 @@ immortalize_interned(PyObject *s)
 #endif
     _PyUnicode_STATE(s).interned = SSTATE_INTERNED_IMMORTAL;
     _Py_SetImmortal(s);
-#ifdef Py_TRACE_REFS
-    /* Make sure the ref is associated with the right interpreter. */
-    _Py_NormalizeImmortalReference(s);
-#endif
 }
 
 static /* non-null */ PyObject*
@@ -15678,6 +15674,10 @@ _PyUnicode_ClearInterned(PyInterpreterState *interp)
     while (PyDict_Next(interned, &pos, &s, &ignored_value)) {
         assert(PyUnicode_IS_READY(s));
         int shared = 0;
+#ifdef Py_TRACE_REFS
+        /* Make sure the ref is associated with the right interpreter. */
+        _Py_NormalizeReference(s);
+#endif
         switch (PyUnicode_CHECK_INTERNED(s)) {
         case SSTATE_INTERNED_IMMORTAL:
             /* Make immortal interned strings mortal again. */

Programs/_testembed.c

@@ -2072,6 +2072,46 @@ static void configure_init_main(PyConfig *config)
 }
 
 
+static int test_subinterpreter_finalize(void)
+{
+    // This test is done by creating two subinterpreters and then sharing
+    // objects between them by importing a basic single-phase init extension
+    // (m_size == -1). Then we finalize the interpreters in the reverse order
+    // so that the interpreter that created the shared objects gets finalized
+    // first.
+    _testembed_Py_InitializeFromConfig();
+
+    PyThreadState *tstate1 = Py_NewInterpreter();
+    PyThreadState_Swap(tstate1);
+    PyRun_SimpleString(
+        "import test.support.singlephase_helper\n"
+        "test.support.singlephase_helper.init_sub1\n"
+    );
+
+    PyThreadState *tstate2 = Py_NewInterpreter();
+    PyThreadState_Swap(tstate2);
+    PyRun_SimpleString(
+        "import test.support.singlephase_helper\n"
+        "test.support.singlephase_helper.init_sub2\n"
+    );
+
+    PyThreadState *main_tstate = _PyRuntime.main_tstate;
+    PyThreadState_Swap(main_tstate);
+    PyRun_SimpleString(
+        "import test.support.singlephase_helper\n"
+        "test.support.singlephase_helper.init_main\n"
+    );
+
+    PyThreadState_Swap(tstate1);
+    Py_EndInterpreter(tstate1);
+    PyThreadState_Swap(tstate2);
+    Py_EndInterpreter(tstate2);
+    Py_Finalize();
+
+    return 0;
+}
+
+
 static int test_init_run_main(void)
 {
     PyConfig config;
@@ -2480,6 +2520,7 @@ static struct TestCase TestCases[] = {
     {"test_initconfig_get_api", test_initconfig_get_api},
     {"test_initconfig_exit", test_initconfig_exit},
     {"test_initconfig_module", test_initconfig_module},
+    {"test_subinterpreter_finalize", test_subinterpreter_finalize},
     {"test_run_main", test_run_main},
     {"test_run_main_loop", test_run_main_loop},
     {"test_get_argc_argv", test_get_argc_argv},