Address reviewer comments, and drop test_iteration_deopt.

Author: Yhg1s

Include/internal/pycore_list.h

@@ -8,14 +8,18 @@ extern "C" {
 #  error "this header requires Py_BUILD_CORE define"
 #endif
 
+#ifdef Py_GIL_DISABLED
+#include "pycore_stackref.h"
+#endif
+
 PyAPI_FUNC(PyObject*) _PyList_Extend(PyListObject *, PyObject *);
 extern void _PyList_DebugMallocStats(FILE *out);
 // _PyList_GetItemRef should be used only when the object is known as a list
 // because it doesn't raise TypeError when the object is not a list, whereas PyList_GetItemRef does.
 extern PyObject* _PyList_GetItemRef(PyListObject *, Py_ssize_t i);
 #ifdef Py_GIL_DISABLED
 // Returns -1 in case of races with other threads.
-extern int _PyList_GetItemRefNoLock(PyListObject *, Py_ssize_t, PyObject **);
+extern int _PyList_GetItemRefNoLock(PyListObject *, Py_ssize_t, _PyStackRef *);
 #endif
 
 #define _PyList_ITEMS(op) _Py_RVALUE(_PyList_CAST(op)->ob_item)

Include/internal/pycore_opcode_metadata.h

@@ -26,7 +26,6 @@
     'test_threadsignals',
     'test_weakref',
     'test_free_threading.test_slots',
-    'test_free_threading.test_iteration',
 ]
 
 

Include/internal/pycore_uop_metadata.h

@@ -417,7 +417,7 @@ _PyList_GetItemRef(PyListObject *list, Py_ssize_t i)
 
 #ifdef Py_GIL_DISABLED
 int
-_PyList_GetItemRefNoLock(PyListObject *list, Py_ssize_t i, PyObject **result)
+_PyList_GetItemRefNoLock(PyListObject *list, Py_ssize_t i, _PyStackRef *result)
 {
     assert(_Py_IsOwnedByCurrentThread((PyObject *)list) ||
            _PyObject_GC_IS_SHARED(list));
@@ -433,8 +433,8 @@ _PyList_GetItemRefNoLock(PyListObject *list, Py_ssize_t i, PyObject **result)
     if (!valid_index(i, cap)) {
         return 0;
     }
-    *result = _Py_TryXGetRef(&ob_item[i]);
-    if (*result == NULL) {
+    PyObject *obj = _Py_atomic_load_ptr(&ob_item[i]);
+    if (obj == NULL || !_Py_TryIncrefCompareStackRef(&ob_item[i], obj, result)) {
         return -1;
     }
     return 1;

Lib/test/libregrtest/tsan.py

@@ -3028,8 +3028,10 @@ dummy_func(
         replaced op(_ITER_JUMP_LIST, (iter -- iter)) {
             PyObject *iter_o = PyStackRef_AsPyObjectBorrow(iter);
             assert(Py_TYPE(iter_o) == &PyListIter_Type);
-// For free-threaded Python, the loop exit can happen at any point during item
-// retrieval, so separate ops don't make much sense.
+// For free-threaded Python, the loop exit can happen at any point during
+// item retrieval, so it doesn't make much sense to check and jump
+// separately before item retrieval. Any length check we do here can be
+// invalid by the time we actually try to fetch the item.
 #ifdef Py_GIL_DISABLED
             assert(_PyObject_IsUniquelyReferenced(iter_o));
 #else
@@ -3075,19 +3077,17 @@ dummy_func(
             assert(_Py_IsOwnedByCurrentThread((PyObject *)seq) ||
                    _PyObject_GC_IS_SHARED(seq));
             STAT_INC(FOR_ITER, hit);
-            PyObject *item;
-            int result = _PyList_GetItemRefNoLock(seq, it->it_index, &item);
+            int result = _PyList_GetItemRefNoLock(seq, it->it_index, &next);
             // A negative result means we lost a race with another thread
             // and we need to take the slow path.
-            EXIT_IF(result < 0);
+            DEOPT_IF(result < 0);
             if (result == 0) {
                 it->it_index = -1;
                 /* Jump forward oparg, then skip following END_FOR instruction */
                 JUMPBY(oparg + 1);
                 DISPATCH();
             }
             it->it_index++;
-            next = PyStackRef_FromPyObjectSteal(item);
 #else
             assert(it->it_index < PyList_GET_SIZE(seq));
             next = PyStackRef_FromPyObjectNew(PyList_GET_ITEM(seq, it->it_index++));
@@ -3110,9 +3110,8 @@ dummy_func(
 
         replaced op(_ITER_JUMP_TUPLE, (iter -- iter)) {
             PyObject *iter_o = PyStackRef_AsPyObjectBorrow(iter);
+            (void)iter_o;
             assert(Py_TYPE(iter_o) == &PyTupleIter_Type);
-// For free-threaded Python, the loop exit can happen at any point during item
-// retrieval, so separate ops don't make much sense.
 #ifdef Py_GIL_DISABLED
             assert(_PyObject_IsUniquelyReferenced(iter_o));
 #endif
@@ -3218,6 +3217,10 @@ dummy_func(
             PyGenObject *gen = (PyGenObject *)PyStackRef_AsPyObjectBorrow(iter);
             DEOPT_IF(Py_TYPE(gen) != &PyGen_Type);
 #ifdef Py_GIL_DISABLED
+            // Since generators can't be used by multiple threads anyway we
+            // don't need to deopt here, but this lets us work on making
+            // generators thread-safe without necessarily having to
+            // specialize them thread-safely as well.
             DEOPT_IF(!_PyObject_IsUniquelyReferenced((PyObject *)gen));
 #endif
             DEOPT_IF(gen->gi_frame_state >= FRAME_EXECUTING);

Lib/test/test_free_threading/test_iteration_deopt.py

@@ -2654,7 +2654,7 @@ _Py_Specialize_ForIter(_PyStackRef iter, _Py_CODEUNIT *instr, int oparg)
 #ifdef Py_GIL_DISABLED
     // Only specialize for uniquely referenced iterators, so that we know
     // they're only referenced by this one thread. This is more limiting
-    // than we need (event `it = iter(mylist); for item in it:` won't get
+    // than we need (even `it = iter(mylist); for item in it:` won't get
     // specialized) but we don't have a way to check whether we're the only
     // _thread_ who has access to the object.
     if (!_PyObject_IsUniquelyReferenced(iter_o))