8047357: More precise synthetic return + unreachable throw
authorattila
Thu, 26 Jun 2014 13:12:32 +0200
changeset 25244 627d7e86f3b5
parent 25243 7a1edca6ce94
child 25245 d1cd1305bcde
8047357: More precise synthetic return + unreachable throw Reviewed-by: lagergren, sundar
nashorn/src/jdk/nashorn/internal/codegen/AssignSymbols.java
nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java
nashorn/src/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java
nashorn/src/jdk/nashorn/internal/codegen/Lower.java
nashorn/src/jdk/nashorn/internal/ir/Block.java
nashorn/src/jdk/nashorn/internal/ir/CaseNode.java
nashorn/src/jdk/nashorn/internal/ir/ExpressionStatement.java
nashorn/src/jdk/nashorn/internal/ir/IdentNode.java
nashorn/src/jdk/nashorn/internal/ir/Node.java
nashorn/src/jdk/nashorn/internal/ir/Statement.java
nashorn/src/jdk/nashorn/internal/ir/Terminal.java
nashorn/src/jdk/nashorn/internal/ir/debug/ASTWriter.java
nashorn/src/jdk/nashorn/internal/ir/debug/PrintVisitor.java
nashorn/test/script/basic/JDK-8047057.js
nashorn/test/script/basic/JDK-8047357.js
nashorn/test/script/basic/JDK-8047357.js.EXPECTED
--- a/nashorn/src/jdk/nashorn/internal/codegen/AssignSymbols.java	Wed Jun 25 14:36:24 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/codegen/AssignSymbols.java	Thu Jun 26 13:12:32 2014 +0200
@@ -209,7 +209,7 @@
                     if (varNode.isFunctionDeclaration()) {
                         symbol.setIsFunctionDeclaration();
                     }
-                    return varNode.setName((IdentNode)ident.setSymbol(symbol));
+                    return varNode.setName(ident.setSymbol(symbol));
                 }
                 return varNode;
             }
