8066225: NPE in MethodEmitter with duplicate integer switch cases

Reviewed-by: hannesw, lagergren

nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/AssignSymbols.java

@@ -917,7 +917,7 @@ final class AssignSymbols extends NodeVisitor<LexicalContext> implements Loggabl
     @Override
     public Node leaveSwitchNode(final SwitchNode switchNode) {
         // We only need a symbol for the tag if it's not an integer switch node
-        if(!switchNode.isInteger()) {
+        if(!switchNode.isUniqueInteger()) {
             switchNode.setTag(newObjectInternal(SWITCH_TAG_PREFIX));
         }
         return switchNode;

nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/CodeGenerator.java

@@ -2817,7 +2817,7 @@ final class CodeGenerator extends NodeOperatorVisitor<CodeGeneratorLexicalContex
         Label defaultLabel = defaultCase != null ? defaultCase.getEntry() : breakLabel;
         final boolean hasSkipConversion = LocalVariableConversion.hasLiveConversion(switchNode);
 
-        if (switchNode.isInteger()) {
+        if (switchNode.isUniqueInteger()) {
             // Tree for sorting values.
             final TreeMap<Integer, Label> tree = new TreeMap<>();
 

nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/FoldConstants.java

@@ -26,12 +26,16 @@
 package jdk.nashorn.internal.codegen;
 
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 import jdk.nashorn.internal.codegen.types.Type;
 import jdk.nashorn.internal.ir.BinaryNode;
 import jdk.nashorn.internal.ir.Block;
 import jdk.nashorn.internal.ir.BlockStatement;
+import jdk.nashorn.internal.ir.CaseNode;
 import jdk.nashorn.internal.ir.EmptyNode;
+import jdk.nashorn.internal.ir.Expression;
 import jdk.nashorn.internal.ir.FunctionNode;
 import jdk.nashorn.internal.ir.FunctionNode.CompilationState;
 import jdk.nashorn.internal.ir.IfNode;
@@ -40,6 +44,7 @@ import jdk.nashorn.internal.ir.LiteralNode;
 import jdk.nashorn.internal.ir.LiteralNode.ArrayLiteralNode;
 import jdk.nashorn.internal.ir.Node;
 import jdk.nashorn.internal.ir.Statement;
+import jdk.nashorn.internal.ir.SwitchNode;
 import jdk.nashorn.internal.ir.TernaryNode;
 import jdk.nashorn.internal.ir.UnaryNode;
 import jdk.nashorn.internal.ir.VarNode;
@@ -131,6 +136,32 @@ final class FoldConstants extends NodeVisitor<LexicalContext> implements Loggabl
         return ternaryNode;
     }
 
+    @Override
+    public Node leaveSwitchNode(final SwitchNode switchNode) {
+        return switchNode.setUniqueInteger(lc, isUniqueIntegerSwitchNode(switchNode));
+    }
+
+    private static boolean isUniqueIntegerSwitchNode(final SwitchNode switchNode) {
+        final Set<Integer> alreadySeen = new HashSet<>();
+        for (final CaseNode caseNode : switchNode.getCases()) {
+            final Expression test = caseNode.getTest();
+            if (test != null && !isUniqueIntegerLiteral(test, alreadySeen)) {
+                return false;
+            }
+        }
+        return true;
+    }
+
+    private static boolean isUniqueIntegerLiteral(final Expression expr, final Set<Integer> alreadySeen) {
+        if (expr instanceof LiteralNode) {
+            final Object value = ((LiteralNode<?>)expr).getValue();
+            if (value instanceof Integer) {
+                return alreadySeen.add((Integer)value);
+            }
+        }
+        return false;
+    }
+
     /**
      * Helper class to evaluate constant expressions at compile time This is
      * also a simplifier used by BinaryNode visits, UnaryNode visits and

nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java

@@ -709,7 +709,7 @@ final class LocalVariableTypesCalculator extends NodeVisitor<LexicalContext>{
 
         // Control flow is different for all-integer cases where we dispatch by switch table, and for all other cases
         // where we do sequential comparison. Note that CaseNode objects act as join points.
-        final boolean isInteger = switchNode.isInteger();
+        final boolean isInteger = switchNode.isUniqueInteger();
         final Label breakLabel = switchNode.getBreakLabel();
         final boolean hasDefault = switchNode.getDefaultCase() != null;
 

nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/Lower.java

@@ -275,7 +275,7 @@ final class Lower extends NodeOperatorVisitor<BlockLexicalContext> implements Lo
 
     @Override
     public Node leaveSwitchNode(final SwitchNode switchNode) {
-        if(!switchNode.isInteger()) {
+        if(!switchNode.isUniqueInteger()) {
             // Wrap it in a block so its internally created tag is restricted in scope
             addStatementEnclosedInBlock(switchNode);
         } else {

nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/SwitchNode.java

@@ -48,6 +48,10 @@ public final class SwitchNode extends BreakableStatement {
     /** Switch default index. */
     private final int defaultCaseIndex;
 
+    /** True if all cases are 32-bit signed integer constants, without repetitions. It's a prerequisite for
+     * using a tableswitch/lookupswitch when generating code. */
+    private final boolean uniqueInteger;
+
     /** Tag symbol. */
     private Symbol tag;
 
@@ -66,15 +70,17 @@ public final class SwitchNode extends BreakableStatement {
         this.expression       = expression;
         this.cases            = cases;
         this.defaultCaseIndex = defaultCase == null ? -1 : cases.indexOf(defaultCase);
+        this.uniqueInteger    = false;
     }
 
     private SwitchNode(final SwitchNode switchNode, final Expression expression, final List<CaseNode> cases,
-            final int defaultCaseIndex, final LocalVariableConversion conversion) {
+            final int defaultCaseIndex, final LocalVariableConversion conversion, final boolean uniqueInteger) {
         super(switchNode, conversion);
         this.expression       = expression;
         this.cases            = cases;
         this.defaultCaseIndex = defaultCaseIndex;
-        this.tag              = switchNode.getTag(); //TODO are symbols inhereted as references?
+        this.tag              = switchNode.getTag(); //TODO are symbols inherited as references?
+        this.uniqueInteger    = uniqueInteger;
     }
 
     @Override
@@ -83,7 +89,7 @@ public final class SwitchNode extends BreakableStatement {
         for (final CaseNode caseNode : cases) {
             newCases.add(new CaseNode(caseNode, caseNode.getTest(), caseNode.getBody(), caseNode.getLocalVariableConversion()));
         }
-        return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, newCases, defaultCaseIndex, conversion));
+        return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, newCases, defaultCaseIndex, conversion, uniqueInteger));
     }
 
     @Override
