8168373: don't emit conversions for symbols outside their lexical scope

Fri, 11 Nov 2016 15:50:51 +0100

author
attila
date
Fri, 11 Nov 2016 15:50:51 +0100
changeset 1980
ee3a76a1dbf2
parent 1978
91d33aea2714
child 1981
00ab24e0ebc5

8168373: don't emit conversions for symbols outside their lexical scope
Reviewed-by: hannesw, sundar

src/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java file | annotate | diff | comparison | revisions
src/jdk/nashorn/internal/ir/debug/PrintVisitor.java file | annotate | diff | comparison | revisions
test/script/basic/es6/JDK-8168373.js file | annotate | diff | comparison | revisions
     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 +})()

mercurial