No unsafe gets from closures; parent may not be jitted.

This is a "fix" for #4235 in that it prevents closures from ever blindly reading closed-over variables that have not been initialized to nil. This may happen if the block jits independent of the method, in which case the method will continue to be run through null-checking interpreter logic, so the closure code cannot guarantee all values are non-null on first read. This is only a "fix" in quotes because it completely disables the fast path optimizaton for all non-method JIT. Note that I did add specialized "OrNil" paths to the generated DynamicScope subclasses, so that should at least inline to a field plus null check.
jruby · Oct 31, 2016 · 0e65638 · 0e65638
1 parent b9b86fc
commit 0e65638
Show file tree

Hide file tree

Showing 3 changed files with 121 additions and 8 deletions.
diff --git a/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java b/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
@@ -2326,19 +2326,41 @@ public void LocalVariable(LocalVariable localvariable) {
         int depth = localvariable.getScopeDepth();
         int location = localvariable.getLocation();
 
-        jvmLoadLocal(DYNAMIC_SCOPE);
+        // We can only use the fast path with no null checking in methods, since closures may JIT independently
+        // atop methods that do not guarantee all scoped vars are initialized. See jruby/jruby#4235.
+        if (jvm.methodData().scope instanceof IRMethod) {
+            jvmLoadLocal(DYNAMIC_SCOPE);
 
-        if (depth == 0) {
-            if (location < DynamicScopeGenerator.SPECIALIZED_GETS.size()) {
-                m.adapter.invokevirtual(p(DynamicScope.class), DynamicScopeGenerator.SPECIALIZED_GETS.get(location), sig(IRubyObject.class));
+            if (depth == 0) {
+                if (location < DynamicScopeGenerator.SPECIALIZED_GETS.size()) {
+                    m.adapter.invokevirtual(p(DynamicScope.class), DynamicScopeGenerator.SPECIALIZED_GETS.get(location), sig(IRubyObject.class));
+                } else {
+                    m.adapter.pushInt(location);
+                    m.adapter.invokevirtual(p(DynamicScope.class), "getValueDepthZero", sig(IRubyObject.class, int.class));
+                }
             } else {
                 m.adapter.pushInt(location);
-                m.adapter.invokevirtual(p(DynamicScope.class), "getValueDepthZero", sig(IRubyObject.class, int.class));
+                m.adapter.pushInt(depth);
+                m.adapter.invokevirtual(p(DynamicScope.class), "getValue", sig(IRubyObject.class, int.class, int.class));
             }
         } else {
-            m.adapter.pushInt(location);
-            m.adapter.pushInt(depth);
-            m.adapter.invokevirtual(p(DynamicScope.class), "getValue", sig(IRubyObject.class, int.class, int.class));
+            jvmLoadLocal(DYNAMIC_SCOPE);
+
+            if (depth == 0) {
+                if (location < DynamicScopeGenerator.SPECIALIZED_GETS_OR_NIL.size()) {
+                    m.pushNil();
+                    m.adapter.invokevirtual(p(DynamicScope.class), DynamicScopeGenerator.SPECIALIZED_GETS_OR_NIL.get(location), sig(IRubyObject.class, IRubyObject.class));
+                } else {
+                    m.adapter.pushInt(location);
+                    m.pushNil();
+                    m.adapter.invokevirtual(p(DynamicScope.class), "getValueDepthZeroOrNil", sig(IRubyObject.class, int.class, IRubyObject.class));
+                }
+            } else {
+                m.adapter.pushInt(location);
+                m.adapter.pushInt(depth);
+                m.pushNil();
+                m.adapter.invokevirtual(p(DynamicScope.class), "getValueOrNil", sig(IRubyObject.class, int.class, int.class, IRubyObject.class));
+            }
         }
     }
 

diff --git a/core/src/main/java/org/jruby/runtime/DynamicScope.java b/core/src/main/java/org/jruby/runtime/DynamicScope.java
@@ -290,6 +290,54 @@ public IRubyObject getValueThreeDepthZeroOrNil(IRubyObject nil) {
         return value == null ? setValueDepthZero(nil, 3) : value;
     }
 
+    /**
+     * getValueOrNil for index 4, depth 0
+     */
+    public IRubyObject getValueFourDepthZeroOrNil(IRubyObject nil) {
+        IRubyObject value = getValueFourDepthZero();
+        return value == null ? setValueDepthZero(nil, 4) : value;
+    }
+
+    /**
+     * getValueOrNil for index 5, depth 0
+     */
+    public IRubyObject getValueFiveDepthZeroOrNil(IRubyObject nil) {
+        IRubyObject value = getValueFiveDepthZero();
+        return value == null ? setValueDepthZero(nil, 5) : value;
+    }
+
+    /**
+     * getValueOrNil for index 6, depth 0
+     */
+    public IRubyObject getValueSixDepthZeroOrNil(IRubyObject nil) {
+        IRubyObject value = getValueSixDepthZero();
+        return value == null ? setValueDepthZero(nil, 6) : value;
+    }
+
+    /**
+     * getValueOrNil for index 7, depth 0
+     */
+    public IRubyObject getValueSevenDepthZeroOrNil(IRubyObject nil) {
+        IRubyObject value = getValueSevenDepthZero();
+        return value == null ? setValueDepthZero(nil, 7) : value;
+    }
+
+    /**
+     * getValueOrNil for index 8, depth 0
+     */
+    public IRubyObject getValueEightDepthZeroOrNil(IRubyObject nil) {
+        IRubyObject value = getValueEightDepthZero();
+        return value == null ? setValueDepthZero(nil, 8) : value;
+    }
+
+    /**
+     * getValueOrNil for index 9, depth 0
+     */
+    public IRubyObject getValueNineDepthZeroOrNil(IRubyObject nil) {
+        IRubyObject value = getValueNineDepthZero();
+        return value == null ? setValueDepthZero(nil, 9) : value;
+    }
+
     /**
      * Set value in current dynamic scope or one of its captured scopes.
      *

diff --git a/core/src/main/java/org/jruby/runtime/scope/DynamicScopeGenerator.java b/core/src/main/java/org/jruby/runtime/scope/DynamicScopeGenerator.java
@@ -44,6 +44,19 @@ public class DynamicScopeGenerator {
             "getValueNineDepthZero"
     ));
 
+    public static final List<String> SPECIALIZED_GETS_OR_NIL = Collections.unmodifiableList(Arrays.asList(
+            "getValueZeroDepthZeroOrNil",
+            "getValueOneDepthZeroOrNil",
+            "getValueTwoDepthZeroOrNil",
+            "getValueThreeDepthZeroOrNil",
+            "getValueFourDepthZeroOrNil",
+            "getValueFiveDepthZeroOrNil",
+            "getValueSixDepthZeroOrNil",
+            "getValueSevenDepthZeroOrNil",
+            "getValueEightDepthZeroOrNil",
+            "getValueNineDepthZeroOrNil"
+    ));
+
     public static final List<String> SPECIALIZED_SETS = Collections.unmodifiableList(Arrays.asList(
             "setValueZeroDepthZeroVoid",
             "setValueOneDepthZeroVoid",
@@ -178,6 +191,36 @@ public static MethodHandle generate(final int size) {
                     }});
                 }
 
+                for (int i = 0; i < SPECIALIZED_GETS_OR_NIL.size(); i++) {
+                    final int offset = i;
+
+                    defineMethod(SPECIALIZED_GETS_OR_NIL.get(offset), ACC_PUBLIC, sig(IRubyObject.class, IRubyObject.class), new CodeBlock() {{
+                        line(6);
+
+                        if (size <= offset) {
+                            invokestatic(clsPath, "sizeError", sig(RuntimeException.class));
+                            athrow();
+                        } else {
+                            aload(0);
+                            getfield(clsPath, newFields[offset], ci(IRubyObject.class));
+
+                            dup();
+
+                            LabelNode ok = new LabelNode(new Label());
+                            ifnonnull(ok);
+
+                            pop();
+
+                            aload(0);
+                            aload(1);
+                            putfield(clsPath, newFields[offset], ci(IRubyObject.class));
+                            aload(1);
+
+                            label(ok);
+                            areturn();
+                        }
+                    }});
+                }
 
                 for (int i = 0; i < SPECIALIZED_SETS.size(); i++) {
                     final int offset = i;