GH-132042: Fix calculation of slotdef index in update_one_slot

[sergey-miryanov]

There is a fix for calculation of the slotdef index.
Thanks @eendebakpt

• Issue: Optimize class creation #132042

[sergey-miryanov]

On the first sight we should not face this error. I added extra checks and, it seems that the existing code is handling the situation well.

Extra checks:

            size_t index_wrong = (p - slotdefs) / sizeof(slotdefs[0]);
            size_t index = (p - slotdefs);
            tptr = slotptr(type, p->offset);
            if (slotdefs_name_counts[index_wrong] != 1 && slotdefs_name_counts[index] == 1) {
                if (tptr != NULL && tptr != ptr) {
                    printf("update_one_slot: %s, wrong idx=%d, idx=%d, wrong count=%d, count=%d\n",
                        p->name, (int)index_wrong,(int) index, slotdefs_name_counts[index_wrong], slotdefs_name_counts[index]
                    );
                }
            }

I didn't see this message in the logs while Python was compiling and freezing.
I ran some extra tests, and they're still running. The results should be ready in about an hour.

@vstinner Could you please take a look?

[sergey-miryanov]

Tests passed, no message in the logs.

[vstinner]

PR merged, thanks for the fix. Thanks @eendebakpt for the bug report!

I tried different ways to check if the previous code had a bug, but I failed to find a case where the change makes any difference. We were just very lucky, the bug had no effects!

[vstinner]

I wrote a test on slots. I leave it here, I don't know if it's useful, so I don't propose a PR.

diff --git a/Lib/test/test_capi/test_type.py b/Lib/test/test_capi/test_type.py
index e6a8ef9eed6..3b6fcb5e12a 100644
--- a/Lib/test/test_capi/test_type.py
+++ b/Lib/test/test_capi/test_type.py
@@ -299,3 +299,20 @@ def test_extension_managed_weakref_nogc_type(self):
                "flag but not Py_TPFLAGS_HAVE_GC flag")
         with self.assertRaisesRegex(SystemError, msg):
             _testcapi.create_managed_weakref_nogc_type()
+
+    def test_type_slots_optim(self):
+        check_type_slots_optim = _testcapi.check_type_slots_optim
+
+        class UserType:
+            __hash__ = None
+            def __eq__(self, other):
+                return True
+            def __ne__(self, other):
+                return False
+            def __or__(self, other):
+                return 123
+
+        class SubClass(UserType):
+            pass
+
+        check_type_slots_optim(UserType, SubClass)
diff --git a/Modules/_testcapi/type.c b/Modules/_testcapi/type.c
index 9bef58d1f83..cbb7d45a35c 100644
--- a/Modules/_testcapi/type.c
+++ b/Modules/_testcapi/type.c
@@ -227,6 +227,43 @@ type_freeze(PyObject *module, PyObject *arg)
 }
 
 
+static PyObject *
+check_type_slots_optim(PyObject *module, PyObject *args)
+{
+    PyTypeObject *user_type, *subclass;
+    if (!PyArg_ParseTuple(args, "O!O!",
+                          &PyType_Type, &user_type,
+                          &PyType_Type, &subclass)) {
+        return NULL;
+    }
+
+    // Methods inherited from object
+    PyTypeObject *object_type = &PyBaseObject_Type;
+    assert(user_type->tp_new == object_type->tp_new);
+    assert(user_type->tp_init == object_type->tp_init);
+    assert(user_type->tp_repr == object_type->tp_repr);
+    assert(user_type->tp_str == object_type->tp_str);
+
+    // Methods not defined
+    assert(user_type->tp_call == NULL);
+    assert(user_type->tp_as_number->nb_and == NULL);
+
+    // Methods implemented in Python
+    assert(user_type->tp_richcompare != NULL);
+    assert(user_type->tp_as_number->nb_or != NULL);
+
+    // __hash__ = None
+    assert(user_type->tp_hash == PyObject_HashNotImplemented);
+
+    // Test subclass
+    assert(subclass->tp_richcompare == user_type->tp_richcompare);
+    assert(subclass->tp_as_number->nb_or == user_type->tp_as_number->nb_or);
+    assert(subclass->tp_hash == user_type->tp_hash);
+
+    Py_RETURN_NONE;
+}
+
+
 static PyMethodDef test_methods[] = {
     {"get_heaptype_for_name", get_heaptype_for_name, METH_NOARGS},
     {"get_type_name", get_type_name, METH_O},
@@ -241,6 +278,7 @@ static PyMethodDef test_methods[] = {
     {"type_get_tp_bases", type_get_tp_bases, METH_O},
     {"type_get_tp_mro", type_get_tp_mro, METH_O},
     {"type_freeze", type_freeze, METH_O},
+    {"check_type_slots_optim", check_type_slots_optim, METH_VARARGS},
     {NULL},
 };
 

[miss-islington-app]

Thanks @sergey-miryanov for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @sergey-miryanov and @vstinner, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker f105265538823126dda5bc113fdb72cb2d5d2dbc 3.14

[sergey-miryanov]

I was sure it was implemented in 3.14. Sorry.