8047357: More precise synthetic return + unreachable throw
Reviewed-by: lagergren, sundar
--- 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