8000 Add _PyType_Fetch and use incref before setting attribute on type · python/cpython@c0e757c · GitHub
[go: up one dir, main page]
Skip to content
8000

Commit c0e757c

Browse files
DinoVDinoV
committed
Add _PyType_Fetch and use incref before setting attribute on type
Makes setting an attribute on a class and signaling type modified atomic Avoid adding re-entrancy exposing the type cache in an inconsistent state by decrefing after type is updated
1 parent 21336aa commit c0e757c

File tree

15 files changed

+416
-105
lines changed

15 files changed

+416
-105
lines changed

‎Include/cpython/object.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,7 @@ typedef struct _heaptypeobject {
275275

276276
PyAPI_FUNC(const char *) _PyType_Name(PyTypeObject *);
277277
PyAPI_FUNC(PyObject *) _PyType_Lookup(PyTypeObject *, PyObject *);
278+
PyAPI_FUNC(PyObject *) _PyType_Fetch(PyTypeObject *, PyObject *);
278279
PyAPI_FUNC(PyObject *) PyType_GetDict(PyTypeObject *);
279280

280281
PyAPI_FUNC(int) PyObject_Print(PyObject *, FILE *, int);

‎Include/cpython/pyatomic.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -484,6 +484,9 @@ _Py_atomic_store_int_release(int *obj, int value);
484484
static inline int
485485
_Py_atomic_load_int_acquire(const int *obj);
486486

487+
static inline void
488+
_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value);
489+
487490
static inline void
488491
_Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value);
489492

‎Include/cpython/pyatomic_gcc.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,10 @@ static inline int
516516
_Py_atomic_load_int_acquire(const int *obj)
517517
{ return __atomic_load_n(obj, __ATOMIC_ACQUIRE); }
518518

519+
static inline void
520+
_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value)
521+
{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }
522+
519523
static inline void
520524
_Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value)
521525
{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }

‎Include/cpython/pyatomic_msc.h

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -989,6 +989,19 @@ _Py_atomic_load_int_acquire(const int *obj)
989989
#endif
990990
}
991991

992+
static inline void
993+
_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value)
994+
{
995+
#if defined(_M_X64) || defined(_M_IX86)
996+
*(uint32_t volatile *)obj = value;
997+
#elif defined(_M_ARM64)
998+
_Py_atomic_ASSERT_ARG_TYPE(unsigned __int32);
999+
__stlr32((unsigned __int32 volatile *)obj, (unsigned __int32)value);
1000+
#else
1001+
# error "no implementation of _Py_atomic_store_uint32_release"
1002+
#endif
1003+
}
1004+
9921005
static inline void
9931006
_Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value)
9941007
{

‎Include/cpython/pyatomic_std.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -911,6 +911,14 @@ _Py_atomic_load_int_acquire(const int *obj)
911911
memory_order_acquire);
912912
}
913913

