Skip to content

Commit c4ba335

Browse files
msimacekmsimacek
committed
[GR-59058] PyType_Modified should not modify native slots
PullRequest: graalpython/3567
2 parents 2846b58 + 246574b commit c4ba335

File tree

5 files changed

+103
-71
lines changed

5 files changed

+103
-71
lines changed

‎graalpython/com.oracle.graal.python.cext/src/typeobject.c

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -303,17 +303,50 @@ _PyTypes_Fini(PyInterpreterState *interp)
303303
clear_slotdefs();
304304
}
305305
}
306-
#endif // GraalPy change
307306

308307

309308
void
310309
PyType_Modified(PyTypeObject *type)
311310
{
312-
// GraalPy change: different implementation
313-
GraalPyTruffle_Type_Modified(type, type->tp_name, type->tp_mro);
311+
/* Invalidate any cached data for the specified type and all
312+
subclasses. This function is called after the base
313+
classes, mro, or attributes of the type are altered.
314+
315+
Invariants:
316+
317+
- before Py_TPFLAGS_VALID_VERSION_TAG can be set on a type,
318+
it must first be set on all super types.
319+
320+
This function clears the Py_TPFLAGS_VALID_VERSION_TAG of a
321+
type (so it must first clear it on all subclasses). The
322+
tp_version_tag value is meaningless unless this flag is set.
323+
We don't assign new version tags eagerly, but only as
324+
needed.
325+
*/
326+
if (!_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) {
327+
return;
328+
}
329+
330+
PyObject *subclasses = type->tp_subclasses;
331+
if (subclasses != NULL) {
332+
assert(PyDict_CheckExact(subclasses));
333+
334+
Py_ssize_t i = 0;
335+
PyObject *ref;
336+
while (PyDict_Next(subclasses, &i, NULL, &ref)) {
337+
assert(PyWeakref_CheckRef(ref));
338+
PyObject *obj = PyWeakref_GET_OBJECT(ref);
339+
if (obj == Py_None) {
340+
continue;
341+
}
342+
PyType_Modified(_PyType_CAST(obj));
343+
}
344+
}
345+
346+
type->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG;
347+
type->tp_version_tag = 0; /* 0 is not a valid version tag */
314348
}
315349

