Mercurial > people > rkennke > jdk9-shenandoah-final > nashorn
changeset 950:ba38d4cea99e
8051439: Wrong type calculated for ADD operator with undefined operand
Reviewed-by: jlaskey, sundar
| author | attila |
|---|---|
| date | Wed, 06 Aug 2014 11:02:14 +0200 |
| parents | fca4db1360f7 |
| children | ed60a4e9dd35 |
| files | src/jdk/nashorn/internal/codegen/CodeGenerator.java src/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java src/jdk/nashorn/internal/ir/BinaryNode.java test/script/basic/JDK-8051439.js test/script/basic/JDK-8051439.js.EXPECTED |
| diffstat | 5 files changed, 106 insertions(+), 6 deletions(-) [+] |
line wrap: on
line diff
--- a/src/jdk/nashorn/internal/codegen/CodeGenerator.java Wed Aug 06 10:42:46 2014 +0200 +++ b/src/jdk/nashorn/internal/codegen/CodeGenerator.java Wed Aug 06 11:02:14 2014 +0200 @@ -314,7 +314,7 @@ if (!symbol.isScope()) { final Type type = identNode.getType(); if(type == Type.UNDEFINED) { - return method.loadUndefined(Type.OBJECT); + return method.loadUndefined(resultBounds.widest); } assert symbol.hasSlot() || symbol.isParam();
--- a/src/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java Wed Aug 06 10:42:46 2014 +0200 +++ b/src/jdk/nashorn/internal/codegen/LocalVariableTypesCalculator.java Wed Aug 06 11:02:14 2014 +0200 @@ -1196,9 +1196,7 @@ } else if(binaryNode.isOptimisticUndecidedType()) { // At this point, we can assign a static type to the optimistic binary ADD operator as now we know // the types of its operands. - final Type type = Type.widest(binaryNode.lhs().getType(), binaryNode.rhs().getType()); - // Use Type.CHARSEQUENCE instead of Type.STRING to avoid conversion of ConsStrings to Strings. - return binaryNode.setType(type.equals(Type.STRING) ? Type.CHARSEQUENCE : type); + return binaryNode.decideType(); } return binaryNode; }
--- a/src/jdk/nashorn/internal/ir/BinaryNode.java Wed Aug 06 10:42:46 2014 +0200 +++ b/src/jdk/nashorn/internal/ir/BinaryNode.java Wed Aug 06 11:02:14 2014 +0200 @@ -183,17 +183,31 @@ switch (tokenType()) { case ADD: case ASSIGN_ADD: { + // Compare this logic to decideType(Type, Type); it's similar, but it handles the optimistic type + // calculation case while this handles the conservative case. final Type lhsType = lhs.getType(localVariableTypes); final Type rhsType = rhs.getType(localVariableTypes); if(lhsType == Type.BOOLEAN && rhsType == Type.BOOLEAN) { + // Will always fit in an int, as the value range is [0, 1, 2]. If we didn't treat them specially here, + // they'd end up being treated as generic INT operands and their sum would be conservatively considered + // to be a LONG in the generic case below; we can do better here. return Type.INT; + } else if(isString(lhsType) || isString(rhsType)) { + // We can statically figure out that this is a string if either operand is a string. In this case, use + // CHARSEQUENCE to prevent it from being proactively flattened. + return Type.CHARSEQUENCE; } - final Type widestOperandType = Type.widest(lhs.getType(localVariableTypes), rhs.getType(localVariableTypes)); + final Type widestOperandType = Type.widest(undefinedToNumber(booleanToInt(lhsType)), undefinedToNumber(booleanToInt(rhsType))); if(widestOperandType == Type.INT) { return Type.LONG; } else if (widestOperandType.isNumeric()) { return Type.NUMBER; } + // We pretty much can't know what it will be statically. Must presume OBJECT conservatively, as we can end + // up getting either a string or an object when adding something + object, e.g.: + // 1 + {} == "1[object Object]", but + // 1 + {valueOf: function() { return 2 }} == 3. Also: + // 1 + {valueOf: function() { return "2" }} == "12". return Type.OBJECT; } case SHR: @@ -256,10 +270,18 @@ } } + private static boolean isString(final Type type) { + return type == Type.STRING || type == Type.CHARSEQUENCE; + } + private static Type booleanToInt(final Type type) { return type == Type.BOOLEAN ? Type.INT : type; } + private static Type undefinedToNumber(final Type type) { + return type == Type.UNDEFINED ? Type.NUMBER : type; + } + /** * Check if this node is an assignment * @@ -527,7 +549,7 @@ private Type getTypeUncached(final Function<Symbol, Type> localVariableTypes) { if(type == OPTIMISTIC_UNDECIDED_TYPE) { - return Type.widest(lhs.getType(localVariableTypes), rhs.getType(localVariableTypes)); + return decideType(lhs.getType(localVariableTypes), rhs.getType(localVariableTypes)); } final Type widest = getWidestOperationType(localVariableTypes); if(type == null) { @@ -536,6 +558,32 @@ return Type.narrowest(widest, Type.widest(type, Type.widest(lhs.getType(localVariableTypes), rhs.getType(localVariableTypes)))); } + private static Type decideType(final Type lhsType, final Type rhsType) { + // Compare this to getWidestOperationType() for ADD and ASSIGN_ADD cases. There's some similar logic, but these + // are optimistic decisions, meaning that we don't have to treat boolean addition separately (as it'll become + // int addition in the general case anyway), and that we also don't conservatively widen sums of ints to + // longs, or sums of longs to doubles. + if(isString(lhsType) || isString(rhsType)) { + return Type.CHARSEQUENCE; + } + // NOTE: We don't have optimistic object-to-(int, long) conversions. Therefore, if any operand is an Object, we + // bail out of optimism here and presume a conservative Object return value, as the object's ToPrimitive() can + // end up returning either a number or a string, and their common supertype is Object, for better or worse. + final Type widest = Type.widest(undefinedToNumber(booleanToInt(lhsType)), undefinedToNumber(booleanToInt(rhsType))); + return widest.isObject() ? Type.OBJECT : widest; + } + + /** + * If the node is a node representing an add operation and has {@link #isOptimisticUndecidedType() optimistic + * undecided type}, decides its type. Should be invoked after its operands types have been finalized. + * @return returns a new node similar to this node, but with its type set to the type decided from the type of its + * operands. + */ + public BinaryNode decideType() { + assert type == OPTIMISTIC_UNDECIDED_TYPE; + return setType(decideType(lhs.getType(), rhs.getType())); + } + @Override public BinaryNode setType(final Type type) { if (this.type == type) {
--- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/test/script/basic/JDK-8051439.js Wed Aug 06 11:02:14 2014 +0200 @@ -0,0 +1,52 @@ +/* + * 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-8051439: Wrong type calculated for ADD operator with undefined operand + * + * @test + * @run + */ + +// Test + operator +function f1() { + var x; + for (var i = 0;i < 3; i++) { + x = x + i; + } + x = x + "test"; + return x; +} + +// Test += operator +function f2() { + var x; + for (var i = 0;i < 3; i++) { + x += i; + } + x += "test"; + return x; +} + +print(f1()); +print(f2());