8066227: CodeGenerator load unitialized slot
authorattila
Mon, 08 Dec 2014 15:14:11 +0100
changeset 27969 c03b64ccbd0f
parent 27968 52e31251a179
child 27970 7b0048b90967
8066227: CodeGenerator load unitialized slot Reviewed-by: hannesw, lagergren
nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/CodeGenerator.java
nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java
nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/BinaryNode.java
nashorn/test/script/basic/JDK-8066227.js
nashorn/test/script/basic/JDK-8066227.js.EXPECTED
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/CodeGenerator.java	Mon Dec 08 15:13:16 2014 +0100
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/CodeGenerator.java	Mon Dec 08 15:14:11 2014 +0100
@@ -3698,8 +3698,7 @@
             final Expression lhs = assignNode.lhs();
             final Expression rhs = assignNode.rhs();
             final Type widestOperationType = assignNode.getWidestOperationType();
-            final Type widest = assignNode.isTokenType(TokenType.ASSIGN_ADD) ? Type.OBJECT : widestOperationType;
-            final TypeBounds bounds = new TypeBounds(assignNode.getType(), widest);
+            final TypeBounds bounds = new TypeBounds(assignNode.getType(), widestOperationType);
             new OptimisticOperation(assignNode, bounds) {
                 @Override
                 void loadStack() {
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java	Mon Dec 08 15:13:16 2014 +0100
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java	Mon Dec 08 15:14:11 2014 +0100
@@ -28,6 +28,7 @@
 import static jdk.nashorn.internal.codegen.CompilerConstants.RETURN;
 import static jdk.nashorn.internal.ir.Expression.isAlwaysFalse;
 import static jdk.nashorn.internal.ir.Expression.isAlwaysTrue;
+
 import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -82,7 +83,6 @@
 import jdk.nashorn.internal.ir.VarNode;
 import jdk.nashorn.internal.ir.WhileNode;
 import jdk.nashorn.internal.ir.visitor.NodeVisitor;
-import jdk.nashorn.internal.parser.Token;
 import jdk.nashorn.internal.parser.TokenType;
 
 /**
@@ -398,48 +398,53 @@
 
     @Override
     public boolean enterBinaryNode(final BinaryNode binaryNode) {
+        // NOTE: regardless of operator's lexical associativity, lhs is always evaluated first.
         final Expression lhs = binaryNode.lhs();
-        final Expression rhs = binaryNode.rhs();
         final boolean isAssignment = binaryNode.isAssignment();
-
-        final TokenType tokenType = Token.descType(binaryNode.getToken());
-        if(tokenType.isLeftAssociative()) {
-            assert !isAssignment;
-            final boolean isLogical = binaryNode.isLogical();
-            final Label joinLabel = isLogical ? new Label("") : null;
-            lhs.accept(this);
-            if(isLogical) {
-                jumpToLabel((JoinPredecessor)lhs, joinLabel);
-            }
-            rhs.accept(this);
-            if(isLogical) {
-                jumpToLabel((JoinPredecessor)rhs, joinLabel);
-            }
-            joinOnLabel(joinLabel);
-        } else {
-            rhs.accept(this);
-            if(isAssignment) {
-                if(lhs instanceof BaseNode) {
-                    ((BaseNode)lhs).getBase().accept(this);
-                    if(lhs instanceof IndexNode) {
-                        ((IndexNode)lhs).getIndex().accept(this);
-                    } else {
-                        assert lhs instanceof AccessNode;
-                    }
+        LvarType lhsTypeOnLoad = null;
+        if(isAssignment) {
+            if(lhs instanceof BaseNode) {
+                ((BaseNode)lhs).getBase().accept(this);
+                if(lhs instanceof IndexNode) {
+                    ((IndexNode)lhs).getIndex().accept(this);
                 } else {
-                    assert lhs instanceof IdentNode;
-                    if(binaryNode.isSelfModifying()) {
-                        ((IdentNode)lhs).accept(this);
-                    }
+                    assert lhs instanceof AccessNode;
                 }
             } else {
-                lhs.accept(this);
+                assert lhs instanceof IdentNode;
+                if(binaryNode.isSelfModifying()) {
+                    final IdentNode ident = ((IdentNode)lhs);
+                    ident.accept(this);
+                    // Self-assignment can cause a change in the type of the variable. For purposes of evaluating
+                    // the type of the operation, we must use its type as it was when it was loaded. If we didn't
+                    // do this, some awkward expressions would end up being calculated incorrectly, e.g.
+                    // "var x; x += x = 0;". In this case we have undefined+int so the result type is double (NaN).
+                    // However, if we used the type of "x" on LHS after we evaluated RHS, we'd see int+int, so the
+                    // result type would be either optimistic int or pessimistic long, which would be wrong.
+                    lhsTypeOnLoad = getLocalVariableTypeIfBytecode(ident.getSymbol());
+                }
             }
+        } else {
+            lhs.accept(this);
         }
 
+        final boolean isLogical = binaryNode.isLogical();
+        assert !(isAssignment && isLogical); // there are no logical assignment operators in JS
+        final Label joinLabel = isLogical ? new Label("") : null;
+        if(isLogical) {
+            jumpToLabel((JoinPredecessor)lhs, joinLabel);
+        }
+
+        final Expression rhs = binaryNode.rhs();
+        rhs.accept(this);
+        if(isLogical) {
+            jumpToLabel((JoinPredecessor)rhs, joinLabel);
+        }
+        joinOnLabel(joinLabel);
+
         if(isAssignment && lhs instanceof IdentNode) {
             if(binaryNode.isSelfModifying()) {
-                onSelfAssignment((IdentNode)lhs, binaryNode);
+                onSelfAssignment((IdentNode)lhs, binaryNode, lhsTypeOnLoad);
             } else {
                 onAssignment((IdentNode)lhs, rhs);
             }
@@ -919,7 +924,8 @@
 
         if(unaryNode.isSelfModifying()) {
             if(expr instanceof IdentNode) {
-                onSelfAssignment((IdentNode)expr, unaryNode);
+                final IdentNode ident = (IdentNode)expr;
+                onSelfAssignment(ident, unaryNode, getLocalVariableTypeIfBytecode(ident.getSymbol()));
             }
         }
         return false;
@@ -973,12 +979,41 @@
         return types;
     }
 
+    /**
+     * Returns the current type of the local variable represented by the symbol. This is the most strict of all
+     * {@code getLocalVariableType*} methods, as it will throw an assertion if the type is null. Therefore, it is only
+     * safe to be invoked on symbols known to be bytecode locals, and only after they have been initialized.
+     * Regardless, it is recommended to use this method in majority of cases, as because of its strictness it is the
+     * best suited for catching missing type calculation bugs early.
+     * @param symbol a symbol representing a bytecode local variable.
+     * @return the current type of the local variable represented by the symbol
+     */
     private LvarType getLocalVariableType(final Symbol symbol) {
         final LvarType type = getLocalVariableTypeOrNull(symbol);
         assert type != null;
         return type;
     }
 
+    /**
+     * Gets the type for a local variable if it is a bytecode local, otherwise null. Can be used in circumstances where
+     * the type is irrelevant if the symbol is not a bytecode local. Note that for bytecode locals, it delegates to
+     * {@link #getLocalVariableType(Symbol)}, so it will still assert that the type for such variable is already
+     * defined (that is, not null).
+     * @param symbol the symbol representing the variable.
+     * @return the current variable type, if it is a bytecode local, otherwise null.
+     */
+    private LvarType getLocalVariableTypeIfBytecode(final Symbol symbol) {
+        return symbol.isBytecodeLocal() ? getLocalVariableType(symbol) : null;
+    }
+
+    /**
+     * Gets the type for a variable represented by a symbol, or null if the type is not know. This is the least strict
+     * of all local variable type getters, and as such its use is discouraged except in initialization scenarios (where
+     * a just-defined symbol might still be null).
+     * @param symbol the symbol
+     * @return the current type for the symbol, or null if the type is not known either because the symbol has not been
+     * initialized, or because the symbol does not represent a bytecode local variable.
+     */
     private LvarType getLocalVariableTypeOrNull(final Symbol symbol) {
         return localVariableTypes.get(symbol);
     }
@@ -1358,13 +1393,13 @@
         jumpToCatchBlock(identNode);
     }
 
-    private void onSelfAssignment(final IdentNode identNode, final Expression assignment) {
+    private void onSelfAssignment(final IdentNode identNode, final Expression assignment, final LvarType typeOnLoad) {
         final Symbol symbol = identNode.getSymbol();
         assert symbol != null : identNode.getName();
         if(!symbol.isBytecodeLocal()) {
             return;
         }
-        final LvarType type = toLvarType(getType(assignment));
+        final LvarType type = toLvarType(getType(assignment, symbol, typeOnLoad.type));
         // Self-assignment never produce either a boolean or undefined
         assert type != null && type != LvarType.UNDEFINED && type != LvarType.BOOLEAN;
         setType(symbol, type);
@@ -1445,13 +1480,24 @@
         symbolIsUsed(symbol, getLocalVariableType(symbol));
     }
 
+    /**
+     * Gets the type of the expression, dependent on the current types of the local variables.
+     *
+     * @param expr the expression
+     * @return the current type of the expression dependent on the current types of the local variables.
+     */
     private Type getType(final Expression expr) {
         return expr.getType(getSymbolToType());
     }
 
+    /**
+     * Returns a function object from symbols to their types, used by the expressions to evaluate their type.
+     * {@link BinaryNode} specifically uses identity of the function to cache type calculations. This method makes
+     * sure to return the same function object while the local variable types don't change, and create a new function
+     * object if the local variable types have been changed.
+     * @return a function object representing a mapping from symbols to their types.
+     */
     private Function<Symbol, Type> getSymbolToType() {
-        // BinaryNode uses identity of the function to cache type calculations. Therefore, we must use different
-        // function instances for different localVariableTypes instances.
         if(symbolToType.isStale()) {
             symbolToType = new SymbolToType();
         }
@@ -1469,4 +1515,41 @@
             return boundTypes != localVariableTypes;
         }
     }
+
+    /**
+     * Gets the type of the expression, dependent on the current types of the local variables and a single overridden
+     * symbol type. Used by type calculation on compound operators to ensure the type of the LHS at the time it was
+     * loaded (which can potentially be different after RHS evaluation, e.g. "var x; x += x = 0;") is preserved for
+     * the calculation.
+     *
+     * @param expr the expression
+     * @param overriddenSymbol the overridden symbol
+     * @param overriddenType the overridden type
+     * @return the current type of the expression dependent on the current types of the local variables and the single
+     * potentially overridden type.
+     */
+    private Type getType(final Expression expr, final Symbol overriddenSymbol, final Type overriddenType) {
+        return expr.getType(getSymbolToType(overriddenSymbol, overriddenType));
+    }
+
+    private Function<Symbol, Type> getSymbolToType(final Symbol overriddenSymbol, final Type overriddenType) {
+        return getLocalVariableType(overriddenSymbol).type == overriddenType ? getSymbolToType() :
+            new SymbolToTypeOverride(overriddenSymbol, overriddenType);
+    }
+
+    private class SymbolToTypeOverride implements Function<Symbol, Type> {
+        private final Function<Symbol, Type> originalSymbolToType = getSymbolToType();
+        private final Symbol overriddenSymbol;
+        private final Type overriddenType;
+
+        SymbolToTypeOverride(final Symbol overriddenSymbol, final Type overriddenType) {
+            this.overriddenSymbol = overriddenSymbol;
+            this.overriddenType = overriddenType;
+        }
+
+        @Override
+        public Type apply(final Symbol symbol) {
+            return symbol == overriddenSymbol ? overriddenType : originalSymbolToType.apply(symbol);
+        }
+    }
 }
