Skip to content

Commit 3ca5511

Browse files
steve-ssteve-s
committed
[GR-54254] Cache managed slots native wrappers to preserve identity on the native side.
PullRequest: graalpython/3346
2 parents 7ac447c + 4838f4b commit 3ca5511

File tree

4 files changed

+91
-31
lines changed

4 files changed

+91
-31
lines changed

‎graalpython/com.oracle.graal.python.test/src/tests/cpyext/test_tp_slots.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,13 +47,21 @@
4747
static PyObject* get_tp_attro(PyObject* unused, PyObject* object) {
4848
return PyLong_FromVoidPtr(Py_TYPE(object)->tp_getattro);
4949
}
50+
static PyObject* get_tp_setattr(PyObject* unused, PyObject* object) {
51+
return PyLong_FromVoidPtr(Py_TYPE(object)->tp_setattr);
52+
}
53+
static PyObject* get_tp_setattro(PyObject* unused, PyObject* object) {
54+
return PyLong_FromVoidPtr(Py_TYPE(object)->tp_setattro);
55+
}
5056
static PyObject* get_tp_descr_get(PyObject* unused, PyObject* object) {
5157
return PyLong_FromVoidPtr(Py_TYPE(object)->tp_descr_get);
5258
}
5359
""",
5460
tp_methods=
5561
'{"get_tp_attr", (PyCFunction)get_tp_attr, METH_O | METH_STATIC, ""},' +
5662
'{"get_tp_attro", (PyCFunction)get_tp_attro, METH_O | METH_STATIC, ""},' +
63+
'{"get_tp_setattr", (PyCFunction)get_tp_setattr, METH_O | METH_STATIC, ""},' +
64+
'{"get_tp_setattro", (PyCFunction)get_tp_setattro, METH_O | METH_STATIC, ""},' +
5765
'{"get_tp_descr_get", (PyCFunction)get_tp_descr_get, METH_O | METH_STATIC, ""}')
5866

5967

@@ -205,6 +213,45 @@ def test_setattr_wrapper():
205213
assert x.__setattr__("bar", 42) is None
206214

207215

216+
def test_setattr_vs_setattro_inheritance():
217+
TestSetAttrOInheritance = CPyExtType("TestSetAttrOInheritance",
218+
'''
219+
int testattr_set(PyObject* self, char* key, PyObject* value) {
220+
Py_XDECREF(((TestSetAttrOInheritanceObject*)self)->payload);
221+
if (value != NULL) {
222+
Py_INCREF(value);
223+
}
224+
((TestSetAttrOInheritanceObject*)self)->payload = value;
225+
return 0;
226+
}
227+
228+
PyObject* get_payload(PyObject *self) {
229+
PyObject* r = ((TestSetAttrOInheritanceObject*)self)->payload;
230+
if (r == NULL) Py_RETURN_NONE;
231+
Py_INCREF(r);
232+
return r;
233+
}
234+
''',
235+
cmembers='PyObject* payload;',
236+
tp_methods='{"get_payload", (PyCFunction)get_payload, METH_NOARGS, ""}',
237+
tp_setattr="testattr_set")
238+
239+
x = TestSetAttrOInheritance()
240+
x.foo = 42
241+
assert x.get_payload() == 42
242+
243+
class Managed(TestSetAttrOInheritance):
244+
pass
245+
246+
x = Managed()
247+
x.foo = 42 # calls slot_tp_setattro, which calls __setattr__, which wraps the object.tp_setattro
248+
assert x.get_payload() is None
249+
250+
assert SlotsGetter.get_tp_setattro(object()) == SlotsGetter.get_tp_setattro(Managed())
251+
assert SlotsGetter.get_tp_setattro(TestSetAttrOInheritance()) == 0
252+
assert SlotsGetter.get_tp_setattr(TestSetAttrOInheritance()) != 0
253+
254+
208255
def test_sq_ass_item_wrapper():
209256
TestSqAssItemWrapperReturn = CPyExtType("TestSqAssItemWrapperReturn",
210257
"static int myassitem(PyObject *self, Py_ssize_t i, PyObject *v) { return 0; }",

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/TpSlots.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -206,18 +206,18 @@ private interface TpSlotGetter {
206206
*/
207207
@FunctionalInterface
208208
private interface NativeWrapperFactory {
209-
PyProcsWrapper create(TpSlotManaged slot);
209+
TpSlotWrapper create(TpSlotManaged slot);
210210

211211
final class Unimplemented implements NativeWrapperFactory {
212212
@Override
213-
public PyProcsWrapper create(TpSlotManaged slot) {
213+
public TpSlotWrapper create(TpSlotManaged slot) {
214214
throw new IllegalStateException("TODO: " + slot);
215215
}
216216
}
217217

218218
record ShouldNotReach(String slotName) implements NativeWrapperFactory {
219219
@Override
220-
public PyProcsWrapper create(TpSlotManaged slot) {
220+
public TpSlotWrapper create(TpSlotManaged slot) {
221221
throw new IllegalStateException(String.format("Slot %s should never be assigned a managed slot value.", slotName));
222222
}
223223
}
@@ -406,7 +406,7 @@ public Object readFromNative(PythonAbstractNativeObject pythonClass) {
406406
return field;
407407
}
408408

409-
public PyProcsWrapper createNativeWrapper(TpSlotManaged slot) {
409+
public TpSlotWrapper createNativeWrapper(TpSlotManaged slot) {
410410
return nativeWrapperFactory.create(slot);
411411
}
412412

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/slots/TpSlot.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,11 @@ public static Object toNative(TpSlotMeta slotMeta, TpSlot slot, Object defaultVa
8888
// This returns PyProcsWrapper, which will, in its toNative message, register the
8989
// pointer in C API context, such that we can map back from a pointer that we get from C
9090
// to the PyProcsWrapper and from that to the slot instance again in TpSlots#fromNative
91-
return slotMeta.createNativeWrapper(managedSlot);
91+
assert PythonContext.get(null).ownsGil(); // without GIL: use AtomicReference & CAS
92+
if (managedSlot.slotWrapper == null) {
93+
managedSlot.slotWrapper = slotMeta.createNativeWrapper(managedSlot);
94+
}
95+
return managedSlot.slotWrapper;
9296
} else {
9397
throw CompilerDirectives.shouldNotReachHere("TpSlotWrapper should wrap only managed slots. Native slots should go directly to native unwrapped.");
9498
}
@@ -121,6 +125,7 @@ public static TpSlot fromNative(PythonContext ctx, Object ptr, InteropLibrary in
121125
* Marker base class for managed slots: either builtin slots or user defined Python slots.
122126
*/
123127
public abstract static sealed class TpSlotManaged extends TpSlot permits TpSlotBuiltin, TpSlotPython {
128+
private TpSlotWrapper slotWrapper;
124129
}
125130

126131
/**

‎scripts/slot_fuzzer.py

Lines changed: 34 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -169,14 +169,6 @@ def compile_ext(name):
169169
)
170170
'''
171171

172-
# language=C
173-
C_SOURCE_HEADER = '''
174-
#include <Python.h>
175-
176-
PyObject *global_stash1;
177-
PyObject *global_stash2;
178-
'''
179-
180172

181173
def write_all(filename, text):
182174
with open(filename, 'w+') as f:
@@ -212,6 +204,14 @@ def tp_decl(self, name_prefix):
212204

213205
NO_GROUP = 'top'
214206

207+
# language=C
208+
C_SOURCE_HEADER = '''
209+
#include <Python.h>
210+
211+
PyObject *global_stash1;
212+
PyObject *global_stash2;
213+
'''
214+
215215
SLOTS = [
216216
Slot('tp_as_number', 'nb_bool', 'int $name$(PyObject* self)', ['1', '0', None]),
217217
Slot('tp_as_sequence', 'sq_length', 'Py_ssize_t $name$(PyObject* self)', ['0', '1', '42', None]),
@@ -235,13 +235,14 @@ def tp_decl(self, name_prefix):
235235
global_stash1 = value;
236236
return 0;
237237
''']),
238-
Slot(NO_GROUP, 'tp_setattr', 'int $name$(PyObject* self, char *name, PyObject *value)', ['0', None,
239-
'''
240-
Py_IncRef(value);
241-
Py_XDECREF(global_stash1);
242-
global_stash1 = value;
243-
return 0;
244-
''']),
238+
# Disabled due to incompatibilities with Carlo Verre hack in some very specific corner cases
239+
# Slot(NO_GROUP, 'tp_setattr', 'int $name$(PyObject* self, char *name, PyObject *value)', ['0', None,
240+
# '''
241+
# Py_IncRef(value);
242+
# Py_XDECREF(global_stash1);
243+
# global_stash1 = value;
244+
# return 0;
245+
# ''']),
245246
Slot(NO_GROUP, 'tp_descr_get', 'PyObject* $name$(PyObject* self, PyObject* key, PyObject* type)', ['Py_RETURN_NONE', 'Py_NewRef(key)', None,
246247
'''
247248
if (global_stash2 == NULL) Py_RETURN_NONE;
@@ -257,26 +258,33 @@ def tp_decl(self, name_prefix):
257258
'''])
258259
]
259260

261+
PY_GLOBALS = '''
262+
global_dict1 = dict()
263+
global_descr_val = None
264+
'''
260265

261266
MAGIC = {
262267
'__bool__(self)': ['True', 'False', None],
263268
'__len__(self)': ['0', '1', '42', None],
264-
'__getattribute__(self,name)': ['name', '42', 'self._dict[name]', None],
265-
'__getattr__(self,name)': ['name+"abc"', 'False', 'self._dict[name]', None],
266-
'__get__(self,obj,objtype=None)': ['obj', 'True', 'self.descr_value', None],
267-
'__set__(self,obj,value)': ['self.descr_value = value\nreturn None', None],
269+
'__getattribute__(self,name)': ['name', '42', 'global_dict1[name]', None],
270+
'__getattr__(self,name)': ['name+"abc"', 'False', 'global_dict1[name]', None],
271+
'__get__(self,obj,objtype=None)': ['obj', 'True', 'global_descr_val', None],
272+
'__set__(self,obj,value)': [None,
273+
'''
274+
global global_descr_val
275+
global_descr_val = value
276+
return None
277+
'''],
268278
'__setattr__(self,name,value)': [None,
269279
'''
270-
if not self._dict:
271-
self._dict = dict()
272-
self._dict[name] = value
280+
global global_dict1 # not using self._dict, because attr lookup can be something funky...
281+
global_dict1[name] = value
273282
return None
274283
'''],
275284
'__delattr__(self,name,value)': [None,
276285
'''
277-
if not self._dict:
278-
self._dict = dict()
279-
del self._dict[name]
286+
global global_dict1
287+
del global_dict1[name]
280288
return None
281289
'''],
282290
}
@@ -446,7 +454,7 @@ def choose_random(l):
446454
classes = []
447455
test_module_name = f"test{test_case_idx}"
448456
c_source = C_SOURCE_HEADER
449-
py_source = SLOTS_TESTER
457+
py_source = SLOTS_TESTER + PY_GLOBALS
450458
native_classes = []
451459
for i in range(classes_count):
452460
base = choose_random(classes) if classes else 'object'

0 commit comments

Comments
 (0)