Skip to content

Commit d66c08a

Browse files
colesburycolesbury
authored
gh-128923: Use zero to indicate unassigned unique id (#128925)
In the free threading build, the per thread reference counting uses a unique id for some objects to index into the local reference count table. Use 0 instead of -1 to indicate that the id is not assigned. This avoids bugs where zero-initialized heap type objects look like they have a unique id assigned.
1 parent 767c89b commit d66c08a

File tree

9 files changed

+110
-33
lines changed

9 files changed

+110
-33
lines changed

‎Include/internal/pycore_dict.h

+1-2
Original file line numberDiff line numberDiff line change
@@ -347,8 +347,7 @@ PyDictObject *_PyObject_MaterializeManagedDict_LockHeld(PyObject *);
347347
static inline Py_ssize_t
348348
_PyDict_UniqueId(PyDictObject *mp)
349349
{
350-
// Offset by one so that _ma_watcher_tag=0 represents an unassigned id
351-
return (Py_ssize_t)(mp->_ma_watcher_tag >> DICT_UNIQUE_ID_SHIFT) - 1;
350+
return (Py_ssize_t)(mp->_ma_watcher_tag >> DICT_UNIQUE_ID_SHIFT);
352351
}
353352

354353
static inline void

‎Include/internal/pycore_object.h

+12-12
Original file line numberDiff line numberDiff line change
@@ -336,20 +336,20 @@ _Py_THREAD_INCREF_OBJECT(PyObject *obj, Py_ssize_t unique_id)
336336
{
337337
_PyThreadStateImpl *tstate = (_PyThreadStateImpl *)_PyThreadState_GET();
338338

339-
// Unsigned comparison so that `unique_id=-1`, which indicates that
340-
// per-thread refcounting has been disabled on this object, is handled by
341-
// the "else".
342-
if ((size_t)unique_id < (size_t)tstate->refcounts.size) {
339+
// The table index is `unique_id - 1` because 0 is not a valid unique id.
340+
// Unsigned comparison so that `idx=-1` is handled by the "else".
341+
size_t idx = (size_t)(unique_id - 1);
342+
if (idx < (size_t)tstate->refcounts.size) {
343343
# ifdef Py_REF_DEBUG
344344
_Py_INCREF_IncRefTotal();
345345
# endif
346346
_Py_INCREF_STAT_INC();
347-
tstate->refcounts.values[unique_id]++;
347+
tstate->refcounts.values[idx]++;
348348
}
349349
else {
350350
// The slow path resizes the per-thread refcount array if necessary.
351-
// It handles the unique_id=-1 case to keep the inlinable function smaller.
352-
_PyObject_ThreadIncrefSlow(obj, unique_id);
351+
// It handles the unique_id=0 case to keep the inlinable function smaller.
352+
_PyObject_ThreadIncrefSlow(obj, idx);
353353
}
354354
}
355355

@@ -386,15 +386,15 @@ _Py_THREAD_DECREF_OBJECT(PyObject *obj, Py_ssize_t unique_id)
386386
{
387387
_PyThreadStateImpl *tstate = (_PyThreadStateImpl *)_PyThreadState_GET();
388388

389-
// Unsigned comparison so that `unique_id=-1`, which indicates that
390-
// per-thread refcounting has been disabled on this object, is handled by
391-
// the "else".
392-
if ((size_t)unique_id < (size_t)tstate->refcounts.size) {
389+
// The table index is `unique_id - 1` because 0 is not a valid unique id.
390+
// Unsigned comparison so that `idx=-1` is handled by the "else".
391+
size_t idx = (size_t)(unique_id - 1);
392+
if (idx < (size_t)tstate->refcounts.size) {
393393
# ifdef Py_REF_DEBUG
394394
_Py_DECREF_DecRefTotal();
395395
# endif
396396
_Py_DECREF_STAT_INC();
397-
tstate->refcounts.values[unique_id]--;
397+
tstate->refcounts.values[idx]--;
398398
}
399399
else {
400400
// Directly decref the object if the id is not assigned or if

‎Include/internal/pycore_uniqueid.h

+4-2
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ extern "C" {
1616
// Per-thread reference counting is used along with deferred reference
1717
// counting to avoid scaling bottlenecks due to reference count contention.
1818
//
19-
// An id of -1 is used to indicate that an object doesn't use per-thread
19+
// An id of 0 is used to indicate that an object doesn't use per-thread
2020
// refcounting. This value is used when the object is finalized by the GC
2121
// and during interpreter shutdown to allow the object to be
2222
// deallocated promptly when the object's refcount reaches zero.
@@ -45,6 +45,8 @@ struct _Py_unique_id_pool {
4545
Py_ssize_t size;
4646
};
4747

48+
#define _Py_INVALID_UNIQUE_ID 0
49+
4850
// Assigns the next id from the pool of ids.
4951
extern Py_ssize_t _PyObject_AssignUniqueId(PyObject *obj);
5052

@@ -65,7 +67,7 @@ extern void _PyObject_FinalizePerThreadRefcounts(_PyThreadStateImpl *tstate);
6567
extern void _PyObject_FinalizeUniqueIdPool(PyInterpreterState *interp);
6668

6769
// Increfs the object, resizing the thread-local refcount array if necessary.
68-
PyAPI_FUNC(void) _PyObject_ThreadIncrefSlow(PyObject *obj, Py_ssize_t unique_id);
70+
PyAPI_FUNC(void) _PyObject_ThreadIncrefSlow(PyObject *obj, size_t idx);
6971

7072
#endif /* Py_GIL_DISABLED */
7173

‎Lib/test/test_capi/test_type.py

+7
Original file line numberDiff line numberDiff line change
@@ -67,3 +67,10 @@ class FreezeThis(metaclass=Meta):
6767
Base.value = 3
6868
type_freeze(FreezeThis)
6969
self.assertEqual(FreezeThis.value, 2)
70+
71+
def test_manual_heap_type(self):
72+
# gh-128923: test that a manually allocated and initailized heap type
73+
# works correctly
74+
ManualHeapType = _testcapi.ManualHeapType
75+
for i in range(100):
76+
self.assertIsInstance(ManualHeapType(), ManualHeapType)

‎Modules/_testcapimodule.c

+64
Original file line numberDiff line numberDiff line change
@@ -4149,6 +4149,61 @@ static PyTypeObject ContainerNoGC_type = {
41494149
.tp_new = ContainerNoGC_new,
41504150
};
41514151

4152+
/* Manually allocated heap type */
4153+
4154+
typedef struct {
4155+
PyObject_HEAD
4156+
PyObject *dict;
4157+
} ManualHeapType;
4158+
4159+
static int
4160+
ManualHeapType_traverse(PyObject *self, visitproc visit, void *arg)
4161+
{
4162+
ManualHeapType *mht = (ManualHeapType *)self;
4163+
Py_VISIT(mht->dict);
4164+
return 0;
4165+
}
4166+
4167+
static void
4168+
ManualHeapType_dealloc(PyObject *self)
4169+
{
4170+
ManualHeapType *mht = (ManualHeapType *)self;
4171+
PyObject_GC_UnTrack(self);
4172+
Py_XDECREF(mht->dict);
4173+
PyTypeObject *type = Py_TYPE(self);
4174+
Py_TYPE(self)->tp_free(self);
4175+
Py_DECREF(type);
4176+
}
4177+
4178+
static PyObject *
4179+
create_manual_heap_type(void)
4180+
{
4181+
// gh-128923: Ensure that a heap type allocated through PyType_Type.tp_alloc
4182+
// with minimal initialization works correctly.
4183+
PyHeapTypeObject *heap_type = (PyHeapTypeObject *)PyType_Type.tp_alloc(&PyType_Type, 0);
4184+
if (heap_type == NULL) {
4185+
return NULL;
4186+
}
4187+
PyTypeObject* type = &heap_type->ht_type;
4188+
type->tp_basicsize = sizeof(ManualHeapType);
4189+
type->tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HEAPTYPE | Py_TPFLAGS_HAVE_GC;
4190+
type->tp_new = PyType_GenericNew;
4191+
type->tp_name = "ManualHeapType";
4192+
type->tp_dictoffset = offsetof(ManualHeapType, dict);
4193+
type->tp_traverse = ManualHeapType_traverse;
4194+
type->tp_dealloc = ManualHeapType_dealloc;
4195+
heap_type->ht_name = PyUnicode_FromString(type->tp_name);
4196+
if (!heap_type->ht_name) {
4197+
Py_DECREF(type);
4198+
return NULL;
4199+
}
4200+
heap_type->ht_qualname = Py_NewRef(heap_type->ht_name);
4201+
if (PyType_Ready(type) < 0) {
4202+
Py_DECREF(type);
4203+
return NULL;
4204+
}
4205+
return (PyObject *)type;
4206+
}
41524207

41534208
static struct PyModuleDef _testcapimodule = {
41544209
PyModuleDef_HEAD_INIT,
@@ -4283,6 +4338,15 @@ PyInit__testcapi(void)
42834338
(PyObject *) &ContainerNoGC_type) < 0)
42844339
return NULL;
42854340

4341+
PyObject *manual_heap_type = create_manual_heap_type();
4342+
if (manual_heap_type == NULL) {
4343+
return NULL;
4344+
}
4345+
if (PyModule_Add(m, "ManualHeapType", manual_heap_type) < 0) {
4346+
return NULL;
4347+
}
4348+
4349+
42864350
/* Include tests from the _testcapi/ directory */
42874351
if (_PyTestCapi_Init_Vectorcall(m) < 0) {
42884352
return NULL;

‎Objects/codeobject.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -1911,7 +1911,7 @@ code_dealloc(PyObject *self)
19111911
Py_XDECREF(co->co_linetable);
19121912
Py_XDECREF(co->co_exceptiontable);
19131913
#ifdef Py_GIL_DISABLED
1914-
assert(co->_co_unique_id == -1);
1914+
assert(co->_co_unique_id == _Py_INVALID_UNIQUE_ID);
19151915
#endif
19161916
if (co->_co_cached != NULL) {
19171917
Py_XDECREF(co->_co_cached->_co_code);

‎Objects/dictobject.c

+4-2
Original file line numberDiff line numberDiff line change
@@ -1659,15 +1659,17 @@ _PyDict_EnablePerThreadRefcounting(PyObject *op)
16591659
assert(PyDict_Check(op));
16601660
#ifdef Py_GIL_DISABLED
16611661
Py_ssize_t id = _PyObject_AssignUniqueId(op);
1662+
if (id == _Py_INVALID_UNIQUE_ID) {
1663+
return;
1664+
}
16621665
if ((uint64_t)id >= (uint64_t)DICT_UNIQUE_ID_MAX) {
16631666
_PyObject_ReleaseUniqueId(id);
16641667
return;
16651668
}
16661669

16671670
PyDictObject *mp = (PyDictObject *)op;
16681671
assert((mp->_ma_watcher_tag >> DICT_UNIQUE_ID_SHIFT) == 0);
1669-
// Plus 1 so that _ma_watcher_tag=0 represents an unassigned id
1670-
mp->_ma_watcher_tag += ((uint64_t)id + 1) << DICT_UNIQUE_ID_SHIFT;
1672+
mp->_ma_watcher_tag += (uint64_t)id << DICT_UNIQUE_ID_SHIFT;
16711673
#endif
16721674
}
16731675

‎Objects/typeobject.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -6160,7 +6160,7 @@ type_dealloc(PyObject *self)
61606160
Py_XDECREF(et->ht_module);
61616161
PyMem_Free(et->_ht_tpname);
61626162
#ifdef Py_GIL_DISABLED
6163-
assert(et->unique_id == -1);
6163+
assert(et->unique_id == _Py_INVALID_UNIQUE_ID);
61646164
#endif
61656165
et->ht_token = NULL;
61666166
Py_TYPE(type)->tp_free((PyObject *)type);

‎Python/uniqueid.c

+16-13
Original file line numberDiff line numberDiff line change
@@ -86,15 +86,17 @@ _PyObject_AssignUniqueId(PyObject *obj)
8686
if (pool->freelist == NULL) {
8787
if (resize_interp_type_id_pool(pool) < 0) {
8888
UNLOCK_POOL(pool);
89-
return -1;
89+
return _Py_INVALID_UNIQUE_ID;
9090
}
9191
}
9292

9393
_Py_unique_id_entry *entry = pool->freelist;
9494
pool->freelist = entry->next;
9595
entry->obj = obj;
9696
_PyObject_SetDeferredRefcount(obj);
97-
Py_ssize_t unique_id = (entry - pool->table);
97+
// The unique id is one plus the index of the entry in the table.
98+
Py_ssize_t unique_id = (entry - pool->table) + 1;
99+
assert(unique_id > 0);
98100
UNLOCK_POOL(pool);
99101
return unique_id;
100102
}
@@ -106,8 +108,9 @@ _PyObject_ReleaseUniqueId(Py_ssize_t unique_id)
106108
struct _Py_unique_id_pool *pool = &interp->unique_ids;
107109

108110
LOCK_POOL(pool);
109-
assert(unique_id >= 0 && unique_id < pool->size);
110-
_Py_unique_id_entry *entry = &pool->table[unique_id];
111+
assert(unique_id > 0 && unique_id <= pool->size);
112+
Py_ssize_t idx = unique_id - 1;
113+
_Py_unique_id_entry *entry = &pool->table[idx];
111114
entry->next = pool->freelist;
112115
pool->freelist = entry;
113116
UNLOCK_POOL(pool);
@@ -116,18 +119,18 @@ _PyObject_ReleaseUniqueId(Py_ssize_t unique_id)
116119
static Py_ssize_t
117120
clear_unique_id(PyObject *obj)
118121
{
119-
Py_ssize_t id = -1;
122+
Py_ssize_t id = _Py_INVALID_UNIQUE_ID;
120123
if (PyType_Check(obj)) {
121124
if (PyType_HasFeature((PyTypeObject *)obj, Py_TPFLAGS_HEAPTYPE)) {
122125
PyHeapTypeObject *ht = (PyHeapTypeObject *)obj;
123126
id = ht->unique_id;
124-
ht->unique_id = -1;
127+
ht->unique_id = _Py_INVALID_UNIQUE_ID;
125128
}
126129
}
127130
else if (PyCode_Check(obj)) {
128131
PyCodeObject *co = (PyCodeObject *)obj;
129132
id = co->_co_unique_id;
130-
co->_co_unique_id = -1;
133+
co->_co_unique_id = _Py_INVALID_UNIQUE_ID;
131134
}
132135
else if (PyDict_Check(obj)) {
133136
PyDictObject *mp = (PyDictObject *)obj;
@@ -141,23 +144,23 @@ void
141144
_PyObject_DisablePerThreadRefcounting(PyObject *obj)
142145
{
143146
Py_ssize_t id = clear_unique_id(obj);
144-
if (id >= 0) {
147+
if (id != _Py_INVALID_UNIQUE_ID) {
145148
_PyObject_ReleaseUniqueId(id);
146149
}
147150
}
148151

149152
void
150-
_PyObject_ThreadIncrefSlow(PyObject *obj, Py_ssize_t unique_id)
153+
_PyObject_ThreadIncrefSlow(PyObject *obj, size_t idx)
151154
{
152155
_PyThreadStateImpl *tstate = (_PyThreadStateImpl *)_PyThreadState_GET();
153-
if (unique_id < 0 || resize_local_refcounts(tstate) < 0) {
156+
if (((Py_ssize_t)idx) < 0 || resize_local_refcounts(tstate) < 0) {
154157
// just incref the object directly.
155158
Py_INCREF(obj);
156159
return;
157160
}
158161

159-
assert(unique_id < tstate->refcounts.size);
160-
tstate->refcounts.values[unique_id]++;
162+
assert(idx < (size_t)tstate->refcounts.size);
163+
tstate->refcounts.values[idx]++;
161164
#ifdef Py_REF_DEBUG
162165
_Py_IncRefTotal((PyThreadState *)tstate);
163166
#endif
@@ -217,7 +220,7 @@ _PyObject_FinalizeUniqueIdPool(PyInterpreterState *interp)
217220
if (obj != NULL) {
218221
Py_ssize_t id = clear_unique_id(obj);
219222
(void)id;
220-
assert(id == i);
223+
assert(id == i + 1);
221224
}
222225
}
223226
PyMem_Free(pool->table);

0 commit comments

Comments
 (0)