Always clear the source location when moving values between locations.

Author: woess

truffle/src/com.oracle.truffle.api.object/src/com/oracle/truffle/api/object/DynamicObjectLibraryImpl.java

@@ -390,14 +390,13 @@ static RemovePlan prepareRemove(Shape shapeBefore, Shape shapeAfter, Property re
                 moves.add(move);
             }
         }
+        Location removedPropertyLoc = removedProperty.getLocation();
+        if (!removedPropertyLoc.isValue()) {
+            // Use a no-op move to clear the location of the removed property. Must be first.
+            moves.add(new Move(removedPropertyLoc, null, removedPropertyLoc.getOrdinal(), Integer.MIN_VALUE));
+        }
         if (canMoveInPlace) {
-            if (moves.isEmpty()) {
-                Location removedPropertyLoc = removedProperty.getLocation();
-                if (!removedPropertyLoc.isPrimitive()) {
-                    // Use a no-op move to clear the location of the removed property.
-                    moves.add(new Move(removedPropertyLoc, removedPropertyLoc, 0, 0));
-                }
-            } else if (!isSorted(moves)) {
+            if (!isSorted(moves)) {
                 moves.sort(Move::compareTo);
             }
         }
@@ -415,6 +414,10 @@ private static boolean isSorted(List<Move> moves) {
         return true;
     }
 
+    /**
+     * Moves the value from one location to another, and clears the from location. If both locations
+     * are the same, only clears the location.
+     */
     private static final class Move implements Comparable<Move> {
         private final Location fromLoc;
         private final Location toLoc;
@@ -424,30 +427,51 @@ private static final class Move implements Comparable<Move> {
         private static final Move[] EMPTY_ARRAY = new Move[0];
 
         Move(Location fromLoc, Location toLoc, int fromOrd, int toOrd) {
-            assert (fromLoc == toLoc) == (fromOrd == toOrd);
-            this.fromLoc = fromLoc;
+            assert fromLoc != toLoc;
+            this.fromLoc = Objects.requireNonNull(fromLoc);
             this.toLoc = toLoc;
             this.fromOrd = fromOrd;
             this.toOrd = toOrd;
         }
 
         void perform(DynamicObject obj) {
-            if (fromLoc == toLoc) {
-                return;
+            if (toLoc != null) {
+                performSet(obj, performGet(obj));
             }
-            performSet(obj, performGet(obj));
+            /*
+             * It does not matter for correctness whether we clear after the get or set. Clearing
+             * after the set might be slightly preferable w.r.t. store ordering.
+             */
+            clear(obj);
         }
 
         Object performGet(DynamicObject obj) {
             return fromLoc.get(obj, false);
         }
 
         void performSet(DynamicObject obj, Object value) {
+            if (toLoc == null) {
+                return; // clear only
+            }
             toLoc.setSafe(obj, value, false, true);
         }
 
-        void clear(DynamicObject obj) {
-            // clear location to avoid memory leak
+        Object performGetAndClear(DynamicObject obj) {
+            Object value = performGet(obj);
+            clear(obj);
+            return value;
+        }
+
+        /*
+         * Clears the location to avoid potential memory leaks AND kills the from location identity.
+         *
+         * Note: It is important that we always set the "from" location in compiled code to kill the
+         * location identity at this point, so as to prevent reads from that location identity (of
+         * the old shape) to move below any write to the same memory location but with a different
+         * location identity (of the new shape), since that would be seen as independent and
+         * therefore not kill the "from" location that is being overwritten or cleared. (GR-59981)
+         */
+        private void clear(DynamicObject obj) {
             fromLoc.clear(obj);
         }
 
@@ -498,25 +522,19 @@ void perform(DynamicObject object) {
                     moves[i].perform(object);
                 }
 
-                if (moves.length > 0) {
-                    moves[0].clear(object);
-                }
-
                 DynamicObjectSupport.trimToSize(object, shapeBefore, shapeAfter);
-                object.setShape(shapeAfter);
             } else {
                 // we cannot perform the moves in place, so stash away the values
                 Object[] tempValues = new Object[moves.length];
                 for (int i = moves.length - 1; i >= 0; i--) {
-                    tempValues[i] = moves[i].performGet(object);
-                    moves[i].clear(object);
+                    tempValues[i] = moves[i].performGetAndClear(object);
                 }
                 DynamicObjectSupport.resize(object, shapeBefore, shapeAfter);
                 for (int i = moves.length - 1; i >= 0; i--) {
                     moves[i].performSet(object, tempValues[i]);
                 }
-                object.setShape(shapeAfter);
             }
+            object.setShape(shapeAfter);
         }
 
         @TruffleBoundary