@@ -217,7 +217,7 @@
     }
 
     private IdentNode compilerConstantIdentifier(final CompilerConstants cc) {
-        return (IdentNode)createImplicitIdentifier(cc.symbolName()).setSymbol(lc.getCurrentFunction().compilerConstant(cc));
+        return createImplicitIdentifier(cc.symbolName()).setSymbol(lc.getCurrentFunction().compilerConstant(cc));
     }
 
     /**
@@ -263,7 +263,7 @@
         final Symbol nameSymbol = fn.getBody().getExistingSymbol(name.getName());
         assert nameSymbol != null;
 
-        return (VarNode)synthVar.setName((IdentNode)name.setSymbol(nameSymbol)).accept(this);
+        return (VarNode)synthVar.setName(name.setSymbol(nameSymbol)).accept(this);
     }
 
     private FunctionNode createSyntheticInitializers(final FunctionNode functionNode) {
@@ -522,7 +522,7 @@
             final Symbol paramSymbol = body.getExistingSymbol(param.getName());
             assert paramSymbol != null;
             assert paramSymbol.isParam() : paramSymbol + " " + paramSymbol.getFlags();
-            newParams.add((IdentNode)param.setSymbol(paramSymbol));
+            newParams.add(param.setSymbol(paramSymbol));
 
             // parameters should not be slots for a function that uses variable arity signature
             if (isVarArg) {
--- a/nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java	Wed Jun 25 14:36:24 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java	Thu Jun 26 13:12:32 2014 +0200
@@ -1097,7 +1097,7 @@
 
         closeBlockVariables(block);
         lc.releaseSlots();
-        assert !method.isReachable() || lc.getUsedSlotCount() == method.getFirstTemp();
+        assert !method.isReachable() || (lc.isFunctionBody() ? 0 : lc.getUsedSlotCount()) == method.getFirstTemp();
 
         return block;
     }
--- a/nashorn/src/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java	Wed Jun 25 14:36:24 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java	Thu Jun 26 13:12:32 2014 +0200
@@ -25,10 +25,12 @@
 
 package jdk.nashorn.internal.codegen;
 
+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;
 import java.util.Deque;
 import java.util.HashSet;
@@ -39,7 +41,6 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.function.Function;
-
 import jdk.nashorn.internal.codegen.types.Type;
 import jdk.nashorn.internal.ir.AccessNode;
 import jdk.nashorn.internal.ir.BaseNode;
@@ -72,6 +73,7 @@
 import jdk.nashorn.internal.ir.RuntimeNode;
 import jdk.nashorn.internal.ir.RuntimeNode.Request;
 import jdk.nashorn.internal.ir.SplitNode;
+import jdk.nashorn.internal.ir.Statement;
 import jdk.nashorn.internal.ir.SwitchNode;
 import jdk.nashorn.internal.ir.Symbol;
 import jdk.nashorn.internal.ir.TernaryNode;
@@ -356,6 +358,8 @@
     private boolean reachable = true;
     // Return type of the function
     private Type returnType = Type.UNKNOWN;
+    // Synthetic return node that we must insert at the end of the function if it's end is reachable.
+    private ReturnNode syntheticReturn;
 
     // Topmost current split node (if any)
     private SplitNode topSplit;
@@ -845,6 +849,10 @@
 
     @Override
     public boolean enterThrowNode(final ThrowNode throwNode) {
+        if(!reachable) {
+            return false;
+        }
+
         throwNode.getExpression().accept(this);
         jumpToCatchBlock(throwNode);
         doesNotContinueSequentially();
@@ -1031,6 +1039,15 @@
     @Override
     public Node leaveBlock(final Block block) {
         if(lc.isFunctionBody()) {
+            if(reachable) {
+                // reachable==true means we can reach the end of the function without an explicit return statement. We
+                // need to insert a synthetic one then. This logic used to be in Lower.leaveBlock(), but Lower's
+                // reachability analysis (through Terminal.isTerminal() flags) is not precise enough so
+                // Lower$BlockLexicalContext.afterSetStatements will sometimes think the control flow terminates even
+                // when it didn't. Example: function() { switch((z)) { default: {break; } throw x; } }.
+                createSyntheticReturn(block);
+                assert !reachable;
+            }
             // We must calculate the return type here (and not in leaveFunctionNode) as it can affect the liveness of
             // the :return symbol and thus affect conversion type liveness calculations for it.
             calculateReturnType();
@@ -1089,6 +1106,23 @@
             retSymbol.setNeedsSlot(true);
         }
     }
+
+    private void createSyntheticReturn(final Block body) {
+        final FunctionNode functionNode = lc.getCurrentFunction();
+        final long token = functionNode.getToken();
+        final int finish = functionNode.getFinish();
+        final List<Statement> statements = body.getStatements();
+        final int lineNumber = statements.isEmpty() ? functionNode.getLineNumber() : statements.get(statements.size() - 1).getLineNumber();
+        final IdentNode returnExpr;
+        if(functionNode.isProgram()) {
+            returnExpr = new IdentNode(token, finish, RETURN.symbolName()).setSymbol(getCompilerConstantSymbol(functionNode, RETURN));
+        } else {
+            returnExpr = null;
+        }
+        syntheticReturn = new ReturnNode(lineNumber, token, finish, returnExpr);
+        syntheticReturn.accept(this);
+    }
+
     /**
      * Leave a breakable node. If there's a join point associated with its break label (meaning there was at least one
      * break statement to the end of the node), insert the join point into the flow.
@@ -1178,6 +1212,16 @@
             }
 
             @Override
+            public Node leaveBlock(final Block block) {
+                if(inOuterFunction && syntheticReturn != null && lc.isFunctionBody()) {
+                    final ArrayList<Statement> stmts = new ArrayList<>(block.getStatements());
+                    stmts.add((ReturnNode)syntheticReturn.accept(this));
+                    return block.setStatements(lc, stmts);
+                }
+                return super.leaveBlock(block);
+            }
+
+            @Override
             public Node leaveFunctionNode(final FunctionNode nestedFunctionNode) {
                 inOuterFunction = true;
                 final FunctionNode newNestedFunction = (FunctionNode)nestedFunctionNode.accept(
--- a/nashorn/src/jdk/nashorn/internal/codegen/Lower.java	Wed Jun 25 14:36:24 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/codegen/Lower.java	Thu Jun 26 13:12:32 2014 +0200
@@ -75,7 +75,6 @@
 import jdk.nashorn.internal.runtime.CodeInstaller;
 import jdk.nashorn.internal.runtime.Context;
 import jdk.nashorn.internal.runtime.JSType;
-import jdk.nashorn.internal.runtime.ScriptRuntime;
 import jdk.nashorn.internal.runtime.Source;
 import jdk.nashorn.internal.runtime.logging.DebugLogger;
 import jdk.nashorn.internal.runtime.logging.Loggable;
@@ -160,30 +159,6 @@
     }
 
     @Override
-    public Node leaveBlock(final Block block) {
-        //now we have committed the entire statement list to the block, but we need to truncate
-        //whatever is after the last terminal. block append won't append past it
-
-
-        if (lc.isFunctionBody()) {
-            final FunctionNode currentFunction = lc.getCurrentFunction();
-            final boolean isProgram = currentFunction.isProgram();
-            final Statement last = lc.getLastStatement();
-            final ReturnNode returnNode = new ReturnNode(
-                last == null ? currentFunction.getLineNumber() : last.getLineNumber(), //TODO?
-                currentFunction.getToken(),
-                currentFunction.getFinish(),
-                isProgram ?
-                    compilerConstant(RETURN) :
-                    LiteralNode.newInstance(block, ScriptRuntime.UNDEFINED));
-
-            returnNode.accept(this);
-        }
-
-        return block;
-    }
-
-    @Override
     public boolean enterBreakNode(final BreakNode breakNode) {
         addStatement(breakNode);
         return false;
--- a/nashorn/src/jdk/nashorn/internal/ir/Block.java	Wed Jun 25 14:36:24 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/ir/Block.java	Thu Jun 26 13:12:32 2014 +0200
@@ -41,7 +41,7 @@
  * IR representation for a list of statements.
  */
 @Immutable
