[GR-53462] Fix: Memmove node bug

PullRequest: graalpython/3322

Author: horakivo

graalpython/com.oracle.graal.python.test/src/com/oracle/graal/python/test/nodes/MemMoveNodeTests.java

@@ -0,0 +1,113 @@
+/*
+ * Copyright (c) 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.nodes;
+
+import com.oracle.graal.python.builtins.objects.PNone;
+import com.oracle.graal.python.builtins.objects.cext.capi.CApiContext;
+import com.oracle.graal.python.builtins.objects.common.SequenceStorageNodes.MemMoveNode;
+import com.oracle.graal.python.builtins.objects.common.SequenceStorageNodes.StorageToNativeNode;
+import com.oracle.graal.python.builtins.objects.common.SequenceStorageNodes.GetItemScalarNode;
+import com.oracle.graal.python.builtins.objects.common.SequenceStorageNodesFactory.MemMoveNodeGen;
+import com.oracle.graal.python.runtime.GilNode;
+import com.oracle.graal.python.runtime.sequence.storage.IntSequenceStorage;
+import com.oracle.graal.python.runtime.sequence.storage.NativeSequenceStorage;
+import com.oracle.graal.python.test.PythonTests;
+import com.oracle.truffle.api.frame.VirtualFrame;
+import com.oracle.truffle.api.nodes.RootNode;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+public class MemMoveNodeTests {
+
+    private GilNode.UncachedAcquire gil;
+
+    @Before
+    public void setUp() {
+        PythonTests.enterContext();
+        this.gil = GilNode.uncachedAcquire();
+        CApiContext.ensureCapiWasLoaded();
+    }
+
+    @After
+    public void tearDown() {
+        this.gil.close();
+        PythonTests.closeContext();
+    }
+
+    @Test
+    public void doArrayBasedSpecialization() {
+        final var storage = new IntSequenceStorage(new int[]{1, 2, 2, 3, 0});
+        new RootNode(null) {
+
+            @Override
+            public Object execute(VirtualFrame frame) {
+                var memMoveNode = MemMoveNodeGen.getUncached();
+                memMoveNode.execute(this, storage, 3, 2, 2);
+                return PNone.NO_VALUE;
+            }
+        }.getCallTarget().call();
+
+        var expectedArr = new int[]{1, 2, 2, 2, 3};
+        Assert.assertArrayEquals(expectedArr, storage.getInternalIntArray());
+    }
+
+    @Test
+    public void doOtherSpecialization() {
+        final NativeSequenceStorage storage = StorageToNativeNode.executeUncached(new Object[]{1, 2, 2, 3, 0}, 5);
+
+        new RootNode(null) {
+            @Override
+            public Object execute(VirtualFrame frame) {
+                MemMoveNode.executeUncached(storage, 3, 2, 2);
+                return PNone.NO_VALUE;
+            }
+
+        }.getCallTarget().call();
+
+        Assert.assertEquals(1, GetItemScalarNode.executeUncached(storage, 0));
+        Assert.assertEquals(2, GetItemScalarNode.executeUncached(storage, 1));
+        Assert.assertEquals(2, GetItemScalarNode.executeUncached(storage, 2));
+        Assert.assertEquals(2, GetItemScalarNode.executeUncached(storage, 3));
+        Assert.assertEquals(3, GetItemScalarNode.executeUncached(storage, 4));
+    }
+}

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/common/SequenceStorageNodes.java

@@ -73,6 +73,8 @@
 import com.oracle.graal.python.builtins.objects.common.SequenceStorageNodesFactory.SetItemDynamicNodeGen;
 import com.oracle.graal.python.builtins.objects.common.SequenceStorageNodesFactory.SetItemNodeGen;
 import com.oracle.graal.python.builtins.objects.common.SequenceStorageNodesFactory.ToByteArrayNodeGen;
+import com.oracle.graal.python.builtins.objects.common.SequenceStorageNodesFactory.StorageToNativeNodeGen;
+import com.oracle.graal.python.builtins.objects.common.SequenceStorageNodesFactory.MemMoveNodeGen;
 import com.oracle.graal.python.builtins.objects.ints.PInt;
 import com.oracle.graal.python.builtins.objects.iterator.IteratorBuiltins.NextHelperNode;
 import com.oracle.graal.python.builtins.objects.iterator.IteratorNodes.BuiltinIteratorLengthHint;
@@ -1171,6 +1173,10 @@ public abstract static class MemMoveNode extends Node {
 
         public abstract void execute(Node inliningTarget, SequenceStorage s, int distPos, int srcPos, int length);
 
+        public static void executeUncached(SequenceStorage s, int distPos, int srcPos, int length) {
+            MemMoveNodeGen.getUncached().execute(null, s, distPos, srcPos, length);
+        }
+
         @SuppressWarnings("unused")
         @Specialization(guards = "length <= 0")
         protected static void nothing(SequenceStorage storage, int distPos, int srcPos, int length) {
@@ -1187,8 +1193,38 @@ protected static void doArrayBasedMove(ArrayBasedSequenceStorage storage, int di
         protected static void doOther(Node inliningTarget, SequenceStorage storage, int distPos, int srcPos, int length,
                         @Cached SetItemScalarNode setLeftItemNode,
                         @Cached GetItemScalarNode getRightItemNode) {
-            for (int cur = distPos, j = srcPos, i = 0; i < length; cur += 1, j++, i++) {
-                setLeftItemNode.execute(inliningTarget, storage, cur, getRightItemNode.execute(inliningTarget, storage, j));
+
+            if (srcPos < distPos && srcPos + length > distPos) {
+                // case where we override values which we later want to read, therefore we need to
+                // copy from the end
+                copyBackward(inliningTarget, storage, getRightItemNode, setLeftItemNode, distPos, srcPos, length);
+
+            } else {
+                // otherwise copying forwards
+                copyForward(inliningTarget, storage, getRightItemNode, setLeftItemNode, distPos, srcPos, length);
+            }
+        }
+
+        private static void copyForward(Node inliningTarget, SequenceStorage storage, GetItemScalarNode getRightItemNode, SetItemScalarNode setLeftItemNode, int destPos, int srcPos, int length) {
+            while (length > 0) {
+                var value = getRightItemNode.execute(inliningTarget, storage, srcPos);
+                setLeftItemNode.execute(inliningTarget, storage, destPos, value);
+                destPos++;
+                srcPos++;
+                length--;
+
+            }
+        }
+
+        private static void copyBackward(Node inliningTarget, SequenceStorage storage, GetItemScalarNode getRightItemNode, SetItemScalarNode setLeftItemNode, int destPos, int srcPos, int length) {
+            destPos += length;
+            srcPos += length;
+            while (length > 0) {
+                destPos--;
+                srcPos--;
+                length--;
+                var value = getRightItemNode.execute(inliningTarget, storage, srcPos);
+                setLeftItemNode.execute(inliningTarget, storage, destPos, value);
             }
         }
 
@@ -1475,6 +1511,10 @@ public final NativeByteSequenceStorage executeBytes(Node inliningTarget, byte[]
             return executeBytes(inliningTarget, obj, length, true);
         }
 
+        public static NativeSequenceStorage executeUncached(Object obj, int length) {
+            return StorageToNativeNodeGen.getUncached().execute(null, obj, length);
+        }
+
         public final NativeSequenceStorage execute(Node inliningTarget, Object obj, int length) {
             return execute(inliningTarget, obj, length, true);
         }