Add nb_add/sq_concat to slots fuzzer, fix issue found by the fuzzer

Author: steve-s

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

@@ -65,6 +65,9 @@
                          static PyObject* get_tp_descr_get(PyObject* unused, PyObject* object) {
                              return PyLong_FromVoidPtr(Py_TYPE(object)->tp_descr_get);
                          }
+                         static PyObject* get_nb_add(PyObject* unused, PyObject* object) {
+                             return PyLong_FromVoidPtr(Py_TYPE(object)->tp_as_number == NULL ? NULL : Py_TYPE(object)->tp_as_number->nb_add);
+                         }
                          """,
                          tp_methods=
                          '{"get_tp_attr", (PyCFunction)get_tp_attr, METH_O | METH_STATIC, ""},' +
@@ -74,6 +77,7 @@
                          '{"get_nb_bool", (PyCFunction)get_nb_bool, METH_O | METH_STATIC, ""},' +
                          '{"get_tp_as_number", (PyCFunction)get_tp_as_number, METH_O | METH_STATIC, ""},' +
                          '{"get_sq_concat", (PyCFunction)get_sq_concat, METH_O | METH_STATIC, ""},' +
+                         '{"get_nb_add", (PyCFunction)get_nb_add, METH_O | METH_STATIC, ""},' +
                          '{"get_tp_descr_get", (PyCFunction)get_tp_descr_get, METH_O | METH_STATIC, ""}')
 
 
@@ -565,4 +569,27 @@ class ManagedSub(NbAddOnlyHeapType):
     class ManagedSub2(NbAddOnlyHeapType):
         def __add__(self, other): return NotImplemented
 
-    assert SlotsGetter.get_sq_concat(ManagedSub2()) == 0
+    assert SlotsGetter.get_sq_concat(ManagedSub2()) == 0
+
+
+def test_nb_add_sq_concat_static_managed_heap_inheritance():
+    NbAddSqConcatStaticType = CPyExtType("NbAddSqConcatStaticType",
+                                       code = 'PyObject* my_nb_add(PyObject* self, PyObject *other) { return PyLong_FromLong(42); }',
+                                       nb_add = 'my_nb_add',
+                                     sq_concat = 'my_nb_add')
+
+    # fixup_slot_dispatchers should figure out that __add__ and __radd__ descriptors wrap the same
+    # native call and set nb_add to it instead of the Python dispatcher C function
+    class IntermediateManagedDummy(NbAddSqConcatStaticType):
+        pass
+
+    # This should inherit my_nb_add as nb_add
+    SqConcatInheritingNbAdd = CPyExtHeapType("SqConcatInheritingNbAdd",
+                                             bases=(IntermediateManagedDummy,),
+                                             code = 'PyObject* my_sq_concat(PyObject* self, PyObject *other) { return PyLong_FromLong(10); }',
+                                             slots=['{Py_sq_concat, &my_sq_concat}'])
+
+    # Doing SqConcatInheritingNbAdd() + NbAddSqConcatStaticType() triggers segfault in CPython debug build
+    # (not in the addition itself, but later in weakref processing). This test is derived from a fuzzer test
+    # case, where it did not segfault...
+    assert SlotsGetter.get_nb_add(NbAddSqConcatStaticType()) == SlotsGetter.get_nb_add(SqConcatInheritingNbAdd())

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

@@ -223,9 +223,19 @@ public static TpSlotNative createHPySlot(Object callable) {
         }
 
         public final boolean isSameCallable(TpSlotNative other, InteropLibrary interop) {
-            // Interop is going to be quite slow (in interpreter)
-            // What else? Compare asPointer() values?
-            return this == other || this.callable == other.callable || interop.isIdentical(callable, other.callable, interop);
+            if (this == other || this.callable == other.callable) {
+                return true;
+            }
+            // NFISymbols do not implement isIdentical interop message, so we compare the pointers
+            // Interop is going to be quite slow (in interpreter), should we eagerly request the
+            // pointer in the ctor?
+            interop.toNative(callable);
+            interop.toNative(other.callable);
+            try {
+                return interop.asPointer(callable) == interop.asPointer(other.callable);
+            } catch (UnsupportedMessageException e) {
+                throw CompilerDirectives.shouldNotReachHere(e);
+            }
         }
 
         /**

scripts/slot_fuzzer.py

@@ -65,7 +65,7 @@
 import sys
 import re
 import traceback
-def slots_tester(Klass):
+def slots_tester(Klass, other_klasses):
     def test(fun, name):
         try:
             print(f'{name}:', end='')
@@ -148,6 +148,11 @@ def del_descr(self): del self.descr
     test_dunder(obj, '__getitem__', -1)
     test_dunder(obj, '__getitem__', 'hello')
 
+    other_objs = [K() for K in other_klasses] + [42, 3.14, 'string', (1,2,3)]
+    for obj2 in other_objs:
+        obj1 = Klass()
+        test(lambda: obj1 + obj2, f"{Klass} + {type(obj2)}")
+        test(lambda: obj2 + obj1, f"{type(obj2)} + {Klass}")
 
 '''
 
@@ -222,8 +227,10 @@ def tp_decl(self, name_prefix):
 
 SLOTS = [
     Slot('tp_as_number', 'nb_bool', 'int $name$(PyObject* self)', ['1', '0', None]),
+    Slot('tp_as_number', 'nb_add', 'PyObject* $name$(PyObject* self, PyObject *other)', ['Py_NewRef(self)', 'PyLong_FromLong(0)', None]),
     Slot('tp_as_sequence', 'sq_length', 'Py_ssize_t $name$(PyObject* self)', ['0', '1', '42', None]),
-    Slot('tp_as_sequence', 'sq_item', 'PyObject* $name$(PyObject* self, Py_ssize_t index)', ['0', 'PyLong_FromSsize_t(index + 1)', None]),
+    Slot('tp_as_sequence', 'sq_item', 'PyObject* $name$(PyObject* self, Py_ssize_t index)', ['Py_NewRef(self)', 'PyLong_FromSsize_t(index + 1)', None]),
+    Slot('tp_as_sequence', 'sq_concat', 'PyObject* $name$(PyObject* self, PyObject *other)', ['Py_NewRef(self)', 'PyLong_FromLong(10)', None]),
     Slot('tp_as_mapping', 'mp_length', 'Py_ssize_t $name$(PyObject* self)', ['0', '1', '42', None]),
     Slot('tp_as_mapping', 'mp_subscript', 'PyObject* $name$(PyObject* self, PyObject* key)', ['Py_RETURN_FALSE', 'Py_NewRef(key)', None]),
     Slot(NO_GROUP, 'tp_getattr', 'PyObject* $name$(PyObject* self, char *name)', ['Py_RETURN_NONE', 'Py_RETURN_FALSE', 'Py_NewRef(self)', None,
@@ -275,6 +282,8 @@ def tp_decl(self, name_prefix):
 
 MAGIC = {
     '__bool__(self)': ['True', 'False', None],
+    '__add__(self)': ['self', '"__add__result"', "NotImplemented", None],
+    '__radd__(self)': ['self', '"__add__result"', "NotImplemented", None],
     '__len__(self)': ['0', '1', '42', None],
     '__getattribute__(self,name)': ['name', '42', 'global_dict1[name]', None],
     '__getattr__(self,name)': ['name+"abc"', 'False', 'global_dict1[name]', None],
@@ -517,9 +526,10 @@ def choose_random(l):
 
     py_source += '\n\n# ===========\n'
     py_source += '# Tests:\n\n'
+    py_source += 'all_classes = ' + ','.join(classes) + '\n\n'
     for klass in classes:
         py_source += f'print("\\n\\n\\nTesting {klass}\\n")\n'
-        py_source += f'slots_tester({klass})\n'
+        py_source += f'slots_tester({klass}, all_classes)\n'
 
     py_filename = f"test{test_case_idx}.py"
     write_all(os.path.join(output_dir, py_filename), py_source)