Code generation patches for fuzzer (feedback requested)
André Bargull
andrebargull at googlemail.com
Tue Oct 15 01:45:52 PDT 2013
I've needed to apply the following patches to continue fuzzing. If those
changes look reasonable, can someone make sure they get into the
upstream repo?
- André
(1) IncompatibleAssignmentCoercion.patch:
`Type.widest(Type, Type)` can promote boolean -> number, or number ->
string. This is not wanted for (?:) - ConditionalExpressions or
ReturnStatements. Added a new uncoercedWidest(Type, Type) method to Attr
which makes sure only valid promotions are applied.
test cases:
/* no verify error */ function f1(c,x){ c (x ? [1] : 1); }
function f2(c,x){ c = (x ? "123" : 1); return c }
typeof f2(null,true) === "string"
typeof f2(null,false) === "number"
function f3(v){if (v) return 123; else return true}
f3(true) === 123
f3(false) === true
(2) JDK-8026249-WidenDISCARD.patch:
The left-hand side of a BinaryNode does not have a Type if it is a
Discard node, that means `lhs.getType()` may throw an assertion error.
Moving the Type.widest() calls makes sure `lhs.getType()` is only called
when it is safe to do.
test case:
/* no assertion */ function f() { if(x3, y) x; }
(3) JDK-8026249-EnsureTypeDiscard.patch:
The type for a CommaRight BinaryNode is based on the right-hand side's
type, make sure the RHS always has a proper type attached. [I've also
fixed this for CommaLeft, but that one isn't actually used, is it?]
test case:
/* no assertion */ function f(x) { return y, x }
(4) JDK-8026249-ControlFlowEscape.patch:
Control flow may escape from SwitchNode or LabelNode through `break` or
`continue` statements. Simply moving the "controlFlowEscapes" logic from
LoopNode to BreakableStatement solves the SwitchNode issue. And for
LabelNode just clear the "terminal" on its body node.
test cases:
/* no verify error / NPE */ function f() { L: { try { return 1; }
finally { break L; }} }
/* no verify error / NPE */ function f() { L: { if (v) break L; return} }
/* no verify error / NPE */ function f() { L: {{ break L; } return; } }
/* no verify error / NPE */ function f() { L: { if (x2) { break L; }
throw x; } }
/* no verify error / NPE */ function f() { switch(v) { default: if(v)
break; return; } }
/* no verify error / NPE */ function f() { switch(x) { default: if(true)
break; return; } }
/* no verify error / NPE */ function f() { switch(x) { default: L:
break; return; } }
-------------- next part --------------
diff --git a/src/jdk/nashorn/internal/codegen/Attr.java b/src/jdk/nashorn/internal/codegen/Attr.java
--- a/src/jdk/nashorn/internal/codegen/Attr.java
+++ b/src/jdk/nashorn/internal/codegen/Attr.java
@@ -1264,11 +1264,13 @@
@Override
public Node leaveCOMMARIGHT(final BinaryNode binaryNode) {
+ ensureTypeNotUnknown(binaryNode.rhs());
return end(ensureSymbol(binaryNode.rhs().getType(), binaryNode));
}
@Override
public Node leaveCOMMALEFT(final BinaryNode binaryNode) {
+ ensureTypeNotUnknown(binaryNode.lhs());
return end(ensureSymbol(binaryNode.lhs().getType(), binaryNode));
}
-------------- next part --------------
diff --git a/src/jdk/nashorn/internal/codegen/BranchOptimizer.java b/src/jdk/nashorn/internal/codegen/BranchOptimizer.java
--- a/src/jdk/nashorn/internal/codegen/BranchOptimizer.java
+++ b/src/jdk/nashorn/internal/codegen/BranchOptimizer.java
@@ -84,7 +84,6 @@
private void branchOptimizer(final BinaryNode binaryNode, final Label label, final boolean state) {
final Expression lhs = binaryNode.lhs();
final Expression rhs = binaryNode.rhs();
- Type widest = Type.widest(lhs.getType(), rhs.getType());
switch (binaryNode.tokenType()) {
case AND:
@@ -113,33 +112,33 @@
case EQ:
case EQ_STRICT:
- codegen.loadBinaryOperands(lhs, rhs, widest);
+ codegen.loadBinaryOperands(lhs, rhs, Type.widest(lhs.getType(), rhs.getType()));
method.conditionalJump(state ? EQ : NE, true, label);
return;
case NE:
case NE_STRICT:
- codegen.loadBinaryOperands(lhs, rhs, widest);
+ codegen.loadBinaryOperands(lhs, rhs, Type.widest(lhs.getType(), rhs.getType()));
method.conditionalJump(state ? NE : EQ, true, label);
return;
case GE:
- codegen.loadBinaryOperands(lhs, rhs, widest);
+ codegen.loadBinaryOperands(lhs, rhs, Type.widest(lhs.getType(), rhs.getType()));
method.conditionalJump(state ? GE : LT, !state, label);
return;
case GT:
- codegen.loadBinaryOperands(lhs, rhs, widest);
+ codegen.loadBinaryOperands(lhs, rhs, Type.widest(lhs.getType(), rhs.getType()));
method.conditionalJump(state ? GT : LE, !state, label);
return;
case LE:
- codegen.loadBinaryOperands(lhs, rhs, widest);
+ codegen.loadBinaryOperands(lhs, rhs, Type.widest(lhs.getType(), rhs.getType()));
method.conditionalJump(state ? LE : GT, state, label);
return;
case LT:
- codegen.loadBinaryOperands(lhs, rhs, widest);
+ codegen.loadBinaryOperands(lhs, rhs, Type.widest(lhs.getType(), rhs.getType()));
method.conditionalJump(state ? LT : GE, state, label);
return;
-------------- next part --------------
diff --git a/src/jdk/nashorn/internal/codegen/Attr.java b/src/jdk/nashorn/internal/codegen/Attr.java
--- a/src/jdk/nashorn/internal/codegen/Attr.java
+++ b/src/jdk/nashorn/internal/codegen/Attr.java
@@ -765,7 +765,7 @@
symbol.setType(Type.OBJECT);
}
- returnType = Type.widest(returnTypes.pop(), symbol.getSymbolType());
+ returnType = uncoercedWidest(returnTypes.pop(), symbol.getSymbolType());
} else {
returnType = Type.OBJECT; //undefined
}
@@ -1427,7 +1427,7 @@
ensureTypeNotUnknown(trueExpr);
ensureTypeNotUnknown(falseExpr);
- final Type type = Type.widest(trueExpr.getType(), falseExpr.getType());
+ final Type type = uncoercedWidest(trueExpr.getType(), falseExpr.getType());
return end(ensureSymbol(type, ternaryNode));
}
@@ -1603,6 +1603,34 @@
}
/**
+ * Returns the widest or most common of two types without applying type
+ * coercion, e.g. coercion from boolean -> int, or int -> string is not
+ * allowed.
+ *
+ * @param type0 type one
+ * @param type1 type two
+ *
+ * @return the widest type
+ */
+ private static Type uncoercedWidest(final Type type0, final Type type1) {
+ final boolean canWiden;
+ if (type0.isUnknown() || type1.isUnknown()) {
+ canWiden = true;
+ } else if (type0.isNumeric() || type1.isNumeric()) {
+ canWiden = type0.isNumeric() && type1.isNumeric();
+ } else if (type0.isBoolean() || type1.isBoolean()) {
+ canWiden = type0.isBoolean() && type1.isBoolean();
+ } else {
+ assert type0.isObject() && type1.isObject();
+ canWiden = true;
+ }
+ if (canWiden) {
+ return Type.widest(type0, type1);
+ }
+ return Type.OBJECT;
+ }
+
+ /**
* If types have changed, we can have failed to update vars. For example
*
* var x = 17; //x is int
-------------- next part --------------
diff --git a/src/jdk/nashorn/internal/codegen/Lower.java b/src/jdk/nashorn/internal/codegen/Lower.java
--- a/src/jdk/nashorn/internal/codegen/Lower.java
+++ b/src/jdk/nashorn/internal/codegen/Lower.java
@@ -48,6 +48,7 @@
import jdk.nashorn.internal.ir.ForNode;
import jdk.nashorn.internal.ir.FunctionNode;
import jdk.nashorn.internal.ir.FunctionNode.CompilationState;
+import jdk.nashorn.internal.ir.CaseNode;
import jdk.nashorn.internal.ir.IdentNode;
import jdk.nashorn.internal.ir.IfNode;
import jdk.nashorn.internal.ir.LabelNode;
@@ -255,7 +256,7 @@
@Override
public Node leaveLabelNode(final LabelNode labelNode) {
- return addStatement(labelNode);
+ return addStatement(checkEscape(labelNode));
}
@Override
@@ -267,7 +268,7 @@
@Override
public Node leaveSwitchNode(final SwitchNode switchNode) {
- return addStatement(switchNode);
+ return addStatement(checkEscape(switchNode));
}
@Override
@@ -627,6 +628,24 @@
return loopNode;
}
+ private SwitchNode checkEscape(final SwitchNode switchNode) {
+ for (final CaseNode caseNode : switchNode.getCases()) {
+ final boolean escapes = controlFlowEscapes(lc, caseNode.getBody());
+ if (escapes) {
+ return switchNode.setControlFlowEscapes(lc, escapes);
+ }
+ }
+ return switchNode;
+ }
+
+ private LabelNode checkEscape(final LabelNode labelNode) {
+ final boolean escapes = controlFlowEscapes(lc, labelNode.getBody());
+ if (escapes) {
+ return labelNode.
+ setBody(lc, labelNode.getBody().setIsTerminal(lc, false));
+ }
+ return labelNode;
+ }
private Node addStatement(final Statement statement) {
lc.appendStatement(statement);
diff --git a/src/jdk/nashorn/internal/ir/BreakableStatement.java b/src/jdk/nashorn/internal/ir/BreakableStatement.java
--- a/src/jdk/nashorn/internal/ir/BreakableStatement.java
+++ b/src/jdk/nashorn/internal/ir/BreakableStatement.java
@@ -36,6 +36,9 @@
/** break label. */
protected final Label breakLabel;
+ /** Can control flow escape from statement, e.g. through breaks or continues to outer loops? */
+ protected final boolean controlFlowEscapes;
+
/**
* Constructor
*
@@ -44,9 +47,10 @@
* @param finish finish
* @param breakLabel break label
*/
- protected BreakableStatement(final int lineNumber, final long token, final int finish, final Label breakLabel) {
+ protected BreakableStatement(final int lineNumber, final long token, final int finish, final Label breakLabel, final boolean controlFlowEscapes) {
super(lineNumber, token, finish);
this.breakLabel = breakLabel;
+ this.controlFlowEscapes = controlFlowEscapes;
}
/**
@@ -54,9 +58,10 @@
*
* @param breakableNode source node
*/
- protected BreakableStatement(final BreakableStatement breakableNode) {
+ protected BreakableStatement(final BreakableStatement breakableNode, final boolean controlFlowEscapes) {
super(breakableNode);
this.breakLabel = new Label(breakableNode.getBreakLabel());
+ this.controlFlowEscapes = controlFlowEscapes;
}
/**
@@ -88,4 +93,23 @@
public List<Label> getLabels() {
return Collections.singletonList(breakLabel);
}
+
+ /**
+ * Does the control flow escape from this statement, i.e. through breaks or
+ * continues to outer loops?
+ * @return true if control flow escapes
+ */
+ public boolean controlFlowEscapes() {
+ return controlFlowEscapes;
+ }
+
+ /**
+ * Set the control flow escapes flag for this node.
+ * TODO - integrate this with Lowering in a better way
+ *
+ * @param lc lexical context
+ * @param controlFlowEscapes control flow escapes value
+ * @return new loop node if changed otherwise the same
+ */
+ public abstract BreakableStatement setControlFlowEscapes(final LexicalContext lc, final boolean controlFlowEscapes);
}
diff --git a/src/jdk/nashorn/internal/ir/LoopNode.java b/src/jdk/nashorn/internal/ir/LoopNode.java
--- a/src/jdk/nashorn/internal/ir/LoopNode.java
+++ b/src/jdk/nashorn/internal/ir/LoopNode.java
@@ -42,9 +42,6 @@
/** Loop body */
protected final Block body;
- /** Can control flow escape from loop, e.g. through breaks or continues to outer loops? */
- protected final boolean controlFlowEscapes;
-
/**
* Constructor
*
@@ -56,11 +53,10 @@
* @param controlFlowEscapes controlFlowEscapes
*/
protected LoopNode(final int lineNumber, final long token, final int finish, final Expression test, final Block body, final boolean controlFlowEscapes) {
- super(lineNumber, token, finish, new Label("while_break"));
+ super(lineNumber, token, finish, new Label("while_break"), controlFlowEscapes);
this.continueLabel = new Label("while_continue");
this.test = test;
this.body = body;
- this.controlFlowEscapes = controlFlowEscapes;
}
/**
@@ -72,26 +68,15 @@
* @param controlFlowEscapes controlFlowEscapes
*/
protected LoopNode(final LoopNode loopNode, final Expression test, final Block body, final boolean controlFlowEscapes) {
- super(loopNode);
+ super(loopNode, controlFlowEscapes);
this.continueLabel = new Label(loopNode.continueLabel);
this.test = test;
this.body = body;
- this.controlFlowEscapes = controlFlowEscapes;
}
@Override
public abstract Node ensureUniqueLabels(final LexicalContext lc);
- /**
- * Does the control flow escape from this loop, i.e. through breaks or
- * continues to outer loops?
- * @return true if control flow escapes
- */
- public boolean controlFlowEscapes() {
- return controlFlowEscapes;
- }
-
-
@Override
public boolean isTerminal() {
if (!mustEnter()) {
@@ -169,6 +154,7 @@
* @param controlFlowEscapes control flow escapes value
* @return new loop node if changed otherwise the same
*/
+ @Override
public abstract LoopNode setControlFlowEscapes(final LexicalContext lc, final boolean controlFlowEscapes);
}
diff --git a/src/jdk/nashorn/internal/ir/SwitchNode.java b/src/jdk/nashorn/internal/ir/SwitchNode.java
--- a/src/jdk/nashorn/internal/ir/SwitchNode.java
+++ b/src/jdk/nashorn/internal/ir/SwitchNode.java
@@ -28,6 +28,7 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
+
import jdk.nashorn.internal.codegen.Label;
import jdk.nashorn.internal.ir.annotations.Immutable;
import jdk.nashorn.internal.ir.visitor.NodeVisitor;
@@ -60,14 +61,14 @@
* @param defaultCase the default case node - null if none, otherwise has to be present in cases list
*/
public SwitchNode(final int lineNumber, final long token, final int finish, final Expression expression, final List<CaseNode> cases, final CaseNode defaultCase) {
- super(lineNumber, token, finish, new Label("switch_break"));
+ super(lineNumber, token, finish, new Label("switch_break"), false);
this.expression = expression;
this.cases = cases;
this.defaultCaseIndex = defaultCase == null ? -1 : cases.indexOf(defaultCase);
}
- private SwitchNode(final SwitchNode switchNode, final Expression expression, final List<CaseNode> cases, final int defaultCase) {
- super(switchNode);
+ private SwitchNode(final SwitchNode switchNode, final Expression expression, final List<CaseNode> cases, final int defaultCase, final boolean controlFlowEscapes) {
+ super(switchNode, controlFlowEscapes);
this.expression = expression;
this.cases = cases;
this.defaultCaseIndex = defaultCase;
@@ -80,11 +81,15 @@
for (final CaseNode caseNode : cases) {
newCases.add(new CaseNode(caseNode, caseNode.getTest(), caseNode.getBody()));
}
- return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, newCases, defaultCaseIndex));
+ return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, newCases, defaultCaseIndex, controlFlowEscapes));
}
@Override
public boolean isTerminal() {
+ //if control flow escapes - then not terminal
+ if (controlFlowEscapes) {
+ return false;
+ }
//there must be a default case, and that including all other cases must terminate
if (!cases.isEmpty() && defaultCaseIndex != -1) {
for (final CaseNode caseNode : cases) {
@@ -148,7 +153,7 @@
if (this.cases == cases) {
return this;
}
- return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, cases, defaultCaseIndex));
+ return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, cases, defaultCaseIndex, controlFlowEscapes));
}
/**
@@ -180,7 +185,7 @@
if (this.expression == expression) {
return this;
}
- return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, cases, defaultCaseIndex));
+ return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, cases, defaultCaseIndex, controlFlowEscapes));
}
/**
@@ -200,5 +205,13 @@
public void setTag(final Symbol tag) {
this.tag = tag;
}
+
+ @Override
+ public SwitchNode setControlFlowEscapes(final LexicalContext lc, final boolean controlFlowEscapes) {
+ if (this.controlFlowEscapes == controlFlowEscapes) {
+ return this;
+ }
+ return Node.replaceInLexicalContext(lc, this, new SwitchNode(this, expression, cases, defaultCaseIndex, controlFlowEscapes));
+ }
}
More information about the nashorn-dev
mailing list