8168373: don't emit conversions for symbols outside their lexical scope
authorattila
Fri, 11 Nov 2016 15:50:51 +0100
changeset 41981 10375379aeac
parent 41980 4fb97fd731eb
child 41982 d7226731ac08
8168373: don't emit conversions for symbols outside their lexical scope Reviewed-by: hannesw, sundar
nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java
nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/debug/PrintVisitor.java
nashorn/test/script/basic/es6/JDK-8168373.js
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java	Fri Nov 11 05:11:55 2016 +0000
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java	Fri Nov 11 15:50:51 2016 +0100
@@ -33,6 +33,7 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Deque;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.IdentityHashMap;
 import java.util.Iterator;
@@ -122,9 +123,9 @@
         private final List<JumpOrigin> origins = new LinkedList<>();
         private Map<Symbol, LvarType> types = Collections.emptyMap();
 
-        void addOrigin(final JoinPredecessor originNode, final Map<Symbol, LvarType> originTypes) {
+        void addOrigin(final JoinPredecessor originNode, final Map<Symbol, LvarType> originTypes, final LocalVariableTypesCalculator calc) {
             origins.add(new JumpOrigin(originNode, originTypes));
-            this.types = getUnionTypes(this.types, originTypes);
+            this.types = calc.getUnionTypes(this.types, originTypes);
         }
     }
     private enum LvarType {
@@ -185,12 +186,15 @@
     }
 
     @SuppressWarnings("unchecked")
