8000 gh-128923: Use zero to indicate unassigned unique id (#128925) · python/cpython@d66c08a · GitHub
[go: up one dir, main page]
Skip to content

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

Lines changed: 1 addition & 2 deletions
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

Lines changed: 12 additions & 12 deletions
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]--;
398 9E88 398
}
399399
else {
400400
// Directly decref the object if the id is not assigned or if

‎Include/internal/pycore_uniqueid.h

Lines changed: 4 additions & 2 deletions
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

Lines changed: 7 additions & 0 deletions
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

Lines changed: 64 additions & 0 deletions
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

Lines changed: 1 addition & 1 deletion
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

Lines changed: 4 additions & 2 deletions
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

Lines changed: 1 addition & 1 deletion
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

Lines changed: 16 additions & 13 deletions
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)
0