Fri, 11 Nov 2016 15:50:51 +0100
8168373: don't emit conversions for symbols outside their lexical scope
Reviewed-by: hannesw, sundar
1.1 --- a/src/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java Tue Oct 25 08:41:10 2016 -0700 1.2 +++ b/src/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java Fri Nov 11 15:50:51 2016 +0100 1.3 @@ -33,6 +33,7 @@ 1.4 import java.util.ArrayList; 1.5 import java.util.Collections; 1.6 import java.util.Deque; 1.7 +import java.util.HashMap; 1.8 import java.util.HashSet; 1.9 import java.util.IdentityHashMap; 1.10 import java.util.Iterator; 1.11 @@ -122,9 +123,9 @@ 1.12 private final List<JumpOrigin> origins = new LinkedList<>(); 1.13 private Map<Symbol, LvarType> types = Collections.emptyMap(); 1.14 1.15 - void addOrigin(final JoinPredecessor originNode, final Map<Symbol, LvarType> originTypes) { 1.16 + void addOrigin(final JoinPredecessor originNode, final Map<Symbol, LvarType> originTypes, final LocalVariableTypesCalculator calc) { 1.17 origins.add(new JumpOrigin(originNode, originTypes)); 1.18 - this.types = getUnionTypes(this.types, originTypes); 1.19 + this.types = calc.getUnionTypes(this.types, originTypes); 1.20 } 1.21 } 1.22 private enum LvarType { 1.23 @@ -185,12 +186,15 @@ 1.24 } 1.25 1.26 @SuppressWarnings("unchecked") 1.27 - private static IdentityHashMap<Symbol, LvarType> cloneMap(final Map<Symbol, LvarType> map) { 1.28 - return (IdentityHashMap<Symbol, LvarType>)((IdentityHashMap<?,?>)map).clone(); 1.29 + private static HashMap<Symbol, LvarType> cloneMap(final Map<Symbol, LvarType> map) { 1.30 + return (HashMap<Symbol, LvarType>)((HashMap<?,?>)map).clone(); 1.31 } 1.32 1.33 private LocalVariableConversion createConversion(final Symbol symbol, final LvarType branchLvarType, 1.34 final Map<Symbol, LvarType> joinLvarTypes, final LocalVariableConversion next) { 1.35 + if (invalidatedSymbols.contains(symbol)) { 1.36 + return next; 1.37 + } 1.38 final LvarType targetType = joinLvarTypes.get(symbol); 1.39 assert targetType != null; 1.40 if(targetType == branchLvarType) { 1.41 @@ -208,7 +212,7 @@ 1.42 return new LocalVariableConversion(symbol, branchLvarType.type, targetType.type, next); 1.43 } 1.44 1.45 - private static Map<Symbol, LvarType> getUnionTypes(final Map<Symbol, LvarType> types1, final Map<Symbol, LvarType> types2) { 1.46 + private Map<Symbol, LvarType> getUnionTypes(final Map<Symbol, LvarType> types1, final Map<Symbol, LvarType> types2) { 1.47 if(types1 == types2 || types1.isEmpty()) { 1.48 return types2; 1.49 } else if(types2.isEmpty()) { 1.50 @@ -261,6 +265,11 @@ 1.51 final LvarType type2 = types2.get(symbol); 1.52 union.put(symbol, widestLvarType(type1, type2)); 1.53 } 1.54 + // If the two sets of symbols differ, there's a good chance that some of 1.55 + // symbols only appearing in one of the sets are lexically invalidated, 1.56 + // so we remove them from further consideration. 1.57 + // This is not strictly necessary, just a working set size optimization. 1.58 + union.keySet().removeAll(invalidatedSymbols); 1.59 return union; 1.60 } 1.61 1.62 @@ -359,8 +368,6 @@ 1.63 if(t1.ordinal() < LvarType.INT.ordinal() || t2.ordinal() < LvarType.INT.ordinal()) { 1.64 return LvarType.OBJECT; 1.65 } 1.66 - // NOTE: we allow "widening" of long to double even though it can lose precision. ECMAScript doesn't have an 1.67 - // Int64 type anyway, so this loss of precision is actually more conformant to the specification... 1.68 return LvarType.values()[Math.max(t1.ordinal(), t2.ordinal())]; 1.69 } 1.70 private final Compiler compiler; 1.71 @@ -368,7 +375,10 @@ 1.72 // Local variable type mapping at the currently evaluated point. No map instance is ever modified; setLvarType() always 1.73 // allocates a new map. Immutability of maps allows for cheap snapshots by just keeping the reference to the current 1.74 // value. 1.75 - private Map<Symbol, LvarType> localVariableTypes = new IdentityHashMap<>(); 1.76 + private Map<Symbol, LvarType> localVariableTypes = Collections.emptyMap(); 1.77 + // Set of symbols whose lexical scope has already ended. 1.78 + private final Set<Symbol> invalidatedSymbols = new HashSet<>(); 1.79 + 1.80 // Stack for evaluated expression types. 1.81 private final Deque<LvarType> typeStack = new ArrayDeque<>(); 1.82 1.83 @@ -464,9 +474,19 @@ 1.84 1.85 @Override 1.86 public boolean enterBlock(final Block block) { 1.87 + boolean cloned = false; 1.88 for(final Symbol symbol: block.getSymbols()) { 1.89 - if(symbol.isBytecodeLocal() && getLocalVariableTypeOrNull(symbol) == null) { 1.90 - setType(symbol, LvarType.UNDEFINED); 1.91 + if(symbol.isBytecodeLocal()) { 1.92 + if (getLocalVariableTypeOrNull(symbol) == null) { 1.93 + if (!cloned) { 1.94 + cloneOrNewLocalVariableTypes(); 1.95 + cloned = true; 1.96 + } 1.97 + localVariableTypes.put(symbol, LvarType.UNDEFINED); 1.98 + } 1.99 + // In case we're repeating analysis of a lexical scope (e.g. it's in a loop), 1.100 + // make sure all symbols lexically scoped by the block become valid again. 1.101 + invalidatedSymbols.remove(symbol); 1.102 } 1.103 } 1.104 return true; 1.105 @@ -1046,15 +1066,11 @@ 1.106 // throw an exception. 1.107 reachable = true; 1.108 catchBody.accept(this); 1.109 - final Symbol exceptionSymbol = exception.getSymbol(); 1.110 if(reachable) { 1.111 - localVariableTypes = cloneMap(localVariableTypes); 1.112 - localVariableTypes.remove(exceptionSymbol); 1.113 jumpToLabel(catchBody, endLabel); 1.114 canExit = true; 1.115 } 1.116 - localVariableTypes = cloneMap(afterConditionTypes); 1.117 - localVariableTypes.remove(exceptionSymbol); 1.118 + localVariableTypes = afterConditionTypes; 1.119 } 1.120 // NOTE: if we had one or more conditional catch blocks with no unconditional catch block following them, then 1.121 // there will be an unconditional rethrow, so the join point can never be reached from the last 1.122 @@ -1204,7 +1220,7 @@ 1.123 } 1.124 1.125 private void jumpToLabel(final JoinPredecessor jumpOrigin, final Label label, final Map<Symbol, LvarType> types) { 1.126 - getOrCreateJumpTarget(label).addOrigin(jumpOrigin, types); 1.127 + getOrCreateJumpTarget(label).addOrigin(jumpOrigin, types, this); 1.128 } 1.129 1.130 @Override 1.131 @@ -1226,16 +1242,18 @@ 1.132 1.133 boolean cloned = false; 1.134 for(final Symbol symbol: block.getSymbols()) { 1.135 - // Undefine the symbol outside the block 1.136 - if(localVariableTypes.containsKey(symbol)) { 1.137 - if(!cloned) { 1.138 - localVariableTypes = cloneMap(localVariableTypes); 1.139 - cloned = true; 1.140 + if(symbol.hasSlot()) { 1.141 + // Invalidate the symbol when its defining block ends 1.142 + if (symbol.isBytecodeLocal()) { 1.143 + if(localVariableTypes.containsKey(symbol)) { 1.144 + if(!cloned) { 1.145 + localVariableTypes = cloneMap(localVariableTypes); 1.146 + cloned = true; 1.147 + } 1.148 + } 1.149 + invalidateSymbol(symbol); 1.150 } 1.151 - localVariableTypes.remove(symbol); 1.152 - } 1.153 1.154 - if(symbol.hasSlot()) { 1.155 final SymbolConversions conversions = symbolConversions.get(symbol); 1.156 if(conversions != null) { 1.157 // Potentially make some currently dead types live if they're needed as a source of a type 1.158 @@ -1605,10 +1623,19 @@ 1.159 } 1.160 assert symbol.hasSlot(); 1.161 assert !symbol.isGlobal(); 1.162 - localVariableTypes = localVariableTypes.isEmpty() ? new IdentityHashMap<Symbol, LvarType>() : cloneMap(localVariableTypes); 1.163 + cloneOrNewLocalVariableTypes(); 1.164 localVariableTypes.put(symbol, type); 1.165 } 1.166 1.167 + private void cloneOrNewLocalVariableTypes() { 1.168 + localVariableTypes = localVariableTypes.isEmpty() ? new HashMap<Symbol, LvarType>() : cloneMap(localVariableTypes); 1.169 + } 1.170 + 1.171 + private void invalidateSymbol(final Symbol symbol) { 1.172 + localVariableTypes.remove(symbol); 1.173 + invalidatedSymbols.add(symbol); 1.174 + } 1.175 + 1.176 /** 1.177 * Set a flag in the symbol marking it as needing to be able to store a value of a particular type. Every symbol for 1.178 * a local variable will be assigned between 1 and 6 local variable slots for storing all types it is known to need
2.1 --- a/src/jdk/nashorn/internal/ir/debug/PrintVisitor.java Tue Oct 25 08:41:10 2016 -0700 2.2 +++ b/src/jdk/nashorn/internal/ir/debug/PrintVisitor.java Fri Nov 11 15:50:51 2016 +0100 2.3 @@ -397,7 +397,7 @@ 2.4 2.5 @Override 2.6 public boolean enterVarNode(final VarNode varNode) { 2.7 - sb.append("var "); 2.8 + sb.append(varNode.isConst() ? "const " : varNode.isLet() ? "let " : "var "); 2.9 varNode.getName().toString(sb, printTypes); 2.10 printLocalVariableConversion(varNode.getName()); 2.11 final Node init = varNode.getInit();
3.1 --- /dev/null Thu Jan 01 00:00:00 1970 +0000 3.2 +++ b/test/script/basic/es6/JDK-8168373.js Fri Nov 11 15:50:51 2016 +0100 3.3 @@ -0,0 +1,44 @@ 3.4 +/* 3.5 + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. 3.6 + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. 3.7 + * 3.8 + * This code is free software; you can redistribute it and/or modify it 3.9 + * under the terms of the GNU General Public License version 2 only, as 3.10 + * published by the Free Software Foundation. 3.11 + * 3.12 + * This code is distributed in the hope that it will be useful, but WITHOUT 3.13 + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or 3.14 + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License 3.15 + * version 2 for more details (a copy is included in the LICENSE file that 3.16 + * accompanied this code). 3.17 + * 3.18 + * You should have received a copy of the GNU General Public License version 3.19 + * 2 along with this work; if not, write to the Free Software Foundation, 3.20 + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. 3.21 + * 3.22 + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA 3.23 + * or visit www.oracle.com if you need additional information or have any 3.24 + * questions. 3.25 + */ 3.26 + 3.27 +/** 3.28 + * JDK-8168373: don't emit conversions for symbols outside their lexical scope 3.29 + * 3.30 + * @test 3.31 + * @run 3.32 + * @option --language=es6 3.33 + */ 3.34 + 3.35 +function p() { return false } // "predicate" 3.36 +function r(x) { return x } // "read" 3.37 + 3.38 +(function() { 3.39 + try { // Try creates control flow edges from assignments into catch blocks. 3.40 + // Lexically scoped, never read int variable (undefined at catch block) but still with a cf edge into catch block. 3.41 + // Since it's never read, it's not written either (Nashorn optimizes some dead writes). 3.42 + let x = 0; 3.43 + if (p()) { throw {}; } // We need `p()` so this block doesn't get optimized away, for possibility of a `throw` 3.44 + x = 0.0; // change the type of x to double 3.45 + r(x); // read x otherwise it's optimized away 3.46 + } catch (e) {} // under the bug, "throw" will try to widen unwritten int x to double for here and cause a verifier error 3.47 +})()