-public class Block extends Node implements BreakableNode, Flags<Block> {
+public class Block extends Node implements BreakableNode, Terminal, Flags<Block> {
     /** List of statements */
     protected final List<Statement> statements;
 
@@ -231,6 +231,11 @@
         return flags;
     }
 
+    /**
+     * Is this a terminal block, i.e. does it end control flow like ending with a throw or return?
+     *
+     * @return true if this node statement is terminal
+     */
     @Override
     public boolean isTerminal() {
         return getFlag(IS_TERMINAL);
--- a/nashorn/src/jdk/nashorn/internal/ir/CaseNode.java	Wed Jun 25 14:36:24 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/ir/CaseNode.java	Thu Jun 26 13:12:32 2014 +0200
@@ -36,7 +36,7 @@
  * Case nodes are not BreakableNodes, but the SwitchNode is
  */
 @Immutable
-public final class CaseNode extends Node implements JoinPredecessor, Labels {
+public final class CaseNode extends Node implements JoinPredecessor, Labels, Terminal {
     /** Test expression. */
     private final Expression test;
 
@@ -77,6 +77,11 @@
         this.conversion = conversion;
     }
 
+    /**
+     * Is this a terminal case node, i.e. does it end control flow like having a throw or return?
+     *
+     * @return true if this node statement is terminal
+     */
     @Override
     public boolean isTerminal() {
         return body.isTerminal();
--- a/nashorn/src/jdk/nashorn/internal/ir/ExpressionStatement.java	Wed Jun 25 14:36:24 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/ir/ExpressionStatement.java	Thu Jun 26 13:12:32 2014 +0200
@@ -57,11 +57,6 @@
     }
 
     @Override
-    public boolean isTerminal() {
-        return expression.isTerminal();
-    }
-
-    @Override
     public Node accept(final NodeVisitor<? extends LexicalContext> visitor) {
         if (visitor.enterExpressionStatement(this)) {
             return visitor.leaveExpressionStatement(setExpression((Expression)expression.accept(visitor)));
--- a/nashorn/src/jdk/nashorn/internal/ir/IdentNode.java	Wed Jun 25 14:36:24 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/ir/IdentNode.java	Thu Jun 26 13:12:32 2014 +0200
@@ -110,7 +110,7 @@
      * @return a temporary identifier for the symbol.
      */
     public static IdentNode createInternalIdentifier(final Symbol symbol) {
-        return (IdentNode)new IdentNode(Token.toDesc(TokenType.IDENT, 0, 0), 0, symbol.getName()).setSymbol(symbol);
+        return new IdentNode(Token.toDesc(TokenType.IDENT, 0, 0), 0, symbol.getName()).setSymbol(symbol);
     }
 
     @Override
@@ -180,7 +180,7 @@
      * @param symbol the symbol
      * @return new node
      */
-    public Expression setSymbol(final Symbol symbol) {
+    public IdentNode setSymbol(final Symbol symbol) {
         if (this.symbol == symbol) {
             return this;
         }
--- a/nashorn/src/jdk/nashorn/internal/ir/Node.java	Wed Jun 25 14:36:24 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/ir/Node.java	Thu Jun 26 13:12:32 2014 +0200
@@ -144,15 +144,6 @@
     public abstract void toString(final StringBuilder sb, final boolean printType);
 
     /**
-     * Check if this node has terminal flags, i.e. ends or breaks control flow
-     *
-     * @return true if terminal
-     */
-    public boolean hasTerminalFlags() {
-        return isTerminal() || hasGoto();
-    }
-
-    /**
      * Get the finish position for this node in the source string
      * @return finish
      */
@@ -169,15 +160,6 @@
     }
 
     /**
-     * Check if this function repositions control flow with goto like
-     * semantics, for example {@link BreakNode} or a {@link ForNode} with no test
-     * @return true if node has goto semantics
-     */
-    public boolean hasGoto() {
-        return false;
-    }
-
-    /**
      * Get start position for node
      * @return start position
      */
@@ -249,16 +231,6 @@
         return token;
     }
 
-    /**
-     * Is this a terminal Node, i.e. does it end control flow like a throw or return
-     * expression does?
-     *
-     * @return true if this node is terminal
-     */
-    public boolean isTerminal() {
-        return false;
-    }
-
     //on change, we have to replace the entire list, that's we can't simple do ListIterator.set
     static <T extends Node> List<T> accept(final NodeVisitor<? extends LexicalContext> visitor, final Class<T> clazz, final List<T> list) {
         boolean changed = false;
--- a/nashorn/src/jdk/nashorn/internal/ir/Statement.java	Wed Jun 25 14:36:24 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/ir/Statement.java	Thu Jun 26 13:12:32 2014 +0200
@@ -30,7 +30,7 @@
  * made up of statements. The only node subclass that needs to keep token and
  * location information is the Statement
  */
-public abstract class Statement extends Node {
+public abstract class Statement extends Node implements Terminal {
 
     private final int lineNumber;
 
@@ -77,4 +77,32 @@
         return lineNumber;
     }
 
+    /**
+     * Is this a terminal statement, i.e. does it end control flow like a throw or return?
+     *
+     * @return true if this node statement is terminal
+     */
+    @Override
+    public boolean isTerminal() {
+        return false;
+    }
+
+    /**
+     * Check if this statement repositions control flow with goto like
+     * semantics, for example {@link BreakNode} or a {@link ForNode} with no test
+     * @return true if statement has goto semantics
+     */
+    public boolean hasGoto() {
+        return false;
+    }
+
+    /**
+     * Check if this statement has terminal flags, i.e. ends or breaks control flow
+     *
+     * @return true if has terminal flags
+     */
+    public final boolean hasTerminalFlags() {
+        return isTerminal() || hasGoto();
+    }
 }
+
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/nashorn/src/jdk/nashorn/internal/ir/Terminal.java	Thu Jun 26 13:12:32 2014 +0200
@@ -0,0 +1,37 @@
+/*
+ * Copyright (c) 2010, 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.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * 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.
+ */
+
+package jdk.nashorn.internal.ir;
+
+/**
+ * Interface for AST nodes that can have a flag determining if they can terminate function control flow.
+ */
+public interface Terminal {
+    /**
+     * Returns true if this AST node is (or contains) a statement that terminates function control flow.
+     * @return true if this AST node is (or contains) a statement that terminates function control flow.
+     */
+    public boolean isTerminal();
+}
--- a/nashorn/src/jdk/nashorn/internal/ir/debug/ASTWriter.java	Wed Jun 25 14:36:24 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/ir/debug/ASTWriter.java	Thu Jun 26 13:12:32 2014 +0200
@@ -38,7 +38,9 @@
 import jdk.nashorn.internal.ir.Expression;
 import jdk.nashorn.internal.ir.IdentNode;
 import jdk.nashorn.internal.ir.Node;
+import jdk.nashorn.internal.ir.Statement;
 import jdk.nashorn.internal.ir.Symbol;
+import jdk.nashorn.internal.ir.Terminal;
 import jdk.nashorn.internal.ir.TernaryNode;
 import jdk.nashorn.internal.ir.annotations.Ignore;
 import jdk.nashorn.internal.ir.annotations.Reference;
@@ -144,11 +146,11 @@
 
         String status = "";
 
-        if (node.isTerminal()) {
+        if (node instanceof Terminal && ((Terminal)node).isTerminal()) {
             status += " Terminal";
         }
 
-        if (node.hasGoto()) {
+        if (node instanceof Statement && ((Statement)node).hasGoto()) {
             status += " Goto ";
         }
 
--- a/nashorn/src/jdk/nashorn/internal/ir/debug/PrintVisitor.java	Wed Jun 25 14:36:24 2014 +0200
+++ b/nashorn/src/jdk/nashorn/internal/ir/debug/PrintVisitor.java	Thu Jun 26 13:12:32 2014 +0200
@@ -182,9 +182,9 @@
 
         final List<Statement> statements = block.getStatements();
 
-        for (final Node statement : statements) {
-            if (printLineNumbers && (statement instanceof Statement)) {
-                final int lineNumber = ((Statement)statement).getLineNumber();
+        for (final Statement statement : statements) {
+            if (printLineNumbers) {
+                final int lineNumber = statement.getLineNumber();
                 sb.append('\n');
                 if (lineNumber != lastLineNumber) {
                     indent();
@@ -196,10 +196,6 @@
 
             statement.accept(this);
 
-            if (statement instanceof FunctionNode) {
-                continue;
-            }
-
             int  lastIndex = sb.length() - 1;
             char lastChar  = sb.charAt(lastIndex);
             while (Character.isWhitespace(lastChar) && lastIndex >= 0) {
--- a/nashorn/test/script/basic/JDK-8047057.js	Wed Jun 25 14:36:24 2014 +0200
+++ b/nashorn/test/script/basic/JDK-8047057.js	Thu Jun 26 13:12:32 2014 +0200
@@ -45,8 +45,8 @@
     }
 }
 
-// makeFuncAndCall("switch(0) { default: {break;} return }");
-// makeFuncAndCall("L: { { break L; } return; }");
+makeFuncAndCall("switch(0) { default: {break;} return }");
+makeFuncAndCall("L: { { break L; } return; }");
 makeFuncAndCall("L: { while(0) break L; return; }");
 makeFuncExpectError("L: {while(0) break L; return [](); }", TypeError);
 // makeFuncAndCall("do with({}) break ; while(0);");
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/nashorn/test/script/basic/JDK-8047357.js	Thu Jun 26 13:12:32 2014 +0200
@@ -0,0 +1,32 @@
+/*
+ * Copyright (c) 2010, 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-8047357: More precise synthetic return + unreachable throw
+ *
+ * @test
+ * @run
+ */
+
+print((function() { switch(0) { default: {var x; break ; } throw x; } })());
+print((function() { switch(0) { default: {break;} return; } })());
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/nashorn/test/script/basic/JDK-8047357.js.EXPECTED	Thu Jun 26 13:12:32 2014 +0200
@@ -0,0 +1,2 @@
+undefined
+undefined