-    private static IdentityHashMap<Symbol, LvarType> cloneMap(final Map<Symbol, LvarType> map) {
-        return (IdentityHashMap<Symbol, LvarType>)((IdentityHashMap<?,?>)map).clone();
+    private static HashMap<Symbol, LvarType> cloneMap(final Map<Symbol, LvarType> map) {
+        return (HashMap<Symbol, LvarType>)((HashMap<?,?>)map).clone();
     }
 
     private LocalVariableConversion createConversion(final Symbol symbol, final LvarType branchLvarType,
             final Map<Symbol, LvarType> joinLvarTypes, final LocalVariableConversion next) {
+        if (invalidatedSymbols.contains(symbol)) {
+            return next;
+        }
         final LvarType targetType = joinLvarTypes.get(symbol);
         assert targetType != null;
         if(targetType == branchLvarType) {
@@ -208,7 +212,7 @@
         return new LocalVariableConversion(symbol, branchLvarType.type, targetType.type, next);
     }
 
-    private static Map<Symbol, LvarType> getUnionTypes(final Map<Symbol, LvarType> types1, final Map<Symbol, LvarType> types2) {
+    private Map<Symbol, LvarType> getUnionTypes(final Map<Symbol, LvarType> types1, final Map<Symbol, LvarType> types2) {
         if(types1 == types2 || types1.isEmpty()) {
             return types2;
         } else if(types2.isEmpty()) {
@@ -261,6 +265,11 @@
             final LvarType type2 = types2.get(symbol);
             union.put(symbol, widestLvarType(type1,  type2));
         }
+        // If the two sets of symbols differ, there's a good chance that some of
+        // symbols only appearing in one of the sets are lexically invalidated,
+        // so we remove them from further consideration.
+        // This is not strictly necessary, just a working set size optimization.
+        union.keySet().removeAll(invalidatedSymbols);
         return union;
     }
 
@@ -359,8 +368,6 @@
         if(t1.ordinal() < LvarType.INT.ordinal() || t2.ordinal() < LvarType.INT.ordinal()) {
             return LvarType.OBJECT;
         }
-        // NOTE: we allow "widening" of long to double even though it can lose precision. ECMAScript doesn't have an
-        // Int64 type anyway, so this loss of precision is actually more conformant to the specification...
         return LvarType.values()[Math.max(t1.ordinal(), t2.ordinal())];
     }
     private final Compiler compiler;
@@ -368,7 +375,10 @@
     // Local variable type mapping at the currently evaluated point. No map instance is ever modified; setLvarType() always
     // allocates a new map. Immutability of maps allows for cheap snapshots by just keeping the reference to the current
     // value.
-    private Map<Symbol, LvarType> localVariableTypes = new IdentityHashMap<>();
+    private Map<Symbol, LvarType> localVariableTypes = Collections.emptyMap();
+    // Set of symbols whose lexical scope has already ended.
+    private final Set<Symbol> invalidatedSymbols = new HashSet<>();
+
     // Stack for evaluated expression types.
     private final Deque<LvarType> typeStack = new ArrayDeque<>();
 
@@ -464,9 +474,19 @@
 
     @Override
     public boolean enterBlock(final Block block) {
+        boolean cloned = false;
         for(final Symbol symbol: block.getSymbols()) {
-            if(symbol.isBytecodeLocal() && getLocalVariableTypeOrNull(symbol) == null) {
-                setType(symbol, LvarType.UNDEFINED);
+            if(symbol.isBytecodeLocal()) {
+                if (getLocalVariableTypeOrNull(symbol) == null) {
+                    if (!cloned) {
+                        cloneOrNewLocalVariableTypes();
+                        cloned = true;
+                    }
+                    localVariableTypes.put(symbol, LvarType.UNDEFINED);
+                }
+                // In case we're repeating analysis of a lexical scope (e.g. it's in a loop),
+                // make sure all symbols lexically scoped by the block become valid again.
+                invalidatedSymbols.remove(symbol);
             }
         }
         return true;
@@ -1046,15 +1066,11 @@
             // throw an exception.
             reachable = true;
             catchBody.accept(this);
-            final Symbol exceptionSymbol = exception.getSymbol();
             if(reachable) {
-                localVariableTypes = cloneMap(localVariableTypes);
-                localVariableTypes.remove(exceptionSymbol);
                 jumpToLabel(catchBody, endLabel);
                 canExit = true;
             }
-            localVariableTypes = cloneMap(afterConditionTypes);
-            localVariableTypes.remove(exceptionSymbol);
+            localVariableTypes = afterConditionTypes;
         }
         // NOTE: if we had one or more conditional catch blocks with no unconditional catch block following them, then
         // there will be an unconditional rethrow, so the join point can never be reached from the last
@@ -1204,7 +1220,7 @@
     }
 
     private void jumpToLabel(final JoinPredecessor jumpOrigin, final Label label, final Map<Symbol, LvarType> types) {
-        getOrCreateJumpTarget(label).addOrigin(jumpOrigin, types);
+        getOrCreateJumpTarget(label).addOrigin(jumpOrigin, types, this);
     }
 
     @Override
@@ -1226,16 +1242,18 @@
 
         boolean cloned = false;
         for(final Symbol symbol: block.getSymbols()) {
-            // Undefine the symbol outside the block
-            if(localVariableTypes.containsKey(symbol)) {
-                if(!cloned) {
-                    localVariableTypes = cloneMap(localVariableTypes);
-                    cloned = true;
+            if(symbol.hasSlot()) {
+                // Invalidate the symbol when its defining block ends
+                if (symbol.isBytecodeLocal()) {
+                    if(localVariableTypes.containsKey(symbol)) {
+                        if(!cloned) {
+                            localVariableTypes = cloneMap(localVariableTypes);
+                            cloned = true;
+                        }
+                    }
+                    invalidateSymbol(symbol);
                 }
-                localVariableTypes.remove(symbol);
-            }
 
-            if(symbol.hasSlot()) {
                 final SymbolConversions conversions = symbolConversions.get(symbol);
                 if(conversions != null) {
                     // Potentially make some currently dead types live if they're needed as a source of a type
@@ -1605,10 +1623,19 @@
         }
         assert symbol.hasSlot();
         assert !symbol.isGlobal();
-        localVariableTypes = localVariableTypes.isEmpty() ? new IdentityHashMap<Symbol, LvarType>() : cloneMap(localVariableTypes);
+        cloneOrNewLocalVariableTypes();
         localVariableTypes.put(symbol, type);
     }
 
+    private void cloneOrNewLocalVariableTypes() {
+        localVariableTypes = localVariableTypes.isEmpty() ? new HashMap<Symbol, LvarType>() : cloneMap(localVariableTypes);
+    }
+
+    private void invalidateSymbol(final Symbol symbol) {
+        localVariableTypes.remove(symbol);
+        invalidatedSymbols.add(symbol);
+    }
+
     /**
      * Set a flag in the symbol marking it as needing to be able to store a value of a particular type. Every symbol for
      * a local variable will be assigned between 1 and 6 local variable slots for storing all types it is known to need
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/debug/PrintVisitor.java	Fri Nov 11 05:11:55 2016 +0000
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/debug/PrintVisitor.java	Fri Nov 11 15:50:51 2016 +0100
@@ -397,7 +397,7 @@
 
     @Override
     public boolean enterVarNode(final VarNode varNode) {
-        sb.append("var ");
+        sb.append(varNode.isConst() ? "const " : varNode.isLet() ? "let " : "var ");
         varNode.getName().toString(sb, printTypes);
         printLocalVariableConversion(varNode.getName());
         final Node init = varNode.getInit();
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/nashorn/test/script/basic/es6/JDK-8168373.js	Fri Nov 11 15:50:51 2016 +0100
@@ -0,0 +1,44 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ * 
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ * 
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ * 
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ * 
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * JDK-8168373: don't emit conversions for symbols outside their lexical scope
+ *
+ * @test
+ * @run
+ * @option --language=es6
+ */
+
+function p() { return false } // "predicate"
+function r(x) { return x } // "read"
+
+(function() {
+  try { // Try creates control flow edges from assignments into catch blocks.
+    // Lexically scoped, never read int variable (undefined at catch block) but still with a cf edge into catch block.
+    // Since it's never read, it's not written either (Nashorn optimizes some dead writes).
+    let x = 0; 
+    if (p()) { throw {}; } // We need `p()` so this block doesn't get optimized away, for possibility of a `throw` 
+    x = 0.0; // change the type of x to double
+    r(x); // read x otherwise it's optimized away
+  } catch (e) {} // under the bug, "throw" will try to widen unwritten int x to double for here and cause a verifier error
+})()