Fix slice assignment with memory overlap

Author: msimacek

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/memoryview/MemoryViewBuiltins.java

@@ -50,19 +50,19 @@
 import com.oracle.graal.python.nodes.function.builtins.clinic.ArgumentClinicProvider;
 import com.oracle.graal.python.nodes.subscript.SliceLiteralNode;
 import com.oracle.graal.python.runtime.AsyncHandler;
-import com.oracle.graal.python.runtime.ExecutionContext.IndirectCallContext;
 import com.oracle.graal.python.runtime.PythonContext;
 import com.oracle.graal.python.runtime.PythonCore;
+import com.oracle.graal.python.runtime.ExecutionContext.IndirectCallContext;
 import com.oracle.graal.python.runtime.sequence.storage.IntSequenceStorage;
 import com.oracle.graal.python.runtime.sequence.storage.SequenceStorage;
 import com.oracle.graal.python.util.PythonUtils;
 import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
 import com.oracle.truffle.api.dsl.Cached;
-import com.oracle.truffle.api.dsl.Cached.Shared;
 import com.oracle.truffle.api.dsl.GenerateNodeFactory;
 import com.oracle.truffle.api.dsl.NodeFactory;
 import com.oracle.truffle.api.dsl.Specialization;
+import com.oracle.truffle.api.dsl.Cached.Shared;
 import com.oracle.truffle.api.frame.VirtualFrame;
 import com.oracle.truffle.api.interop.ArityException;
 import com.oracle.truffle.api.interop.InteropLibrary;
