8041625: AccessorProperty currentType must only by Object.class when non-primitive, and scoping followup problem for lazily generated with bodies
authorlagergren
Fri, 02 May 2014 18:22:29 +0200
changeset 24749 1549c85f8200
parent 24748 69787da7c664
child 24750 01ea334ab39a
8041625: AccessorProperty currentType must only by Object.class when non-primitive, and scoping followup problem for lazily generated with bodies Reviewed-by: jlaskey, attila
nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java
nashorn/src/jdk/nashorn/internal/codegen/CodeGeneratorLexicalContext.java
nashorn/src/jdk/nashorn/internal/codegen/FindScopeDepths.java
nashorn/src/jdk/nashorn/internal/ir/FunctionNode.java
nashorn/src/jdk/nashorn/internal/runtime/AccessorProperty.java
nashorn/src/jdk/nashorn/internal/runtime/RecompilableScriptFunctionData.java
nashorn/src/jdk/nashorn/internal/runtime/ScriptFunctionData.java
nashorn/test/script/basic/run-octane.js
--- a/nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java	Tue Apr 29 16:00:53 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java	Fri May 02 18:22:29 2014 +0200
@@ -409,7 +409,7 @@
                 }
                 previousWasBlock = true;
             } else {
-                if (node instanceof WithNode && previousWasBlock || node instanceof FunctionNode && CodeGeneratorLexicalContext.isFunctionDynamicScope((FunctionNode)node)) {
+                if (node instanceof WithNode && previousWasBlock || node instanceof FunctionNode && ((FunctionNode)node).needsDynamicScope()) {
                     // If we hit a scope that can have symbols introduced into it at run time before finding the defining
                     // block, the symbol can't be fast scoped. A WithNode only counts if we've immediately seen a block
                     // before - its block. Otherwise, we are currently processing the WithNode's expression, and that's
--- a/nashorn/src/jdk/nashorn/internal/codegen/CodeGeneratorLexicalContext.java	Tue Apr 29 16:00:53 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/codegen/CodeGeneratorLexicalContext.java	Fri May 02 18:22:29 2014 +0200
@@ -31,6 +31,7 @@
 import java.util.Deque;
 import java.util.HashMap;
 import java.util.Map;
+
 import jdk.nashorn.internal.IntDeque;
 import jdk.nashorn.internal.codegen.types.Type;
 import jdk.nashorn.internal.ir.Block;
@@ -75,14 +76,20 @@
     /** size of next free slot vector */
     private int nextFreeSlotsSize;
 
+    private boolean isWithBoundary(final LexicalContextNode node) {
+        return node instanceof Block && !isEmpty() && peek() instanceof WithNode;
+    }
+
     @Override
     public <T extends LexicalContextNode> T push(final T node) {
-        if (isDynamicScopeBoundary(node)) {
-            ++dynamicScopeCount;
-        }
-        if(node instanceof FunctionNode) {
+        if (isWithBoundary(node)) {
+            dynamicScopeCount++;
+        } else if (node instanceof FunctionNode) {
+            if (((FunctionNode)node).inDynamicContext()) {
+                dynamicScopeCount++;
+            }
             splitNodes.push(0);
-        } else if(node instanceof SplitNode) {
+        } else if (node instanceof SplitNode) {
             enterSplitNode();
         }
         return super.push(node);
@@ -99,32 +106,22 @@
     @Override
     public <T extends LexicalContextNode> T pop(final T node) {
         final T popped = super.pop(node);
-        if (isDynamicScopeBoundary(popped)) {
-            --dynamicScopeCount;
-        }
-        if(node instanceof FunctionNode) {
+        if (isWithBoundary(node)) {
+            dynamicScopeCount--;
+            assert dynamicScopeCount >= 0;
+        } else if (node instanceof FunctionNode) {
+            if (((FunctionNode)node).inDynamicContext()) {
+                dynamicScopeCount--;
+                assert dynamicScopeCount >= 0;
+            }
             assert splitNodes.peek() == 0;
             splitNodes.pop();
-        } else if(node instanceof SplitNode) {
+        } else if (node instanceof SplitNode) {
             exitSplitNode();
         }
         return popped;
     }
 
-    private boolean isDynamicScopeBoundary(final LexicalContextNode node) {
-        if (node instanceof Block) {
-            // Block's immediate parent is a with node. Note we aren't testing for a WithNode, as that'd capture
-            // processing of WithNode.expression too, but it should be unaffected.
-            return !isEmpty() && peek() instanceof WithNode;
-        } else if (node instanceof FunctionNode) {
-            // Function has a direct eval in it (so a top-level "var ..." in the eval code can introduce a new
-            // variable into the function's scope), and it isn't strict (as evals in strict functions get an
-            // isolated scope).
-            return isFunctionDynamicScope((FunctionNode)node);
-        }
-        return false;
-    }
-
     boolean inDynamicScope() {
         return dynamicScopeCount > 0;
     }
@@ -133,10 +130,6 @@
         return !splitNodes.isEmpty() && splitNodes.peek() > 0;
     }
 
-    static boolean isFunctionDynamicScope(FunctionNode fn) {
-        return fn.hasEval() && !fn.isStrict();
-    }
-
     MethodEmitter pushMethodEmitter(final MethodEmitter newMethod) {
         methodEmitters.push(newMethod);
         return newMethod;
@@ -230,7 +223,7 @@
         return nextFreeSlots[nextFreeSlotsSize - 1];
     }
 
-    void releaseBlockSlots(boolean optimistic) {
+    void releaseBlockSlots(final boolean optimistic) {
         --nextFreeSlotsSize;
         if(optimistic) {
             slotTypesDescriptors.peek().setLength(nextFreeSlots[nextFreeSlotsSize]);
--- a/nashorn/src/jdk/nashorn/internal/codegen/FindScopeDepths.java	Tue Apr 29 16:00:53 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/codegen/FindScopeDepths.java	Fri May 02 18:22:29 2014 +0200
@@ -27,6 +27,7 @@
 
 import static jdk.nashorn.internal.codegen.ObjectClassGenerator.getClassName;
 import static jdk.nashorn.internal.codegen.ObjectClassGenerator.getPaddedFieldCount;
+import static jdk.nashorn.internal.runtime.logging.DebugLogger.quote;
 
 import java.util.HashMap;
 import java.util.HashSet;
@@ -37,6 +38,7 @@
 import jdk.nashorn.internal.ir.Block;
 import jdk.nashorn.internal.ir.Expression;
 import jdk.nashorn.internal.ir.FunctionNode;
+import jdk.nashorn.internal.ir.WithNode;
 import jdk.nashorn.internal.ir.FunctionNode.CompilationState;
 import jdk.nashorn.internal.ir.LexicalContext;
 import jdk.nashorn.internal.ir.Node;
@@ -63,9 +65,12 @@
     private final Map<Integer, Map<Integer, RecompilableScriptFunctionData>> fnIdToNestedFunctions = new HashMap<>();
     private final Map<Integer, Map<String, Integer>> externalSymbolDepths = new HashMap<>();
     private final Map<Integer, Set<String>> internalSymbols = new HashMap<>();
+    private final Set<Block> withBodies = new HashSet<>();
 
     private final DebugLogger log;
 
+    private int dynamicScopeCount;
+
     FindScopeDepths(final Compiler compiler) {
         super(new LexicalContext());
         this.compiler = compiler;
@@ -151,12 +156,24 @@
         return globalBlock;
     }
 
+    private static boolean isDynamicScopeBoundary(final FunctionNode fn) {
+        return fn.needsDynamicScope();
+    }
+
+    private boolean isDynamicScopeBoundary(final Block block) {
+        return withBodies.contains(block);
+    }
+
     @Override
     public boolean enterFunctionNode(final FunctionNode functionNode) {
         if (env.isOnDemandCompilation()) {
             return true;
         }
 
+        if (isDynamicScopeBoundary(functionNode)) {
+            increaseDynamicScopeCount(functionNode);
+        }
+
         final int fnId = functionNode.getId();
         Map<Integer, RecompilableScriptFunctionData> nestedFunctions = fnIdToNestedFunctions.get(fnId);
         if (nestedFunctions == null) {
@@ -170,13 +187,24 @@
     //external symbols hold the scope depth of sc11 from global at the start of the method
     @Override
     public Node leaveFunctionNode(final FunctionNode functionNode) {
-        final FunctionNode newFunctionNode = functionNode.setState(lc, CompilationState.SCOPE_DEPTHS_COMPUTED);
+        final String name = functionNode.getName();
+        FunctionNode newFunctionNode = functionNode.setState(lc, CompilationState.SCOPE_DEPTHS_COMPUTED);
+
         if (env.isOnDemandCompilation()) {
             final RecompilableScriptFunctionData data = env.getScriptFunctionData(newFunctionNode.getId());
             assert data != null : newFunctionNode.getName() + " lacks data";
+            if (data.inDynamicContext()) {
+                log.fine("Reviving scriptfunction ", quote(name), " as defined in previous (now lost) dynamic scope.");
+                newFunctionNode = newFunctionNode.setInDynamicContext(lc);
+            }
             return newFunctionNode;
         }
 
+        if (inDynamicScope()) {
+            log.fine("Tagging ", quote(name), " as defined in dynamic scope");
+            newFunctionNode = newFunctionNode.setInDynamicContext(lc);
+        }
+
         //create recompilable scriptfunctiondata
         final int fnId = newFunctionNode.getId();
         final Map<Integer, RecompilableScriptFunctionData> nestedFunctions = fnIdToNestedFunctions.get(fnId);
@@ -207,15 +235,49 @@
             env.setData(data);
         }
 
+        if (isDynamicScopeBoundary(functionNode)) {
+            decreaseDynamicScopeCount(functionNode);
+        }
+
         return newFunctionNode;
     }
 
+    private boolean inDynamicScope() {
+        return dynamicScopeCount > 0;
+    }
+
+    private void increaseDynamicScopeCount(final Node node) {
+        assert dynamicScopeCount >= 0;
+        ++dynamicScopeCount;
+        if (log.isEnabled()) {
+            log.finest(quote(lc.getCurrentFunction().getName()), " ++dynamicScopeCount = ", dynamicScopeCount, " at: ", node, node.getClass());
+        }
+    }
+
+    private void decreaseDynamicScopeCount(final Node node) {
+        --dynamicScopeCount;
+        assert dynamicScopeCount >= 0;
+        if (log.isEnabled()) {
+            log.finest(quote(lc.getCurrentFunction().getName()), " --dynamicScopeCount = ", dynamicScopeCount, " at: ", node, node.getClass());
+        }
+    }
+
+    @Override
+    public boolean enterWithNode(final WithNode node) {
+        withBodies.add(node.getBody());
+        return true;
+    }
+
     @Override
     public boolean enterBlock(final Block block) {
         if (env.isOnDemandCompilation()) {
             return true;
         }
 
+        if (isDynamicScopeBoundary(block)) {
+            increaseDynamicScopeCount(block);
+        }
+
         if (!lc.isFunctionBody()) {
             return true;
         }
@@ -288,6 +350,17 @@
         return true;
     }
 
+    @Override
+    public Node leaveBlock(final Block block) {
+        if (env.isOnDemandCompilation()) {
+            return block;
+        }
+        if (isDynamicScopeBoundary(block)) {
+            decreaseDynamicScopeCount(block);
+        }
+        return block;
+    }
+
     private void addInternalSymbols(final FunctionNode functionNode, final Set<String> symbols) {
         final int fnId = functionNode.getId();
         assert internalSymbols.get(fnId) == null || internalSymbols.get(fnId).equals(symbols); //e.g. cloned finally block
@@ -303,4 +376,5 @@
         }
         depths.put(symbol.getName(), depthAtStart);
     }
+
 }
--- a/nashorn/src/jdk/nashorn/internal/ir/FunctionNode.java	Tue Apr 29 16:00:53 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/ir/FunctionNode.java	Fri May 02 18:22:29 2014 +0200
@@ -209,7 +209,10 @@
     public static final int IS_PROGRAM = 1 << 15;
 
     /** Does this function use the "this" keyword? */
-    public static final int USES_THIS                   = 1 << 16;
+    public static final int USES_THIS = 1 << 16;
+
+    /** Is this declared in a dynamic context */
+    public static final int IN_DYNAMIC_CONTEXT = 1 << 17;
 
     /** Does this function or any nested functions contain an eval? */
     private static final int HAS_DEEP_EVAL = HAS_EVAL | HAS_NESTED_EVAL;
@@ -649,6 +652,36 @@
     }
 
     /**
+     * Was this function declared in a dynamic context, i.e. in a with or eval style
+     * chain
+     * @return true if in dynamic context
+     */
+    public boolean inDynamicContext() {
+        return getFlag(IN_DYNAMIC_CONTEXT);
+    }
+
+    /**
+     * Check whether a function would need dynamic scope, which is does if it has
+     * evals and isn't strict.
+     * @return true if dynamic scope is needed
+     */
+    public boolean needsDynamicScope() {
+        // Function has a direct eval in it (so a top-level "var ..." in the eval code can introduce a new
+        // variable into the function's scope), and it isn't strict (as evals in strict functions get an
+        // isolated scope).
+        return hasEval() && !isStrict();
+    }
+
+    /**
+     * Flag this function as declared in a dynamic context
+     * @param lc lexical context
+     * @return new function node, or same if unmodified
+     */
+    public FunctionNode setInDynamicContext(final LexicalContext lc) {
+        return setFlag(lc, IN_DYNAMIC_CONTEXT);
+    }
+
+    /**
      * Returns true if this function needs to have an Arguments object defined as a local variable named "arguments".
      * Functions that use "arguments" as identifier and don't define it as a name of a parameter or a nested function
      * (see ECMAScript 5.1 Chapter 10.5), as well as any function that uses eval or with, or has a nested function that
--- a/nashorn/src/jdk/nashorn/internal/runtime/AccessorProperty.java	Tue Apr 29 16:00:53 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/runtime/AccessorProperty.java	Fri May 02 18:22:29 2014 +0200
@@ -631,7 +631,7 @@
                  mh = ObjectClassGenerator.createGuardBoxedPrimitiveSetter(ct, generateSetter(ct, ct), mh);
             }
         } else {
-            mh = generateSetter(forType, type);
+            mh = generateSetter(!forType.isPrimitive() ? Object.class : forType, type);
         }
 
         /**
@@ -681,7 +681,7 @@
     @Override
     public final void setCurrentType(final Class<?> currentType) {
         assert currentType != boolean.class : "no boolean storage support yet - fix this";
-        this.currentType = currentType;
+        this.currentType = currentType == null ? null : currentType.isPrimitive() ? currentType : Object.class;
     }
 
     @Override
--- a/nashorn/src/jdk/nashorn/internal/runtime/RecompilableScriptFunctionData.java	Tue Apr 29 16:00:53 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/runtime/RecompilableScriptFunctionData.java	Fri May 02 18:22:29 2014 +0200
@@ -269,6 +269,11 @@
         return functionName;
     }
 
+    @Override
+    public boolean inDynamicContext() {
+        return (flags & IN_DYNAMIC_CONTEXT) != 0;
+    }
+
     private static String functionName(final FunctionNode fn) {
         if (fn.isAnonymous()) {
             return "";
@@ -304,6 +309,9 @@
         if (functionNode.isVarArg()) {
             flags |= IS_VARIABLE_ARITY;
         }
+        if (functionNode.inDynamicContext()) {
+            flags |= IN_DYNAMIC_CONTEXT;
+        }
         return flags;
     }
 
@@ -346,6 +354,7 @@
         if (isAnonymous) {
             parser.setFunctionName(functionName);
         }
+
         final FunctionNode program = parser.parse(scriptName, descPosition, Token.descLength(token), true);
         // Parser generates a program AST even if we're recompiling a single function, so when we are only recompiling a
         // single function, extract it from the program.
@@ -373,7 +382,7 @@
         return sb.toString();
     }
 
-    FunctionNodeTransform getNopTransform() {
+    private FunctionNodeTransform getNopTransform() {
         return new FunctionNodeTransform() {
             @Override
             FunctionNode apply(final FunctionNode functionNode) {
--- a/nashorn/src/jdk/nashorn/internal/runtime/ScriptFunctionData.java	Tue Apr 29 16:00:53 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/runtime/ScriptFunctionData.java	Fri May 02 18:22:29 2014 +0200
@@ -85,6 +85,8 @@
     public static final int USES_THIS      = 1 << 4;
     /** Is this a variable arity function? */
     public static final int IS_VARIABLE_ARITY = 1 << 5;
+    /** Is this declared in a dynamic context */
+    public static final int IN_DYNAMIC_CONTEXT = 1 << 6;
 
     /** Flag for strict or built-in functions */
     public static final int IS_STRICT_OR_BUILTIN = IS_STRICT | IS_BUILTIN;
@@ -748,6 +750,14 @@
         return type.parameterType(type.parameterCount() - 1).isArray();
     }
 
+    /**
+     * Is this ScriptFunction declared in a dynamic context
+     * @return true if in dynamic context, false if not or irrelevant
+     */
+    public boolean inDynamicContext() {
+        return false;
+    }
+
     @SuppressWarnings("unused")
     private static Object[] bindVarArgs(final Object[] array1, final Object[] array2) {
         if (array2 == null) {
--- a/nashorn/test/script/basic/run-octane.js	Tue Apr 29 16:00:53 2014 +0200
+++ b/nashorn/test/script/basic/run-octane.js	Fri May 02 18:22:29 2014 +0200
@@ -25,7 +25,13 @@
  * @subtest
  */
 
-var read = readFully;
+var read;
+try {
+    read = readFully;
+} catch (e) {
+    print("ABORTING: Cannot find 'readFully'. You must have scripting enabled to use this test harness. (-scripting)");
+    throw e;
+}
 
 function initZlib() {
     zlib = new BenchmarkSuite('zlib', [152815148], [