Make the cpdef override check work with Python subclasses of extension types...

Make the cpdef override check work with Python subclasses of extension types again that do not initialise their own dict.
This lead to a crash with dict versioning in Py3.6+.
Closes GH-2823.

CHANGES.rst

@@ -5,6 +5,9 @@ Cython Changelog
 0.29.5 (2019-??-??)
 ===================
 
+* Crash when defining a Python subclass of an extension type and repeatedly calling
+  a cpdef method on it.  (Github issue #2823)
+
 * Compiler crash when ``prange()`` loops appear inside of with-statements.
   (Github issue #2780)
 

Cython/Compiler/Naming.py

@@ -117,6 +117,9 @@ frame_code_cname = pyrex_prefix + "frame_code"
 binding_cfunc    = pyrex_prefix + "binding_PyCFunctionType"
 fused_func_prefix = pyrex_prefix + 'fuse_'
 quick_temp_cname = pyrex_prefix + "temp" # temp variable for quick'n'dirty temping
+tp_dict_version_temp = pyrex_prefix + "tp_dict_version"
+obj_dict_version_temp = pyrex_prefix + "obj_dict_version"
+type_dict_guard_temp = pyrex_prefix + "type_dict_guard"
 cython_runtime_cname   = pyrex_prefix + "cython_runtime"
 
 global_code_object_cache_find = pyrex_prefix + 'find_code_object'

Cython/Compiler/Nodes.py

@@ -4359,17 +4359,12 @@ class OverrideCheckNode(StatNode):
         # This would allow checking the dict versions around _PyType_Lookup() if it returns a descriptor,
         # and would (tada!) make this check a pure type based thing instead of supporting only a single
         # instance at a time.
-        code.putln("static PY_UINT64_T tp_dict_version = 0, obj_dict_version = 0;")
-        code.putln("if (likely("
-                   "Py_TYPE(%s)->tp_dict && "
-                   "tp_dict_version == __PYX_GET_DICT_VERSION(Py_TYPE(%s)->tp_dict) && "
-                   "(!Py_TYPE(%s)->tp_dictoffset || "
-                   "obj_dict_version == __PYX_GET_DICT_VERSION(_PyObject_GetDictPtr(%s)))"
-                   "));" % (
-            self_arg, self_arg, self_arg, self_arg))
-        code.putln("else {")
-        code.putln("PY_UINT64_T type_dict_guard = (likely(Py_TYPE(%s)->tp_dict)) ? __PYX_GET_DICT_VERSION(Py_TYPE(%s)->tp_dict) : 0;" % (
-            self_arg, self_arg))
+        code.putln("static PY_UINT64_T %s = __PYX_DICT_VERSION_INIT, %s = __PYX_DICT_VERSION_INIT;" % (
+            Naming.tp_dict_version_temp, Naming.obj_dict_version_temp))
+        code.putln("if (unlikely(!__Pyx_object_dict_version_matches(%s, %s, %s))) {" % (
+            self_arg, Naming.tp_dict_version_temp, Naming.obj_dict_version_temp))
+        code.putln("PY_UINT64_T %s = __Pyx_get_tp_dict_version(%s);" % (
+            Naming.type_dict_guard_temp, self_arg))
         code.putln("#endif")
 
         func_node_temp = code.funcstate.allocate_temp(py_object_type, manage_ref=True)
@@ -4393,19 +4388,19 @@ class OverrideCheckNode(StatNode):
         # but it is very unlikely that the versions change during lookup, and the type dict safe guard
         # should increase the chance of detecting such a case.
         code.putln("#if CYTHON_USE_DICT_VERSIONS && CYTHON_USE_PYTYPE_LOOKUP")
-        code.putln("tp_dict_version = likely(Py_TYPE(%s)->tp_dict) ?"
-                   " __PYX_GET_DICT_VERSION(Py_TYPE(%s)->tp_dict) : 0;" % (
-            self_arg, self_arg))
-        code.putln("obj_dict_version = likely(Py_TYPE(%s)->tp_dictoffset) ?"
-                   " __PYX_GET_DICT_VERSION(_PyObject_GetDictPtr(%s)) : 0;" % (
-            self_arg, self_arg))
+        code.putln("%s = __Pyx_get_tp_dict_version(%s);" % (
+            Naming.tp_dict_version_temp, self_arg))
+        code.putln("%s = __Pyx_get_object_dict_version(%s);" % (
+            Naming.obj_dict_version_temp, self_arg))
         # Safety check that the type dict didn't change during the lookup.  Since CPython looks up the
         # attribute (descriptor) first in the type dict and then in the instance dict or through the
         # descriptor, the only really far-away lookup when we get here is one in the type dict. So we
         # double check the type dict version before and afterwards to guard against later changes of
         # the type dict during the lookup process.
-        code.putln("if (unlikely(type_dict_guard != tp_dict_version)) {")
-        code.putln("tp_dict_version = obj_dict_version = 0;")
+        code.putln("if (unlikely(%s != %s)) {" % (
+            Naming.type_dict_guard_temp, Naming.tp_dict_version_temp))
+        code.putln("%s = %s = __PYX_DICT_VERSION_INIT;" % (
+            Naming.tp_dict_version_temp, Naming.obj_dict_version_temp))
         code.putln("}")
         code.putln("#endif")
 

Cython/Utility/ModuleSetupCode.c

@@ -429,25 +429,51 @@ class __Pyx_FakeReference {
 #endif
 
 #if CYTHON_USE_DICT_VERSIONS
+
+#define __PYX_DICT_VERSION_INIT  ((PY_UINT64_T) -1)
 #define __PYX_GET_DICT_VERSION(dict)  (((PyDictObject*)(dict))->ma_version_tag)
 #define __PYX_UPDATE_DICT_CACHE(dict, value, cache_var, version_var) \
     (version_var) = __PYX_GET_DICT_VERSION(dict); \
     (cache_var) = (value);
-#define __PYX_PY_DICT_LOOKUP_IF_MODIFIED(VAR, DICT, LOOKUP) { \
-        static PY_UINT64_T __pyx_dict_version = 0; \
-        static PyObject *__pyx_dict_cached_value = NULL; \
-        if (likely(__PYX_GET_DICT_VERSION(DICT) == __pyx_dict_version)) { \
-            (VAR) = __pyx_dict_cached_value; \
-        } else { \
-            (VAR) = __pyx_dict_cached_value = (LOOKUP); \
-            __pyx_dict_version = __PYX_GET_DICT_VERSION(DICT); \
-        } \
+
+static CYTHON_INLINE PY_UINT64_T __Pyx_get_tp_dict_version(PyObject *obj) {
+    PyObject *dict = Py_TYPE(obj)->tp_dict;
+    return dict ? __PYX_GET_DICT_VERSION(dict) : 0;
+}
+
+static CYTHON_INLINE PY_UINT64_T __Pyx_get_object_dict_version(PyObject *obj) {
+    PyObject **dictptr = NULL;
+    Py_ssize_t offset = Py_TYPE(obj)->tp_dictoffset;
+    if (offset) {
+        dictptr = (offset > 0) ? (PyObject **) ((char *)obj + offset) : _PyObject_GetDictPtr(obj);
     }
+    return (dictptr && *dictptr) ? __PYX_GET_DICT_VERSION(*dictptr) : 0;
+}
+
+#define __PYX_PY_DICT_LOOKUP_IF_MODIFIED(VAR, DICT, LOOKUP) { \
+    static PY_UINT64_T __pyx_dict_version = 0; \
+    static PyObject *__pyx_dict_cached_value = NULL; \
+    if (likely(__PYX_GET_DICT_VERSION(DICT) == __pyx_dict_version)) { \
+        (VAR) = __pyx_dict_cached_value; \
+    } else { \
+        (VAR) = __pyx_dict_cached_value = (LOOKUP); \
+        __pyx_dict_version = __PYX_GET_DICT_VERSION(DICT); \
+    } \
+}
+
+static CYTHON_INLINE int __Pyx_object_dict_version_matches(PyObject* obj, PY_UINT64_T tp_dict_version, PY_UINT64_T obj_dict_version) {
+    PyObject *dict = Py_TYPE(obj)->tp_dict;
+    if (!dict || tp_dict_version != __PYX_GET_DICT_VERSION(dict))
+        return 0;
+    return obj_dict_version == __Pyx_get_object_dict_version(obj);
+}
+
 #else
 #define __PYX_GET_DICT_VERSION(dict)  (0)
 #define __PYX_UPDATE_DICT_CACHE(dict, value, cache_var, version_var)
 #define __PYX_PY_DICT_LOOKUP_IF_MODIFIED(VAR, DICT, LOOKUP)  (VAR) = (LOOKUP);
 #endif
+// endif: CYTHON_USE_DICT_VERSIONS
 
 #if CYTHON_COMPILING_IN_PYPY && !defined(PyObject_Malloc)
   #define PyObject_Malloc(s)   PyMem_Malloc(s)

tests/run/cpdef_method_override.pyx

@@ -2,10 +2,34 @@
 # tag: cpdef
 # ticket: gh-1771
 
+def _call_method(cls):
+    obj = cls()
+    obj.callmeth()
+    obj = cls()
+    obj.callmeth()
+    obj.callmeth()
+    obj = cls()
+    obj.callmeth()
+    obj.callmeth()
+    obj.callmeth()
+
+
 cdef class BaseType:
     """
     >>> BaseType().callmeth()
     BaseType.meth
+    >>> obj = BaseType()
+    >>> obj.callmeth()
+    BaseType.meth
+    >>> obj.callmeth()
+    BaseType.meth
+    >>> _call_method(BaseType)
+    BaseType.meth
+    BaseType.meth
+    BaseType.meth
+    BaseType.meth
+    BaseType.meth
+    BaseType.meth
     """
     def callmeth(self):
         return self.meth()
@@ -17,6 +41,20 @@ class PyClass(BaseType):
     """
     >>> PyClass().callmeth()
     PyClass.meth
+    >>> obj = PyClass()
+    >>> obj.callmeth()
+    PyClass.meth
+    >>> obj.callmeth()
+    PyClass.meth
+    >>> obj.callmeth()
+    PyClass.meth
+    >>> _call_method(PyClass)
+    PyClass.meth
+    PyClass.meth
+    PyClass.meth
+    PyClass.meth
+    PyClass.meth
+    PyClass.meth
     """
     def meth(self):
         print("PyClass.meth")
@@ -26,6 +64,20 @@ class PySlotsClass(BaseType):
     """
     >>> PySlotsClass().callmeth()
     PySlotsClass.meth
+    >>> obj = PySlotsClass()
+    >>> obj.callmeth()
+    PySlotsClass.meth
+    >>> obj.callmeth()
+    PySlotsClass.meth
+    >>> obj.callmeth()
+    PySlotsClass.meth
+    >>> _call_method(PySlotsClass)
+    PySlotsClass.meth
+    PySlotsClass.meth
+    PySlotsClass.meth
+    PySlotsClass.meth
+    PySlotsClass.meth
+    PySlotsClass.meth
     """
     __slots__ = []