914+
static inline void
915+
_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value)
916+
{
917+
_Py_USING_STD;
918+
atomic_store_explicit((_Atomic(uint32_t)*)obj, value,
919+
memory_order_release);
920+
}
921+
914922
static inline void
915923
_Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value)
916924
{

‎Include/internal/pycore_dict.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,8 @@ PyAPI_FUNC(PyObject *)_PyDict_LoadGlobal(PyDictObject *, PyDictObject *, PyObjec
106106
/* Consumes references to key and value */
107107
PyAPI_FUNC(int) _PyDict_SetItem_Take2(PyDictObject *op, PyObject *key, PyObject *value);
108108
extern int _PyObjectDict_SetItem(PyTypeObject *tp, PyObject **dictptr, PyObject *name, PyObject *value);
109+
extern int _PyDict_SetItem_LockHeld(PyDictObject *dict, PyObject *name, PyObject *value);
110+
extern int _PyDict_GetItemRef_LockHeld(PyObject *op, PyObject *key, PyObject **result);
109111

110112
extern int _PyDict_Pop_KnownHash(
111113
PyDictObject *dict,

‎Lib/test/test_class.py

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -882,6 +882,25 @@ class Foo:
882882
f.a = 3
883883
self.assertEqual(f.a, 3)
884884

885+
def test_store_attr_type_cache(self):
886+
"""Verifies that the type cache doesn't provide a value which is
887+
inconsistent from the dict."""
888+
class X:
889+
def __del__(inner_self):
890+
v = C.a
891+
self.assertEqual(v, C.__dict__['a'])
892+
893+
class C:
894+
a = X()
895+
896+
# prime the cache
897+
C.a
898+
C.a
899+
900+
# destructor shouldn't be able to see inconsisent state
901+
C.a = X()
902+
C.a = X()
903+
885904

886905
if __name__ == '__main__':
887906
unittest.main()

‎Lib/test/test_free_threading/test_type.py

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
import unittest
2+
3+
from threading import Thread
4+
from multiprocessing.dummy import Pool
5+
from unittest import TestCase
6+
7+
from test.support import is_wasi
8+
9+
10+
NTHREADS = 6
11+
BOTTOM = 0
12+
TOP = 1000
13+
ITERS = 100
14+
15+
class A:
16+
attr = 1
17+
18+
@unittest.skipIf(is_wasi, "WASI has no threads.")
19+
class TestType(TestCase):
20+
def test_attr_cache(self):
21+
def read(id0):
22+
for _ in range(ITERS):
23+
for _ in range(BOTTOM, TOP):
24+
A.attr
25+
26+
def write(id0):
27+
for _ in range(ITERS):
28+
for _ in range(BOTTOM, TOP):
29+
# Make _PyType_Lookup cache hot first
30+
A.attr
31+
A.attr
32+
x = A.attr
33+
x += 1
34+
A.attr = x
35+
36+
37+
with Pool(NTHREADS) as pool:
38+
pool.apply_async(read, (1,))
39+
pool.apply_async(write, (1,))
40+
pool.close()
41+
pool.join()
42+
43+
def test_attr_cache_consistency(self):
44+
class C:
45+
x = 0
46+
47+
DONE = False
48+
def writer_func():
49+
for i in range(10000):
50+
C.x
51+
C.x
52+
C.x += 1
53+
nonlocal DONE
54+
DONE = True
55+
56+
def reader_func():
57+
while True:
58+
# We should always see a greater value read from the type than the
59+
# dictionary
60+
a = C.__dict__['x']
61+
b = C.x
62+
self.assertGreaterEqual(b, a)
63+
64+
if DONE:
65+
break
66+
67+
self.run_one(writer_func, reader_func)
68+
69+
def test_attr_cache_consistency_subclass(self):
70+
class C:
71+
x = 0
72+
73+
class D(C):
74+
pass
75+
76+
DONE = False
77+
def writer_func():
78+
for i in range(10000):
79+
D.x
80+
D.x
81+
C.x += 1
82+
nonlocal DONE
83+
DONE = True
84+
85+
def reader_func():
86+
while True:
87+
# We should always see a greater value read from the type than the
88+
# dictionary
89+
a = C.__dict__['x']
90+
b = D.x
91+
self.assertGreaterEqual(b, a)
92+
93+
if DONE:
94+
break
95+
96+
self.run_one(writer_func, reader_func)
97+
98+
def run_one(self, writer_func, reader_func):
99+
writer = Thread(target=writer_func)
100+
readers = []
101+
for x in range(30):
102+
reader = Thread(target=reader_func)
103+
readers.append(reader)
104+
reader.start()
105+
106+
writer.start()
107+
writer.join()
108+
for reader in readers:
109+
reader.join()
110+
111+
if __name__ == "__main__":
112+
unittest.main()

‎Modules/_collectionsmodule.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2511,9 +2511,9 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping,
25112511
/* Only take the fast path when get() and __setitem__()
25122512
* have not been overridden.
25132513
*/
2514-
mapping_get = _PyType_Lookup(Py_TYPE(mapping), &_Py_ID(get));
2514+
mapping_get = _PyType_Fetch(Py_TYPE(mapping), &_Py_ID(get));
25152515
dict_get = _PyType_Lookup(&PyDict_Type, &_Py_ID(get));
2516-
mapping_setitem = _PyType_Lookup(Py_TYPE(mapping), &_Py_ID(__setitem__));
2516+
mapping_setitem = _PyType_Fetch(Py_TYPE(mapping), &_Py_ID(__setitem__));
25172517
dict_setitem = _PyType_Lookup(&PyDict_Type, &_Py_ID(__setitem__));
25182518

25192519
if (mapping_get != NULL && mapping_get == dict_get &&
@@ -2587,6 +2587,8 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping,
25872587
}
25882588

25892589
done:
2590+
Py_XDECREF(mapping_get);
2591+
Py_XDECREF(mapping_setitem);
25902592
Py_DECREF(it);
25912593
Py_XDECREF(key);
25922594
Py_XDECREF(newval);

‎Modules/_lsprof.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -175,8 +175,7 @@ normalizeUserObj(PyObject *obj)
175175
PyObject *modname = fn->m_module;
176176

177177
if (name != NULL) {
178-
PyObject *mo = _PyType_Lookup(Py_TYPE(self), name);
179-
Py_XINCREF(mo);
178+
PyObject *mo = _PyType_Fetch(Py_TYPE(self), name);
180179
Py_DECREF(name);
181180
if (mo != NULL) {
182181
PyObject *res = PyObject_Repr(mo);

‎Modules/_testcapi/gc.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,10 +99,11 @@ slot_tp_del(PyObject *self)
9999
return;
100100
}
101101
/* Execute __del__ method, if any. */
102-
del = _PyType_Lookup(Py_TYPE(self), tp_del);
102+
del = _PyType_Fetch(Py_TYPE(self), tp_del);
103103
Py_DECREF(tp_del);
104104
if (del != NULL) {
105105
res = PyObject_CallOneArg(del, self);
106+
Py_DECREF(del);
106107
if (res == NULL)
107108
PyErr_WriteUnraisable(del);
108109
else

‎Objects/classobject.c

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -188,15 +188,18 @@ method_getattro(PyObject *obj, PyObject *name)
188188
if (PyType_Ready(tp) < 0)
189189
return NULL;
190190
}
191-
descr = _PyType_Lookup(tp, name);
191+
descr = _PyType_Fetch(tp, name);
192192
}
193193

194194
if (descr != NULL) {
195195
descrgetfunc f = TP_DESCR_GET(Py_TYPE(descr));
196-
if (f != NULL)
197-
return f(descr, obj, (PyObject *)Py_TYPE(obj));
196+
if (f != NULL) {
197+
PyObject *res = f(descr, obj, (PyObject *)Py_TYPE(obj));
198+
Py_DECREF(descr);
199+
return res;
200+
}
198201
else {
199-
return Py_NewRef(descr);
202+
return descr;
200203
}
201204
}
202205

@@ -410,14 +413,17 @@ instancemethod_getattro(PyObject *self, PyObject *name)
410413
if (PyType_Ready(tp) < 0)
411414
return NULL;
412415
}
413-
descr = _PyType_Lookup(tp, name);
416+
descr = _PyType_Fetch(tp, name);
414417

415418
if (descr != NULL) {
416419
descrgetfunc f = TP_DESCR_GET(Py_TYPE(descr));
417-
if (f != NULL)
418-
return f(descr, self, (PyObject *)Py_TYPE(self));
420+
if (f != NULL) {
421+
PyObject *res = f(descr, self, (PyObject *)Py_TYPE(self));
422+
Py_DECREF(descr);
423+
return res;
424+
}
419425
else {
420-
return Py_NewRef(descr);
426+
return descr;
421427
}
422428
}
423429

‎Objects/dictobject.c

Lines changed: 38 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2317,6 +2317,38 @@ PyDict_GetItemRef(PyObject *op, PyObject *key, PyObject **result)
23172317
return _PyDict_GetItemRef_KnownHash(op, key, hash, result);
23182318
}
23192319

2320+
int
2321+
_PyDict_GetItemRef_LockHeld(PyObject *op, PyObject *key, PyObject **result)
2322+
{
2323+
ASSERT_DICT_LOCKED(op);
2324+
assert(PyUnicode_CheckExact(key));
2325+
2326+
Py_hash_t hash;
2327+
if ((hash = unicode_get_hash(key)) == -1)
2328+
{
2329+
hash = PyObject_Hash(key);
2330+
if (hash == -1) {
2331+
*result = NULL;
2332+
return -1;
2333+
}
2334+
}
2335+
2336+
PyDictObject*mp = (PyDictObject *)op;
2337+
2338+
PyObject *value;
2339+
Py_ssize_t ix = _Py_dict_lookup(mp, key, hash, &value);
2340+
assert(ix >= 0 || value == NULL);
2341+
if (ix == DKIX_ERROR) {
2342+
*result = NULL;
2343+
return -1;
2344+
}
2345+
if (value == NULL) {
2346+
*result = NULL;
2347+
return 0; // missing key
2348+
}
2349+
*result = Py_NewRef(value);
2350+
return 1; // key is present
2351+
}
23202352

23212353
/* Variant of PyDict_GetItem() that doesn't suppress exceptions.
23222354
This returns NULL *with* an exception set if an exception occurred.
@@ -6704,8 +6736,8 @@ _PyObject_MaterializeManagedDict(PyObject *obj)
67046736
return dict;
67056737
}
67066738

6707-
static int
6708-
set_or_del_lock_held(PyDictObject *dict, PyObject *name, PyObject *value)
6739+
int
6740+
_PyDict_SetItem_LockHeld(PyDictObject *dict, PyObject *name, PyObject *value)
67096741
{
67106742
if (value == NULL) {
67116743
Py_hash_t hash;
@@ -6767,7 +6799,7 @@ store_instance_attr_lock_held(PyObject *obj, PyDictValues *values,
67676799
// so that no one else will see it.
67686800
dict = make_dict_from_instance_attributes(PyInterpreterState_Get(), keys, values);
67696801
if (dict == NULL ||
6770-
set_or_del_lock_held(dict, name, value) < 0) {
6802+
_PyDict_SetItem_LockHeld(dict, name, value) < 0) {
67716803
Py_XDECREF(dict);
67726804
return -1;
67736805
}
@@ -6779,7 +6811,7 @@ store_instance_attr_lock_held(PyObject *obj, PyDictValues *values,
67796811

67806812
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(dict);
67816813

6782-
res = set_or_del_lock_held (dict, name, value);
6814+
res = _PyDict_SetItem_LockHeld(dict, name, value);
67836815
return res;
67846816
}
67856817

@@ -6822,7 +6854,7 @@ store_instance_attr_dict(PyObject *obj, PyDictObject *dict, PyObject *name, PyOb
68226854
res = store_instance_attr_lock_held(obj, values, name, value);
68236855
}
68246856
else {
6825-
res = set_or_del_lock_held(dict, name, value);
6857+
res = _PyDict_SetItem_LockHeld(dict, name, value);
68266858
}
68276859
Py_END_CRITICAL_SECTION();
68286860
return res;
@@ -7253,6 +7285,7 @@ _PyObjectDict_SetItem(PyTypeObject *tp, PyObject **dictptr,
72537285
res = PyDict_SetItem(dict, key, value);
72547286
}
72557287
}
7288+
72567289
ASSERT_CONSISTENT(dict);
72577290
return res;
72587291
}

0 commit comments

Comments
 (0)
0