@@ -151,7 +157,7 @@ public final class SwitchNode extends BreakableStatement {
         if (this.cases == cases) {
             return this;
         }
-        return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, cases, defaultCaseIndex, conversion));
+        return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, cases, defaultCaseIndex, conversion, uniqueInteger));
     }
 
     /**
@@ -183,7 +189,7 @@ public final class SwitchNode extends BreakableStatement {
         if (this.expression == expression) {
             return this;
         }
-        return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, cases, defaultCaseIndex, conversion));
+        return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, cases, defaultCaseIndex, conversion, uniqueInteger));
     }
 
     /**
@@ -205,25 +211,30 @@ public final class SwitchNode extends BreakableStatement {
     }
 
     /**
-     * Returns true if all cases of this switch statement are 32-bit signed integer constants.
+     * Returns true if all cases of this switch statement are 32-bit signed integer constants, without repetitions.
-     * @return true if all cases of this switch statement are 32-bit signed integer constants.
+     * @return true if all cases of this switch statement are 32-bit signed integer constants, without repetitions.
      */
-    public boolean isInteger() {
+    public boolean isUniqueInteger() {
-        for (final CaseNode caseNode : cases) {
+        return uniqueInteger;
-            final Expression test = caseNode.getTest();
+    }
-            if (test != null && !isIntegerLiteral(test)) {
+
-                return false;
+    /**
-            }
+     * Sets whether all cases of this switch statement are 32-bit signed integer constants, without repetitions.
+     * @param lc lexical context
+     * @param uniqueInteger if true, all cases of this switch statement have been determined to be 32-bit signed
+     * integer constants, without repetitions.
+     * @return this switch node, if the value didn't change, or a new switch node with the changed value
+     */
+    public SwitchNode setUniqueInteger(final LexicalContext lc, final boolean uniqueInteger) {
+        if(this.uniqueInteger == uniqueInteger) {
+            return this;
         }
-        return true;
+        return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, cases, defaultCaseIndex, conversion, uniqueInteger));
     }
 
     @Override
     JoinPredecessor setLocalVariableConversionChanged(final LexicalContext lc, final LocalVariableConversion conversion) {
-        return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, cases, defaultCaseIndex, conversion));
+        return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, cases, defaultCaseIndex, conversion, uniqueInteger));
     }
 
-    private static boolean isIntegerLiteral(final Expression expr) {
-        return expr instanceof LiteralNode && ((LiteralNode<?>)expr).getValue() instanceof Integer;
-    }
 }

nashorn/test/script/basic/JDK-8066225.js

@@ -0,0 +1,36 @@
+/*
+ * Copyright (c) 2014 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-8066225: NPE in MethodEmitter with duplicate integer switch cases
+ *
+ * @test
+ * @run
+ */
+
+(function (x){
+    switch(x) { 
+       case 44: for (var x in {}) {x}; print("1"); 
+       case 44: print("2");
+    }
+})(44);

nashorn/test/script/basic/JDK-8066225.js.EXPECTED

@@ -0,0 +1,2 @@
+1
+2