# HG changeset patch # User attila # Date 1478875851 -3600 # Node ID 10375379aeac05e4b4ab652a6bd7ed5d5e7e71f8 # Parent 4fb97fd731ebd8eca0e93ee0d44ef8c3208ecba0 8168373: don't emit conversions for symbols outside their lexical scope Reviewed-by: hannesw, sundar diff -r 4fb97fd731eb -r 10375379aeac nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java --- 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 origins = new LinkedList<>(); private Map types = Collections.emptyMap(); - void addOrigin(final JoinPredecessor originNode, final Map originTypes) { + void addOrigin(final JoinPredecessor originNode, final Map 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 cloneMap(final Map map) { - return (IdentityHashMap)((IdentityHashMap)map).clone(); + private static HashMap cloneMap(final Map map) { + return (HashMap)((HashMap)map).clone(); } private LocalVariableConversion createConversion(final Symbol symbol, final LvarType branchLvarType, final Map 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 getUnionTypes(final Map types1, final Map types2) { + private Map getUnionTypes(final Map types1, final Map 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 localVariableTypes = new IdentityHashMap<>(); + private Map localVariableTypes = Collections.emptyMap(); + // Set of symbols whose lexical scope has already ended. + private final Set invalidatedSymbols = new HashSet<>(); + // Stack for evaluated expression types. private final Deque 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 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() : cloneMap(localVariableTypes); + cloneOrNewLocalVariableTypes(); localVariableTypes.put(symbol, type); } + private void cloneOrNewLocalVariableTypes() { + localVariableTypes = localVariableTypes.isEmpty() ? new HashMap() : 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 diff -r 4fb97fd731eb -r 10375379aeac nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/debug/PrintVisitor.java --- 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(); diff -r 4fb97fd731eb -r 10375379aeac nashorn/test/script/basic/es6/JDK-8168373.js --- /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 +})()