[GR-56701] Migrate to nb_add/sq_concat CPython like slots.

PullRequest: graalpython/3412

Author: steve-s

graalpython/com.oracle.graal.python.annotations/src/com/oracle/graal/python/annotations/Slot.java

@@ -93,8 +93,10 @@
 
     enum SlotKind {
         nb_bool,
+        nb_add,
         sq_length,
         sq_item,
+        sq_concat,
         mp_length,
         mp_subscript,
         tp_descr_get,

graalpython/com.oracle.graal.python.cext/src/capi.c

@@ -367,7 +367,6 @@ PyAPI_FUNC(int64_t) get_methods_flags(PyTypeObject *cls) {
 	PyNumberMethods* number = cls->tp_as_number;
     if (number != NULL) {
 #define COMPUTE_FLAGS(NAME, BIT_IDX) flags |= ((number->NAME != NULL) * BIT_IDX);
-		COMPUTE_FLAGS(nb_add, NB_ADD)
 		COMPUTE_FLAGS(nb_subtract, NB_SUBTRACT)
 		COMPUTE_FLAGS(nb_multiply, NB_MULTIPLY)
 		COMPUTE_FLAGS(nb_remainder, NB_REMAINDER)
@@ -409,7 +408,6 @@ PyAPI_FUNC(int64_t) get_methods_flags(PyTypeObject *cls) {
     if (sequence != NULL) {
 #define COMPUTE_FLAGS(NAME, BIT_IDX) flags |= ((sequence->NAME != NULL) * BIT_IDX);
 		COMPUTE_FLAGS(sq_length, SQ_LENGTH)
-		COMPUTE_FLAGS(sq_concat, SQ_CONCAT)
 		COMPUTE_FLAGS(sq_repeat, SQ_REPEAT)
 		COMPUTE_FLAGS(sq_item, SQ_ITEM)
 		COMPUTE_FLAGS(sq_ass_item, SQ_ASS_ITEM)

graalpython/com.oracle.graal.python.processor/src/com/oracle/graal/python/processor/SlotsMapping.java

@@ -42,6 +42,7 @@
 
 import com.oracle.graal.python.annotations.Slot;
 import com.oracle.graal.python.annotations.Slot.SlotKind;
+import com.oracle.graal.python.processor.SlotsProcessor.TpSlotData;
 
 public class SlotsMapping {
     private static String getSuffix(boolean isComplex) {
@@ -51,6 +52,8 @@ private static String getSuffix(boolean isComplex) {
     static String getSlotBaseClass(Slot s) {
         return switch (s.value()) {
             case nb_bool -> "TpSlotInquiry.TpSlotInquiryBuiltin";
+            case nb_add -> "TpSlotBinaryOp.TpSlotBinaryOpBuiltin";
+            case sq_concat -> "TpSlotBinaryFunc.TpSlotSqConcat";
             case sq_length, mp_length -> "TpSlotLen.TpSlotLenBuiltin" + getSuffix(s.isComplex());
             case sq_item -> "TpSlotSizeArgFun.TpSlotSizeArgFunBuiltin";
             case mp_subscript -> "TpSlotBinaryFunc.TpSlotMpSubscript";
@@ -65,6 +68,8 @@ static String getSlotNodeBaseClass(Slot s) {
         return switch (s.value()) {
             case tp_descr_get -> "com.oracle.graal.python.builtins.objects.type.slots.TpSlotDescrGet.DescrGetBuiltinNode";
             case nb_bool -> "com.oracle.graal.python.builtins.objects.type.slots.TpSlotInquiry.NbBoolBuiltinNode";
+            case nb_add -> "com.oracle.graal.python.builtins.objects.type.slots.TpSlotBinaryOp.BinaryOpBuiltinNode";
+            case sq_concat -> "com.oracle.graal.python.builtins.objects.type.slots.TpSlotBinaryFunc.SqConcatBuiltinNode";
             case sq_length, mp_length -> "com.oracle.graal.python.builtins.objects.type.slots.TpSlotLen.LenBuiltinNode";
             case sq_item -> "com.oracle.graal.python.builtins.objects.type.slots.TpSlotSizeArgFun.SqItemBuiltinNode";
             case mp_subscript -> "com.oracle.graal.python.builtins.objects.type.slots.TpSlotBinaryFunc.MpSubscriptBuiltinNode";
@@ -79,7 +84,7 @@ static String getUncachedExecuteSignature(SlotKind s) {
             case nb_bool -> "boolean executeUncached(Object self)";
             case tp_descr_get -> "Object executeUncached(Object self, Object obj, Object type)";
             case sq_length, mp_length -> "int executeUncached(Object self)";
-            case tp_getattro, tp_descr_set, tp_setattro, sq_item, mp_subscript ->
+            case tp_getattro, tp_descr_set, tp_setattro, sq_item, mp_subscript, nb_add, sq_concat ->
                 throw new AssertionError("Should not reach here: should be always complex");
         };
     }
@@ -88,15 +93,15 @@ static boolean supportsComplex(SlotKind s) {
         return switch (s) {
             case nb_bool -> false;
             case sq_length, mp_length, tp_getattro, tp_descr_get, tp_descr_set,
-                            tp_setattro, sq_item, mp_subscript ->
+                            tp_setattro, sq_item, mp_subscript, nb_add, sq_concat ->
                 true;
         };
     }
 
     static boolean supportsSimple(SlotKind s) {
         return switch (s) {
             case nb_bool, sq_length, mp_length, tp_descr_get -> true;
-            case tp_getattro, tp_descr_set, tp_setattro, sq_item, mp_subscript -> false;
+            case tp_getattro, tp_descr_set, tp_setattro, sq_item, mp_subscript, nb_add, sq_concat -> false;
         };
     }
 
@@ -105,8 +110,18 @@ static String getUncachedExecuteCall(SlotKind s) {
             case nb_bool -> "executeBool(null, self)";
             case sq_length, mp_length -> "executeInt(null, self)";
             case tp_descr_get -> "execute(null, self, obj, type)";
-            case tp_getattro, tp_descr_set, tp_setattro, sq_item, mp_subscript ->
+            case tp_getattro, tp_descr_set, tp_setattro, sq_item, mp_subscript, nb_add, sq_concat ->
                 throw new AssertionError("Should not reach here: should be always complex");
         };
     }
+
+    public static String getExtraCtorArgs(TpSlotData slot) {
+        return switch (slot.slot().value()) {
+            case nb_add -> ", com.oracle.graal.python.nodes.SpecialMethodNames.J___ADD__";
+            case nb_bool, tp_setattro, tp_getattro,
+                            tp_descr_set, tp_descr_get, mp_subscript,
+                            mp_length, sq_concat, sq_item, sq_length ->
+                "";
+        };
+    }
 }

graalpython/com.oracle.graal.python.processor/src/com/oracle/graal/python/processor/SlotsProcessor.java

@@ -184,7 +184,7 @@ private void writeSlot(CodeWriter w, TpSlotData slot) throws IOException {
                 w.startLn().//
                                 write("super(").//
                                 write(getNodeFactory(slot, slot.slotNodeType()) + ".getInstance()").//
-                                endLn(");");
+                                write(SlotsMapping.getExtraCtorArgs(slot)).endLn(");");
 
             }
             w.writeLn("}");

graalpython/com.oracle.graal.python.test/src/com/oracle/graal/python/test/builtin/MethodFlagsTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * The Universal Permissive License (UPL), Version 1.0
@@ -87,7 +87,6 @@ private static void assertMethodConsistentWithFlag(PythonBuiltinClassType clazz,
     @Test
     public void testMethodFlagsConsistency() {
         for (PythonBuiltinClassType clazz : PythonBuiltinClassType.VALUES) {
-            assertMethodConsistentWithFlag(clazz, MethodsFlags.NB_ADD | MethodsFlags.SQ_CONCAT, SpecialMethodNames.T___ADD__);
             assertMethodConsistentWithFlag(clazz, MethodsFlags.NB_SUBTRACT, SpecialMethodNames.T___SUB__);
             assertMethodConsistentWithFlag(clazz, MethodsFlags.NB_MULTIPLY | MethodsFlags.SQ_REPEAT, SpecialMethodNames.T___MUL__);
             assertMethodConsistentWithFlag(clazz, MethodsFlags.NB_REMAINDER, SpecialMethodNames.T___MOD__);

graalpython/com.oracle.graal.python.test/src/com/oracle/graal/python/test/builtin/objects/TpSlotsTests.java

@@ -72,6 +72,26 @@ public void testBuilderBasic() {
         checkSlotValue(TpSlotMeta.TP_SETATTRO, slots.combined_tp_setattro_setattr());
     }
 
+    @Test
+    public void testBuilderExplicitGroups() {
+        Builder builder = TpSlots.newBuilder();
+        builder.allocateAllGroups();
+        TpSlots slots = builder.build();
+        for (TpSlotGroup group : TpSlotGroup.VALID_VALUES) {
+            Assert.assertTrue(getGroup(slots, group));
+        }
+    }
+
+    @Test
+    public void testBuilderExplicitGroup() {
+        for (TpSlotGroup group : TpSlotGroup.VALID_VALUES) {
+            Builder builder = TpSlots.newBuilder();
+            builder.setExplicitGroup(group);
+            TpSlots slots = builder.build();
+            Assert.assertTrue(getGroup(slots, group));
+        }
+    }
+
     @Test
     public void testBuilderOptimizations1() {
         Builder builder = TpSlots.newBuilder();
@@ -112,18 +132,21 @@ private static void verifySlots(TpSlots slots, Function<TpSlotMeta, Boolean> che
                 Assert.assertNull(def.name(), slotValue);
             }
         }
-        for (TpSlotGroup group : TpSlotGroup.values()) {
-            switch (group) {
-                case NO_GROUP -> {
-                }
-                case AS_NUMBER -> Assert.assertEquals(slots.has_as_number(), groupsSeen.contains(group));
-                case AS_SEQUENCE -> Assert.assertEquals(slots.has_as_sequence(), groupsSeen.contains(group));
-                case AS_MAPPING -> Assert.assertEquals(slots.has_as_mapping(), groupsSeen.contains(group));
-            }
+        for (TpSlotGroup group : TpSlotGroup.VALID_VALUES) {
+            Assert.assertEquals(getGroup(slots, group), groupsSeen.contains(group));
         }
     }
 
     private static void checkSlotValue(TpSlotMeta def, TpSlot slotValue) {
         Assert.assertTrue(def.name(), slotValue instanceof TpSlotNative slotNative && slotNative.getCallable() == def);
     }
+
+    private static boolean getGroup(TpSlots slots, TpSlotGroup group) {
+        return switch (group) {
+            case NO_GROUP -> throw new IllegalArgumentException();
+            case AS_NUMBER -> slots.has_as_number();
+            case AS_SEQUENCE -> slots.has_as_sequence();
+            case AS_MAPPING -> slots.has_as_mapping();
+        };
+    }
 }

graalpython/com.oracle.graal.python.test/src/com/oracle/graal/python/test/builtin/objects/cext/SlotWrapperTests.java

@@ -0,0 +1,80 @@
+/*
+ * Copyright (c) 2024, 2024, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * The Universal Permissive License (UPL), Version 1.0
+ *
+ * Subject to the condition set forth below, permission is hereby granted to any
+ * person obtaining a copy of this software, associated documentation and/or
+ * data (collectively the "Software"), free of charge and under any and all
+ * copyright rights in the Software, and any and all patent rights owned or
+ * freely licensable by each licensor hereunder covering either (i) the
+ * unmodified Software as contributed to or provided by such licensor, or (ii)
+ * the Larger Works (as defined below), to deal in both
+ *
+ * (a) the Software, and
+ *
+ * (b) any piece of software and/or hardware listed in the lrgrwrks.txt file if
+ * one is included with the Software each a "Larger Work" to which the Software
+ * is contributed by such licensors),
+ *
+ * without restriction, including without limitation the rights to copy, create
+ * derivative works of, display, perform, and distribute the Software and make,
+ * use, sell, offer for sale, import, export, have made, and have sold the
+ * Software and the Larger Work(s), and to sublicense the foregoing rights on
+ * either these or other terms.
+ *
+ * This license is subject to the following condition:
+ *
+ * The above copyright notice and either this complete permission notice or at a
+ * minimum a reference to the UPL must be included in all copies or substantial
+ * portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+package com.oracle.graal.python.test.builtin.objects.cext;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import org.junit.Test;
+
+import com.oracle.graal.python.builtins.objects.cext.capi.PyProcsWrapper.TpSlotWrapper;
+import com.oracle.graal.python.builtins.objects.type.TpSlots.TpSlotMeta;
+import com.oracle.graal.python.builtins.objects.type.slots.TpSlot.TpSlotPythonSingle;
+import com.oracle.graal.python.util.PythonUtils;
+import com.oracle.truffle.api.strings.TruffleString;
+
+public class SlotWrapperTests {
+    // Doesn't really need to be a real callable or type
+    private static final Object DUMMY_CALLABLE = new Object();
+    private static final Object DUMMY_TYPE = new Object();
+
+    @Test
+    public void testCloneContract() {
+        TruffleString testName = PythonUtils.toTruffleStringUncached("__test__");
+        TpSlotPythonSingle slotValue = new TpSlotPythonSingle(DUMMY_CALLABLE, DUMMY_TYPE, testName);
+        TpSlotPythonSingle slotValue2 = new TpSlotPythonSingle(DUMMY_CALLABLE, DUMMY_TYPE, testName);
+        for (TpSlotMeta slot : TpSlotMeta.values()) {
+            if (!slot.supportsManagedSlotValues()) {
+                continue;
+            }
+            // We rely on the wrapper not doing any validation of the value...
+            TpSlotWrapper wrapper = slot.createNativeWrapper(slotValue);
+            assertThat(wrapper.getSlot(), equalTo(slotValue));
+
+            TpSlotWrapper clonedWrapper = wrapper.cloneWith(slotValue2);
+            assertThat(clonedWrapper, not(equalTo(wrapper)));
+            assertThat(clonedWrapper, instanceOf(wrapper.getClass()));
+            assertThat(clonedWrapper.getSlot(), equalTo(slotValue2));
+        }
+    }
+}

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

@@ -53,15 +53,31 @@
                          static PyObject* get_tp_setattro(PyObject* unused, PyObject* object) {
                              return PyLong_FromVoidPtr(Py_TYPE(object)->tp_setattro);
                          }
+                         static PyObject* get_nb_bool(PyObject* unused, PyObject* object) {
+                             return PyLong_FromVoidPtr(Py_TYPE(object)->tp_as_number == NULL ? NULL : Py_TYPE(object)->tp_as_number->nb_bool);
+                         }
+                         static PyObject* get_tp_as_number(PyObject* unused, PyObject* object) {
+                             return PyLong_FromVoidPtr(Py_TYPE(object)->tp_as_number);
+                         }
+                         static PyObject* get_sq_concat(PyObject* unused, PyObject* object) {
+                             return PyLong_FromVoidPtr(Py_TYPE(object)->tp_as_sequence == NULL ? NULL : Py_TYPE(object)->tp_as_sequence->sq_concat);
+                         }
                          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, ""},' +
                          '{"get_tp_attro", (PyCFunction)get_tp_attro, METH_O | METH_STATIC, ""},' +
                          '{"get_tp_setattr", (PyCFunction)get_tp_setattr, METH_O | METH_STATIC, ""},' +
                          '{"get_tp_setattro", (PyCFunction)get_tp_setattro, METH_O | METH_STATIC, ""},' +
+                         '{"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, ""}')
 
 
@@ -513,3 +529,58 @@ class DisablesHash2(TypeWithHash):
     # )
     #
     # assert_has_no_hash(TypeWithoutHashExplicit())
+
+
+def test_attr_update():
+    # Note: version with managed super type whose attribute is updated and should
+    # be propagated to the native subtype segfaults on CPython in various ways
+    TypeToBeUpdated = CPyExtHeapType("TypeToBeUpdated")
+    assert SlotsGetter.get_tp_as_number(TypeToBeUpdated()) != 0
+
+    TypeToBeUpdated.__bool__ = lambda self: False
+    assert not bool(TypeToBeUpdated())
+
+    TypeToBeUpdated.__add__ = lambda self,other: f"plus {other}"
+    assert TypeToBeUpdated() + "test" == "plus test"
+
+    class ManagedGoesNative:
+        pass
+
+    assert bool(ManagedGoesNative())
+    assert SlotsGetter.get_nb_bool(ManagedGoesNative()) == 0 # Sends it to native
+    assert SlotsGetter.get_tp_as_number(ManagedGoesNative()) != 0
+    assert bool(ManagedGoesNative())
+
+    ManagedGoesNative.__bool__ = lambda self: False
+    assert not bool(ManagedGoesNative())
+    assert SlotsGetter.get_nb_bool(ManagedGoesNative()) != 0
+
+
+def test_nb_add_inheritace_does_not_add_sq_concat():
+    NbAddOnlyHeapType = CPyExtHeapType("NbAddOnlyHeapType",
+                                       code = 'PyObject* my_nb_add(PyObject* self, PyObject *other) { return Py_NewRef(self); }',
+                                       slots=['{Py_nb_add, &my_nb_add}'])
+    class ManagedSub(NbAddOnlyHeapType):
+        pass
+
+    assert ManagedSub.__add__
+    assert SlotsGetter.get_sq_concat(ManagedSub()) == 0
+
+    class ManagedSub2(NbAddOnlyHeapType):
+        def __add__(self, other): return NotImplemented
+
+    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 ManagedDummy(NbAddSqConcatStaticType):
+        pass
+
+    assert SlotsGetter.get_nb_add(ManagedDummy()) == SlotsGetter.get_nb_add(NbAddSqConcatStaticType())