--- a/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/BinaryNode.java	Mon Dec 08 15:13:16 2014 +0100
+++ b/nashorn/src/jdk.scripting.nashorn/share/classes/jdk/nashorn/internal/ir/BinaryNode.java	Mon Dec 08 15:14:11 2014 +0100
@@ -341,10 +341,7 @@
     @Override
     public Node accept(final NodeVisitor<? extends LexicalContext> visitor) {
         if (visitor.enterBinaryNode(this)) {
-            if(tokenType().isLeftAssociative()) {
-                return visitor.leaveBinaryNode(setLHS((Expression)lhs.accept(visitor)).setRHS((Expression)rhs.accept(visitor)));
-            }
-            return visitor.leaveBinaryNode(setRHS((Expression)rhs.accept(visitor)).setLHS((Expression)lhs.accept(visitor)));
+            return visitor.leaveBinaryNode(setLHS((Expression)lhs.accept(visitor)).setRHS((Expression)rhs.accept(visitor)));
         }
 
         return this;
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/nashorn/test/script/basic/JDK-8066227.js	Mon Dec 08 15:14:11 2014 +0100
@@ -0,0 +1,40 @@
+/*
+ * 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-8066227: CodeGenerator load unitialized slot
+ *
+ * @test
+ * @run
+ */
+
+print((function () { var x; (x += x = 0); return x; })());
+print((function () { var x; (x -= x = 0); return x; })());
+print((function () { var x; (x *= x = 0); return x; })());
+print((function () { var x; (x /= x = 0); return x; })());
+print((function () { var x; (x %= x = 0); return x; })());
+print((function () { var x; (x <<= x = 0); return x; })());
+print((function () { var x; (x >>= x = 0); return x; })());
+print((function () { var x; (x >>>= x = 0); return x; })());
+print((function () { var x; (x |= x = 0); return x; })());
+print((function () { var x; (x &= x = 0); return x; })());
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/nashorn/test/script/basic/JDK-8066227.js.EXPECTED	Mon Dec 08 15:14:11 2014 +0100
@@ -0,0 +1,10 @@
+NaN
+NaN
+NaN
+NaN
+NaN
+0
+0
+0
+0
+0