fix potential issues do to Binding bug, and profile getState when opportune

Author: Nicolas Laurent

src/main/java/org/truffleruby/core/rope/ConcatRope.java

@@ -9,8 +9,10 @@
  */
 package org.truffleruby.core.rope;
 
+import com.oracle.truffle.api.CompilerAsserts;
 import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.CompilerDirectives.ValueType;
+import com.oracle.truffle.api.profiles.ConditionProfile;
 import org.jcodings.Encoding;
 import org.jcodings.specific.ASCIIEncoding;
 
@@ -96,8 +98,13 @@ protected byte[] getBytesSlow() {
         return out;
     }
 
-    /** Access the state in a way that prevents race conditions. */
+    /** Access the state in a way that prevents race conditions.
+     *
+     * <p>
+     * This version is not allowed in compiled code, use {@link #getState(ConditionProfile)} there instead. */
     public ConcatState getState() {
+        CompilerAsserts.neverPartOfCompilation("use getState(ConditionProfile) instead!");
+
         if (this.bytes != null) {
             return new ConcatState(null, null, this.bytes);
         }
@@ -112,4 +119,24 @@ public ConcatState getState() {
         assert this.bytes != null;
         return new ConcatState(null, null, this.bytes);
     }
+
+    /** Access the state in a way that prevents race conditions.
+     *
+     * <p>
+     * Outside compiled code, you can use {@link #getState()}. */
+    public ConcatState getState(ConditionProfile bytesNotNull) {
+        if (bytesNotNull.profile(this.bytes != null)) {
+            return new ConcatState(null, null, this.bytes);
+        }
+
+        final ManagedRope left = this.left;
+        final ManagedRope right = this.right;
+        if (left != null && right != null) {
+            return new ConcatState(left, right, null);
+        }
+
+        CompilerDirectives.transferToInterpreterAndInvalidate();
+        assert this.bytes != null;
+        return new ConcatState(null, null, this.bytes);
+    }
 }

src/main/java/org/truffleruby/core/rope/RopeNodes.java

@@ -804,45 +804,35 @@ protected Object debugPrintSubstringRope(SubstringRope rope, int currentLevel, b
         }
 
         @TruffleBoundary
-        @Specialization(guards = "state.isBytes()")
-        protected Object debugPrintConcatRopeBytes(ConcatRope rope, int currentLevel, boolean printString,
-                @Bind("rope.getState()") ConcatState state) {
-            printPreamble(currentLevel);
-
-            System.err
-                    .println(StringUtils.format(
-                            "%s (%s; BN: %b; BL: %d; CL: %d; CR: %s; E: %s)",
-                            printString ? rope.toString() : "<skipped>",
-                            rope.getClass().getSimpleName(),
-                            false,
-                            rope.byteLength(),
-                            rope.characterLength(),
-                            rope.getCodeRange(),
-                            rope.getEncoding()));
-
-            return nil;
-        }
-
-        @TruffleBoundary
-        @Specialization(guards = "state.isChildren()")
-        protected Object debugPrintConcatRopeChildren(ConcatRope rope, int currentLevel, boolean printString,
-                @Bind("rope.getState()") ConcatState state) {
+        protected Object debugPrintConcatRopeBytes(ConcatRope rope, int currentLevel, boolean printString) {
             printPreamble(currentLevel);
 
-            System.err
-                    .println(StringUtils.format(
-                            "%s (%s; BN: %b; BL: %d; CL: %d; CR: %s; E: %s)",
-                            printString ? rope.toString() : "<skipped>",
-                            rope.getClass().getSimpleName(),
-                            // Note: converting a rope to a java.lang.String may populate the byte[].
-                            true, // are bytes null?
-                            rope.byteLength(),
-                            rope.characterLength(),
-                            rope.getCodeRange(),
-                            rope.getEncoding()));
-
-            executeDebugPrint(state.left, currentLevel + 1, printString);
-            executeDebugPrint(state.right, currentLevel + 1, printString);
+            final ConcatState state = rope.getState();
+            if (state.isBytes()) {
+                System.err.println(StringUtils.format(
+                        "%s (%s; BN: %b; BL: %d; CL: %d; CR: %s; E: %s)",
+                        printString ? rope.toString() : "<skipped>",
+                        rope.getClass().getSimpleName(),
+                        false,
+                        rope.byteLength(),
+                        rope.characterLength(),
+                        rope.getCodeRange(),
+                        rope.getEncoding()));
+            } else {
+                System.err.println(StringUtils.format(
+                        "%s (%s; BN: %b; BL: %d; CL: %d; CR: %s; E: %s)",
+                        printString ? rope.toString() : "<skipped>",
+                        rope.getClass().getSimpleName(),
+                        // Note: converting a rope to a java.lang.String may populate the byte[].
+                        true, // are bytes null?
+                        rope.byteLength(),
+                        rope.characterLength(),
+                        rope.getCodeRange(),
+                        rope.getEncoding()));
+
+                executeDebugPrint(state.left, currentLevel + 1, printString);
+                executeDebugPrint(state.right, currentLevel + 1, printString);
+            }
 
             return nil;
         }
@@ -1015,9 +1005,15 @@ protected int getByteRepeatingRope(RepeatingRope rope, int index,
             return rope.getChild().getRawBytes()[index % rope.getChild().byteLength()] & 0xff;
         }
 
+        // NOTE(norswap, 12 Jan 2021): The order of the two next specialization is significant.
+        //   Normally, @Bind expressions should only be run per node, but that's not the case currently (GR-28671).
+        //   Therefore it's important to test isChildren first, as it's possible to transition from children to bytes
+        //   but not the other way around.
+
         @Specialization(guards = "state.isChildren()")
         protected int getByteConcatRope(ConcatRope rope, int index,
-                @Bind("rope.getState()") ConcatState state,
+                @Cached ConditionProfile stateBytesNotNull,
+                @Bind("rope.getState(stateBytesNotNull)") ConcatState state,
                 @Cached ConditionProfile chooseLeftChildProfile,
                 @Cached ConditionProfile leftChildRawBytesNullProfile,
                 @Cached ConditionProfile rightChildRawBytesNullProfile,
@@ -1042,7 +1038,8 @@ protected int getByteConcatRope(ConcatRope rope, int index,
         // before we get to run the other getByteConcatRope.
         @Specialization(guards = "state.isBytes()")
         protected int getByteConcatRope(ConcatRope rope, int index,
-                @Bind("rope.getState()") ConcatState state) {
+                @Cached ConditionProfile stateBytesNotNull,
+                @Bind("rope.getState(stateBytesNotNull)") ConcatState state) {
             return state.bytes[index] & 0xff;
         }
     }