@@ -211,7 +211,8 @@ Object setitem(VirtualFrame frame, IntrinsifiedPMemoryView self, PSlice slice, O
                         @Cached GetItemNode getItemNode,
                         @Cached BuiltinConstructors.MemoryViewNode createMemoryView,
                         @Cached MemoryViewNodes.PointerLookupNode pointerLookupNode,
-                        @Cached MemoryViewNodes.CopyBytesNode copyBytesNode) {
+                        @Cached MemoryViewNodes.ToJavaBytesNode toJavaBytesNode,
+                        @Cached MemoryViewNodes.WriteBytesAtNode writeBytesAtNode) {
             self.checkReleased(this);
             checkReadonly(self);
             if (self.getDimensions() != 1) {
@@ -223,11 +224,13 @@ Object setitem(VirtualFrame frame, IntrinsifiedPMemoryView self, PSlice slice, O
             if (srcView.getDimensions() != destView.getDimensions() || srcView.getBufferShape()[0] != destView.getBufferShape()[0] || !srcView.getFormatString().equals(destView.getFormatString())) {
                 throw raise(ValueError, ErrorMessages.MEMORYVIEW_DIFFERENT_STRUCTURES);
             }
+            // The intermediate array is necessary for overlapping views (where src and dest are the
+            // same buffer)
+            byte[] srcBytes = toJavaBytesNode.execute(srcView);
+            int itemsize = srcView.getItemSize();
             for (int i = 0; i < destView.getBufferShape()[0]; i++) {
-                // TODO doesn't look very efficient
                 MemoryViewNodes.MemoryPointer destPtr = pointerLookupNode.execute(frame, destView, i);
-                MemoryViewNodes.MemoryPointer srcPtr = pointerLookupNode.execute(frame, srcView, i);
-                copyBytesNode.execute(destView, destPtr.ptr, destPtr.offset, srcView, srcPtr.ptr, srcPtr.offset, destView.getItemSize());
+                writeBytesAtNode.execute(srcBytes, i * itemsize, itemsize, self, destPtr.ptr, destPtr.offset);
             }
             return PNone.NONE;
         }

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/memoryview/MemoryViewNodes.java

@@ -208,6 +208,34 @@ static void doManaged(byte[] dest, int destOffset, int len, IntrinsifiedPMemoryV
         }
     }
 
+    @GenerateUncached
+    static abstract class WriteBytesAtNode extends Node {
+        public abstract void execute(byte[] src, int srcOffset, int len, IntrinsifiedPMemoryView self, Object ptr, int offset);
+
+        @Specialization(guards = "ptr != null")
+        static void doNative(byte[] src, int srcOffset, int len, @SuppressWarnings("unused") IntrinsifiedPMemoryView self, Object ptr, int offset,
+                        @CachedLibrary(limit = "1") InteropLibrary lib) {
+            try {
+                for (int i = 0; i < len; i++) {
+                    lib.writeArrayElement(ptr, offset + i, src[srcOffset + i]);
+                }
+            } catch (UnsupportedMessageException | InvalidArrayIndexException | UnsupportedTypeException e) {
+                throw CompilerDirectives.shouldNotReachHere("native buffer read failed");
+            }
+        }
+
+        @Specialization(guards = "ptr == null")
+        static void doManaged(byte[] src, int srcOffset, int len, IntrinsifiedPMemoryView self, @SuppressWarnings("unused") Object ptr, int offset,
+                        @Cached SequenceNodes.GetSequenceStorageNode getStorageNode,
+                        @Cached SequenceStorageNodes.SetItemScalarNode setItemNode) {
+            // TODO assumes byte storage
+            SequenceStorage storage = getStorageNode.execute(self.getOwner());
+            for (int i = 0; i < len; i++) {
+                setItemNode.execute(storage, offset + i, src[srcOffset + i]);
+            }
+        }
+    }
+
     static abstract class ReadItemAtNode extends Node {
         public abstract Object execute(IntrinsifiedPMemoryView self, Object ptr, int offset);
 
@@ -274,74 +302,6 @@ static void doManaged(IntrinsifiedPMemoryView self, @SuppressWarnings("unused")
         }
     }
 
-    static abstract class CopyBytesNode extends Node {
-        public abstract void execute(IntrinsifiedPMemoryView dest, Object destPtr, int destOffset, IntrinsifiedPMemoryView src, Object srcPtr, int srcOffset, int nbytes);
-
-        @Specialization(guards = {"destPtr == null", "srcPtr == null"})
-        @SuppressWarnings("unused")
-        static void managedToManaged(IntrinsifiedPMemoryView dest, Object destPtr, int destOffset, IntrinsifiedPMemoryView src, Object srcPtr, int srcOffset, int nbytes,
-                        @Cached SequenceNodes.GetSequenceStorageNode getSequenceStorageNode,
-                        @Cached SequenceStorageNodes.MemCopyNode memCopyNode) {
-            // TODO assumes bytes storage
-            SequenceStorage destStorage = getSequenceStorageNode.execute(dest.getOwner());
-            SequenceStorage srcStorage = getSequenceStorageNode.execute(src.getOwner());
-            memCopyNode.execute(destStorage, destOffset, srcStorage, srcOffset, nbytes);
-        }
-
-        @Specialization(guards = {"destPtr != null", "srcPtr == null"})
-        @SuppressWarnings("unused")
-        static void managedToNative(IntrinsifiedPMemoryView dest, Object destPtr, int destOffset, IntrinsifiedPMemoryView src, Object srcPtr, int srcOffset, int nbytes,
-                        @Cached SequenceNodes.GetSequenceStorageNode getSequenceStorageNode,
-                        @Cached SequenceStorageNodes.GetItemScalarNode getItemNode,
-                        @Shared("lib") @CachedLibrary(limit = "1") InteropLibrary lib) {
-            // TODO assumes bytes storage
-            // TODO avoid byte->int conversion
-            // TODO explode?
-            SequenceStorage srcStorage = getSequenceStorageNode.execute(src.getOwner());
-            try {
-                for (int i = 0; i < nbytes; i++) {
-                    lib.writeArrayElement(destPtr, destOffset + i, (byte) getItemNode.executeInt(srcStorage, srcOffset + i));
-                }
-            } catch (UnsupportedMessageException | UnsupportedTypeException | InvalidArrayIndexException e) {
-                throw CompilerDirectives.shouldNotReachHere(e);
-            }
-        }
-
-        @Specialization(guards = {"destPtr == null", "srcPtr != null"})
-        @SuppressWarnings("unused")
-        static void nativeToManaged(IntrinsifiedPMemoryView dest, Object destPtr, int destOffset, IntrinsifiedPMemoryView src, Object srcPtr, int srcOffset, int nbytes,
-                        @Cached SequenceNodes.GetSequenceStorageNode getSequenceStorageNode,
-                        @Cached SequenceStorageNodes.SetItemScalarNode setItemNode,
-                        @Shared("lib") @CachedLibrary(limit = "1") InteropLibrary lib) {
-            // TODO assumes bytes storage
-            // TODO avoid byte->int conversion
-            // TODO explode?
-            SequenceStorage destStorage = getSequenceStorageNode.execute(dest.getOwner());
-            try {
-                for (int i = 0; i < nbytes; i++) {
-                    setItemNode.execute(destStorage, (byte) lib.readArrayElement(srcPtr, srcOffset + i) & 0xFF, destOffset + i);
-                }
-            } catch (UnsupportedMessageException | InvalidArrayIndexException e) {
-                throw CompilerDirectives.shouldNotReachHere(e);
-            }
-        }
-
-        @Specialization(guards = {"destPtr != null", "srcPtr != null"})
-        @SuppressWarnings("unused")
-        static void nativeToNative(IntrinsifiedPMemoryView dest, Object destPtr, int destOffset, IntrinsifiedPMemoryView src, Object srcPtr, int srcOffset, int nbytes,
-                        @Shared("lib") @CachedLibrary(limit = "1") InteropLibrary lib) {
-            // TODO call native memcpy?
-            // TODO explode?
-            try {
-                for (int i = 0; i < nbytes; i++) {
-                    lib.writeArrayElement(destPtr, destOffset + i, lib.readArrayElement(srcPtr, srcOffset + i));
-                }
-            } catch (UnsupportedMessageException | UnsupportedTypeException | InvalidArrayIndexException e) {
-                throw CompilerDirectives.shouldNotReachHere(e);
-            }
-        }
-    }
-
     @ValueType
     static class MemoryPointer {
         public Object ptr;