316-
#if 0 // GraalPy change
317350
static void
318351
type_mro_modified(PyTypeObject *type, PyObject *bases) {
319352
/*
@@ -6555,9 +6588,9 @@ PyType_Ready(PyTypeObject *type)
65556588
type->tp_flags |= Py_TPFLAGS_IMMUTABLETYPE;
65566589
}
65576590

6558-
/* GraalPy change: Types are often just static mem; so register them to be able to rule out invalid accesses. */
6591+
// GraalPy change
65596592
if (PyTruffle_Trace_Memory()) {
6560-
GraalPyTruffle_Trace_Type(type, type->tp_name != NULL);
6593+
GraalPyTruffle_Trace_Type(type);
65616594
}
65626595

65636596
/* GraalPy change: IMPORTANT: This is a Truffle-specific statement. Since the refcnt for the type is currently 0 and
@@ -6575,8 +6608,8 @@ PyType_Ready(PyTypeObject *type)
65756608
type->tp_flags = (type->tp_flags & ~Py_TPFLAGS_READYING) | Py_TPFLAGS_READY;
65766609
assert(_PyType_CheckConsistency(type));
65776610

6578-
// GraalPy change: it may be that the type was used uninitialized
6579-
GraalPyTruffle_Type_Modified(type, type->tp_name, NULL);
6611+
// GraalPy change
6612+
GraalPyTruffle_InitializeOldStyleSlots(type);
65806613

65816614
// GraalPy change: for reason, see first call to Py_INCREF in this function
65826615
Py_DECREF(type);

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

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -598,4 +598,32 @@ def __get__(self,obj,objtype=None): return "dummy"
598598
def __setattr__(self,name,value): return None
599599

600600
assert 3 * Managed1() == 3
601-
assert Managed1() * 3 == "__mul__result: 3"
601+
assert Managed1() * 3 == "__mul__result: 3"
602+
603+
604+
def test_PyType_Modified_doesnt_change_slots():
605+
TypeWithSqItemAndMpSubscr = CPyExtType(
606+
"TypeWithSqItemAndMpSubscr",
607+
code='''
608+
static PyObject* sq_item(PyObject* self, Py_ssize_t index) {
609+
return PyUnicode_FromString("sq_item");
610+
}
611+
static PyObject* mp_subscript(PyObject* self, PyObject* index) {
612+
return PyUnicode_FromString("mp_subscript");
613+
}
614+
static PyObject* call_PySequence_GetItem(PyObject* self, PyObject* index) {
615+
Py_ssize_t i = PyLong_AsSsize_t(index);
616+
if (i == -1 && PyErr_Occurred())
617+
return NULL;
618+
return PySequence_GetItem(self, i);
619+
}
620+
''',
621+
sq_item="sq_item",
622+
mp_subscript="mp_subscript",
623+
tp_methods='{"call_PySequence_GetItem", (PyCFunction)call_PySequence_GetItem, METH_O, ""}',
624+
post_ready_code="PyType_Modified(&TypeWithSqItemAndMpSubscrType);"
625+
)
626+
tester = TypeWithSqItemAndMpSubscr()
627+
assert tester[1] == 'mp_subscript'
628+
assert tester.call_PySequence_GetItem(1) == 'sq_item'
629+

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextTypeBuiltins.java

Lines changed: 32 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -51,19 +51,18 @@
5151
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyTypeObject;
5252
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.Py_ssize_t;
5353
import static com.oracle.graal.python.builtins.objects.cext.common.CExtContext.METH_CLASS;
54+
import static com.oracle.graal.python.builtins.objects.cext.structs.CFields.PyTypeObject__tp_name;
5455
import static com.oracle.graal.python.nodes.HiddenAttr.METHOD_DEF_PTR;
5556
import static com.oracle.graal.python.nodes.SpecialAttributeNames.T___DOC__;
5657
import static com.oracle.graal.python.nodes.SpecialAttributeNames.T___NAME__;
5758
import static com.oracle.graal.python.util.PythonUtils.EMPTY_OBJECT_ARRAY;
58-
import static com.oracle.graal.python.util.PythonUtils.TS_ENCODING;
5959

6060
import com.oracle.graal.python.PythonLanguage;
6161
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApi7BuiltinNode;
6262
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApi8BuiltinNode;
6363
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApiBinaryBuiltinNode;
6464
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApiBuiltin;
6565
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApiCallPath;
66-
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApiTernaryBuiltinNode;
6766
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.CApiUnaryBuiltinNode;
6867
import com.oracle.graal.python.builtins.modules.cext.PythonCextBuiltins.PyObjectSetAttrNode;
6968
import com.oracle.graal.python.builtins.objects.PNone;
@@ -78,6 +77,7 @@
7877
import com.oracle.graal.python.builtins.objects.cext.capi.ExternalFunctionNodes.GetterRoot;
7978
import com.oracle.graal.python.builtins.objects.cext.capi.ExternalFunctionNodes.PExternalFunctionWrapper;
8079
import com.oracle.graal.python.builtins.objects.cext.capi.ExternalFunctionNodes.SetterRoot;
80+
import com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor;
8181
import com.oracle.graal.python.builtins.objects.cext.common.CArrayWrappers.CArrayWrapper;
8282
import com.oracle.graal.python.builtins.objects.cext.common.CExtCommonNodes.EnsureExecutableNode;
8383
import com.oracle.graal.python.builtins.objects.cext.common.CExtContext;
@@ -89,7 +89,6 @@
8989
import com.oracle.graal.python.builtins.objects.function.PBuiltinFunction;
9090
import com.oracle.graal.python.builtins.objects.getsetdescriptor.GetSetDescriptor;
9191
import com.oracle.graal.python.builtins.objects.object.PythonObject;
92-
import com.oracle.graal.python.builtins.objects.tuple.PTuple;
9392
import com.oracle.graal.python.builtins.objects.type.PythonAbstractClass;
9493
import com.oracle.graal.python.builtins.objects.type.PythonManagedClass;
9594
import com.oracle.graal.python.builtins.objects.type.SpecialMethodSlot;
@@ -103,9 +102,9 @@
103102
import com.oracle.graal.python.nodes.attributes.WriteAttributeToPythonObjectNode;
104103
import com.oracle.graal.python.nodes.object.GetDictIfExistsNode;
105104
import com.oracle.graal.python.nodes.util.CastToTruffleStringNode;
105+
import com.oracle.graal.python.runtime.PythonContext;
106106
import com.oracle.graal.python.runtime.object.PythonObjectFactory;
107107
import com.oracle.graal.python.runtime.sequence.storage.MroSequenceStorage;
108-
import com.oracle.graal.python.runtime.sequence.storage.SequenceStorage;
109108
import com.oracle.graal.python.util.Function;
110109
import com.oracle.graal.python.util.PythonUtils;
111110
import com.oracle.truffle.api.CompilerDirectives;
@@ -120,14 +119,12 @@
120119
import com.oracle.truffle.api.dsl.ImportStatic;
121120
import com.oracle.truffle.api.dsl.Specialization;
122121
import com.oracle.truffle.api.interop.InteropLibrary;
123-
import com.oracle.truffle.api.interop.UnsupportedMessageException;
124122
import com.oracle.truffle.api.library.CachedLibrary;
125123
import com.oracle.truffle.api.nodes.Node;
126124
import com.oracle.truffle.api.nodes.RootNode;
127125
import com.oracle.truffle.api.object.DynamicObject;
128126
import com.oracle.truffle.api.object.DynamicObjectLibrary;
129127
import com.oracle.truffle.api.object.Shape;
130-
import com.oracle.truffle.api.profiles.InlinedExactClassProfile;
131128
import com.oracle.truffle.api.strings.TruffleString;
132129
import com.oracle.truffle.api.utilities.CyclicAssumption;
133130

@@ -217,73 +214,48 @@ static PDict doGeneric(PythonNativeClass nativeClass) {
217214
}
218215
}
219216

220-
@CApiBuiltin(ret = Int, args = {PyTypeObject, ConstCharPtrAsTruffleString, PyObject}, call = Ignored)
221-
abstract static class PyTruffle_Type_Modified extends CApiTernaryBuiltinNode {
217+
@CApiBuiltin(ret = ArgDescriptor.Void, args = {PyTypeObject}, call = Ignored)
218+
abstract static class PyTruffle_InitializeOldStyleSlots extends CApiUnaryBuiltinNode {
222219

223220
@TruffleBoundary
224-
@Specialization(guards = "isNoValue(mroTuple)")
225-
int doIt(PythonNativeClass clazz, TruffleString name, @SuppressWarnings("unused") PNone mroTuple) {
226-
CyclicAssumption nativeClassStableAssumption = getContext().getNativeClassStableAssumption(clazz, false);
227-
if (nativeClassStableAssumption != null) {
228-
nativeClassStableAssumption.invalidate("PyType_Modified(\"" + name.toJavaStringUncached() + "\") (without MRO) called");
229-
}
230-
SpecialMethodSlot.reinitializeSpecialMethodSlots(PythonNativeClass.cast(clazz), getLanguage());
231-
// TODO: this is called from two places: at the end of PyType_Ready, and theoretically
232-
// could be called from:
233-
//
234-
// void PyType_Modified(PyTypeObject* type) -> GraalPyTruffle_Type_Modified(type,
235-
// type->tp_name, type->tp_mro);
236-
//
237-
// in unlikely (impossible?) case that type->tp_mro was NULL. Should we distinguish
238-
// the two cases? As a cleanup if it is impossible situation (separate two different
239-
// upcalls), or because at the end of PyType_Ready, we do not want to call
240-
// TpSlots.updateAllSlots(clazz), but from PyType_Modified we do.
241-
return 0;
221+
@Specialization
222+
static Object doIt(PythonAbstractNativeObject clazz,
223+
@Bind("this") Node inliningTarget) {
224+
SpecialMethodSlot.reinitializeSpecialMethodSlots(clazz, PythonLanguage.get(inliningTarget));
225+
return PNone.NO_VALUE;
242226
}
227+
}
228+
229+
@CApiBuiltin(ret = ArgDescriptor.Void, args = {PyTypeObject}, call = Direct)
230+
abstract static class PyType_Modified extends CApiUnaryBuiltinNode {
243231

244232
@TruffleBoundary
245233
@Specialization
246-
int doIt(PythonNativeClass clazz, TruffleString name, PTuple mroTuple,
247-
@Bind("this") Node inliningTarget,
248-
@Cached InlinedExactClassProfile profile) {
249-
CyclicAssumption nativeClassStableAssumption = getContext().getNativeClassStableAssumption(clazz, false);
234+
static Object doIt(PythonAbstractNativeObject clazz,
235+
@Bind("this") Node inliningTarget) {
236+
PythonContext context = PythonContext.get(inliningTarget);
237+
CyclicAssumption nativeClassStableAssumption = context.getNativeClassStableAssumption(clazz, false);
250238
if (nativeClassStableAssumption != null) {
251-
nativeClassStableAssumption.invalidate("PyType_Modified(\"" + name.toJavaStringUncached() + "\") called");
239+
nativeClassStableAssumption.invalidate("PyType_Modified(\"" + TypeNodes.GetNameNode.executeUncached(clazz).toJavaStringUncached() + "\") called");
252240
}
253-
SequenceStorage sequenceStorage = profile.profile(inliningTarget, mroTuple.getSequenceStorage());
254-
if (sequenceStorage instanceof MroSequenceStorage) {
255-
((MroSequenceStorage) sequenceStorage).lookupChanged();
256-
} else {
257-
CompilerDirectives.transferToInterpreterAndInvalidate();
258-
throw new IllegalStateException("invalid MRO object for native type \"" + name.toJavaStringUncached() + "\"");
259-
}
260-
SpecialMethodSlot.reinitializeSpecialMethodSlots(PythonNativeClass.cast(clazz), getLanguage());
261-
TpSlots.updateAllSlots(clazz);
262-
return 0;
241+
MroSequenceStorage mroStorage = TypeNodes.GetMroStorageNode.executeUncached(clazz);
242+
mroStorage.lookupChanged();
243+
// Reload slots from native, which also invalidates cached slot lookups
244+
clazz.setTpSlots(TpSlots.fromNative(clazz, context));
245+
return PNone.NO_VALUE;
263246
}
264247
}
265248

266-
@CApiBuiltin(ret = Int, args = {Pointer, Pointer}, call = Ignored)
267-
abstract static class PyTruffle_Trace_Type extends CApiBinaryBuiltinNode {
249+
@CApiBuiltin(ret = Int, args = {Pointer}, call = Ignored)
250+
abstract static class PyTruffle_Trace_Type extends CApiUnaryBuiltinNode {
268251
private static final TruffleLogger LOGGER = CApiContext.getLogger(PyTruffle_Trace_Type.class);
269252

270-
@Specialization(limit = "3")
271-
int trace(Object ptr, Object classNameObj,
272-
@CachedLibrary("ptr") InteropLibrary ptrLib,
273-
@CachedLibrary("classNameObj") InteropLibrary nameLib,
274-
@Cached TruffleString.SwitchEncodingNode switchEncodingNode) {
275-
final TruffleString className;
276-
if (nameLib.isString(classNameObj)) {
277-
try {
278-
className = switchEncodingNode.execute(nameLib.asTruffleString(classNameObj), TS_ENCODING);
279-
} catch (UnsupportedMessageException e) {
280-
throw CompilerDirectives.shouldNotReachHere(e);
281-
}
282-
} else {
283-
className = null;
284-
}
285-
Object primitivePtr = CApiContext.asPointer(ptr, ptrLib);
286-
LOGGER.fine(() -> PythonUtils.formatJString("Initializing native type %s (ptr = %s)", className, CApiContext.asHex(primitivePtr)));
253+
@Specialization
254+
@TruffleBoundary
255+
int trace(Object ptr) {
256+
LOGGER.fine(() -> PythonUtils.formatJString("Initializing native type %s (ptr = %s)",
257+
CStructAccess.ReadCharPtrNode.getUncached().read(ptr, PyTypeObject__tp_name),
258+
CApiContext.asHex(CApiContext.asPointer(ptr, InteropLibrary.getUncached()))));
287259
return 0;
288260
}
289261
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/CApiFunction.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -482,7 +482,6 @@ public final class CApiFunction {
482482
@CApiBuiltin(name = "PyType_GetQualName", ret = PyObject, args = {PyTypeObject}, call = CImpl)
483483
@CApiBuiltin(name = "PyType_GetSlot", ret = Pointer, args = {PyTypeObject, Int}, call = CImpl)
484484
@CApiBuiltin(name = "PyType_IsSubtype", ret = Int, args = {PyTypeObject, PyTypeObject}, call = CImpl)
485-
@CApiBuiltin(name = "PyType_Modified", ret = Void, args = {PyTypeObject}, call = CImpl)
486485
@CApiBuiltin(name = "PyType_Ready", ret = Int, args = {PyTypeObject}, call = CImpl)
487486
@CApiBuiltin(name = "PyUnicode_Append", ret = Void, args = {PyObjectPtr, PyObject}, call = CImpl)
488487
@CApiBuiltin(name = "PyUnicode_AppendAndDel", ret = Void, args = {PyObjectPtr, PyObject}, call = CImpl)

‎mx.graalpython/mx_graalpython.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -847,7 +847,7 @@ def graalpytest(args):
847847
python_args = []
848848
runner_args = []
849849
for arg in unknown_args:
850-
if arg.startswith(('--python.', '--engine.', '--llvm.', '--vm.', '--inspect', '--experimental-options')):
850+
if arg.startswith(('--python.', '--engine.', '--llvm.', '--vm.', '--inspect', '--log.', '--experimental-options')):
851851
python_args.append(arg)
852852
else:
853853
runner_args.append(arg)

0 commit comments

Comments
 (0)