8058610: must not let long operations overflow

Tue, 21 Oct 2014 14:27:49 +0200

author
attila
date
Tue, 21 Oct 2014 14:27:49 +0200
changeset 1065
3219e9e47daf
parent 1064
03c06c337d9d
child 1066
7fad0ce81344

8058610: must not let long operations overflow
Reviewed-by: hannesw, jlaskey, lagergren

src/jdk/nashorn/internal/codegen/CodeGenerator.java file | annotate | diff | comparison | revisions
test/script/basic/JDK-8058610.js file | annotate | diff | comparison | revisions
test/script/basic/JDK-8058610.js.EXPECTED file | annotate | diff | comparison | revisions
     1.1 --- a/src/jdk/nashorn/internal/codegen/CodeGenerator.java	Mon Oct 20 12:06:36 2014 +0200
     1.2 +++ b/src/jdk/nashorn/internal/codegen/CodeGenerator.java	Tue Oct 21 14:27:49 2014 +0200
     1.3 @@ -552,10 +552,10 @@
     1.4      }
     1.5  
     1.6      MethodEmitter loadBinaryOperands(final BinaryNode binaryNode) {
     1.7 -        return loadBinaryOperands(binaryNode.lhs(), binaryNode.rhs(), TypeBounds.UNBOUNDED.notWiderThan(binaryNode.getWidestOperandType()), false);
     1.8 +        return loadBinaryOperands(binaryNode.lhs(), binaryNode.rhs(), TypeBounds.UNBOUNDED.notWiderThan(binaryNode.getWidestOperandType()), false, false);
     1.9      }
    1.10  
    1.11 -    private MethodEmitter loadBinaryOperands(final Expression lhs, final Expression rhs, final TypeBounds explicitOperandBounds, final boolean baseAlreadyOnStack) {
    1.12 +    private MethodEmitter loadBinaryOperands(final Expression lhs, final Expression rhs, final TypeBounds explicitOperandBounds, final boolean baseAlreadyOnStack, final boolean forceConversionSeparation) {
    1.13          // ECMAScript 5.1 specification (sections 11.5-11.11 and 11.13) prescribes that when evaluating a binary
    1.14          // expression "LEFT op RIGHT", the order of operations must be: LOAD LEFT, LOAD RIGHT, CONVERT LEFT, CONVERT
    1.15          // RIGHT, EXECUTE OP. Unfortunately, doing it in this order defeats potential optimizations that arise when we
    1.16 @@ -572,15 +572,34 @@
    1.17          final Type narrowestOperandType = Type.narrowest(Type.widest(lhs.getType(), rhs.getType()), explicitOperandBounds.widest);
    1.18          final TypeBounds operandBounds = explicitOperandBounds.notNarrowerThan(narrowestOperandType);
    1.19          if (noToPrimitiveConversion(lhs.getType(), explicitOperandBounds.widest) || rhs.isLocal()) {
    1.20 -            // Can reorder. Combine load and convert into single operations.
    1.21 -            loadExpression(lhs, operandBounds, baseAlreadyOnStack);
    1.22 -            loadExpression(rhs, operandBounds, false);
    1.23 +            // Can reorder. We might still need to separate conversion, but at least we can do it with reordering
    1.24 +            if (forceConversionSeparation) {
    1.25 +                // Can reorder, but can't move conversion into the operand as the operation depends on operands
    1.26 +                // exact types for its overflow guarantees. E.g. with {L}{%I}expr1 {L}* {L}{%I}expr2 we are not allowed
    1.27 +                // to merge {L}{%I} into {%L}, as that can cause subsequent overflows; test for JDK-8058610 contains
    1.28 +                // concrete cases where this could happen.
    1.29 +                final TypeBounds safeConvertBounds = TypeBounds.UNBOUNDED.notNarrowerThan(narrowestOperandType);
    1.30 +                loadExpression(lhs, safeConvertBounds, baseAlreadyOnStack);
    1.31 +                method.convert(operandBounds.within(method.peekType()));
    1.32 +                loadExpression(rhs, safeConvertBounds, false);
    1.33 +                method.convert(operandBounds.within(method.peekType()));
    1.34 +            } else {
    1.35 +                // Can reorder and move conversion into the operand. Combine load and convert into single operations.
    1.36 +                loadExpression(lhs, operandBounds, baseAlreadyOnStack);
    1.37 +                loadExpression(rhs, operandBounds, false);
    1.38 +            }
    1.39          } else {
    1.40              // Can't reorder. Load and convert separately.
    1.41              final TypeBounds safeConvertBounds = TypeBounds.UNBOUNDED.notNarrowerThan(narrowestOperandType);
    1.42              loadExpression(lhs, safeConvertBounds, baseAlreadyOnStack);
    1.43 +            final Type lhsType = method.peekType();
    1.44              loadExpression(rhs, safeConvertBounds, false);
    1.45 -            method.swap().convert(operandBounds.within(method.peekType())).swap().convert(operandBounds.within(method.peekType()));
    1.46 +            final Type convertedLhsType = operandBounds.within(method.peekType());
    1.47 +            if (convertedLhsType != lhsType) {
    1.48 +                // Do it conditionally, so that if conversion is a no-op we don't introduce a SWAP, SWAP.
    1.49 +                method.swap().convert(convertedLhsType).swap();
    1.50 +            }
    1.51 +            method.convert(operandBounds.within(method.peekType()));
    1.52          }
    1.53          assert Type.generic(method.peekType()) == operandBounds.narrowest;
    1.54          assert Type.generic(method.peekType(1)) == operandBounds.narrowest;
    1.55 @@ -631,19 +650,11 @@
    1.56          }
    1.57  
    1.58          TypeBounds booleanToInt() {
    1.59 -            return maybeNew(booleanToInt(narrowest), booleanToInt(widest));
    1.60 +            return maybeNew(CodeGenerator.booleanToInt(narrowest), CodeGenerator.booleanToInt(widest));
    1.61          }
    1.62  
    1.63          TypeBounds objectToNumber() {
    1.64 -            return maybeNew(objectToNumber(narrowest), objectToNumber(widest));
    1.65 -        }
    1.66 -
    1.67 -        private static Type booleanToInt(final Type t) {
    1.68 -            return t == Type.BOOLEAN ? Type.INT : t;
    1.69 -        }
    1.70 -
    1.71 -        private static Type objectToNumber(final Type t) {
    1.72 -            return t.isObject() ? Type.NUMBER : t;
    1.73 +            return maybeNew(CodeGenerator.objectToNumber(narrowest), CodeGenerator.objectToNumber(widest));
    1.74          }
    1.75  
    1.76          Type within(final Type type) {
    1.77 @@ -662,6 +673,14 @@
    1.78          }
    1.79      }
    1.80  
    1.81 +    private static Type booleanToInt(final Type t) {
    1.82 +        return t == Type.BOOLEAN ? Type.INT : t;
    1.83 +    }
    1.84 +
    1.85 +    private static Type objectToNumber(final Type t) {
    1.86 +        return t.isObject() ? Type.NUMBER : t;
    1.87 +    }
    1.88 +
    1.89      MethodEmitter loadExpressionAsType(final Expression expr, final Type type) {
    1.90          if(type == Type.BOOLEAN) {
    1.91              return loadExpressionAsBoolean(expr);
    1.92 @@ -3543,13 +3562,15 @@
    1.93              void loadStack() {
    1.94                  final TypeBounds operandBounds;
    1.95                  final boolean isOptimistic = isValid(getProgramPoint());
    1.96 +                boolean forceConversionSeparation = false;
    1.97                  if(isOptimistic) {
    1.98                      operandBounds = new TypeBounds(binaryNode.getType(), Type.OBJECT);
    1.99                  } else {
   1.100                      // Non-optimistic, non-FP +. Allow it to overflow.
   1.101                      operandBounds = new TypeBounds(binaryNode.getWidestOperandType(), Type.OBJECT);
   1.102 +                    forceConversionSeparation = binaryNode.getWidestOperationType().narrowerThan(resultBounds.widest);
   1.103                  }
   1.104 -                loadBinaryOperands(binaryNode.lhs(), binaryNode.rhs(), operandBounds, false);
   1.105 +                loadBinaryOperands(binaryNode.lhs(), binaryNode.rhs(), operandBounds, false, forceConversionSeparation);
   1.106              }
   1.107  
   1.108              @Override
   1.109 @@ -3660,12 +3681,21 @@
   1.110          @Override
   1.111          protected void evaluate() {
   1.112              final Expression lhs = assignNode.lhs();
   1.113 -            final Type widest = assignNode.isTokenType(TokenType.ASSIGN_ADD) ? Type.OBJECT : assignNode.getWidestOperationType();
   1.114 +            final Expression rhs = assignNode.rhs();
   1.115 +            final Type widestOperationType = assignNode.getWidestOperationType();
   1.116 +            final Type widest = assignNode.isTokenType(TokenType.ASSIGN_ADD) ? Type.OBJECT : widestOperationType;
   1.117              final TypeBounds bounds = new TypeBounds(assignNode.getType(), widest);
   1.118              new OptimisticOperation(assignNode, bounds) {
   1.119                  @Override
   1.120                  void loadStack() {
   1.121 -                    loadBinaryOperands(lhs, assignNode.rhs(), bounds, true);
   1.122 +                    final boolean forceConversionSeparation;
   1.123 +                    if (isValid(getProgramPoint()) || widestOperationType == Type.NUMBER) {
   1.124 +                        forceConversionSeparation = false;
   1.125 +                    } else {
   1.126 +                        final Type operandType = Type.widest(booleanToInt(objectToNumber(lhs.getType())), booleanToInt(objectToNumber(rhs.getType())));
   1.127 +                        forceConversionSeparation = operandType.narrowerThan(widestOperationType);
   1.128 +                    }
   1.129 +                    loadBinaryOperands(lhs, rhs, bounds, true, forceConversionSeparation);
   1.130                  }
   1.131                  @Override
   1.132                  void consumeStack() {
   1.133 @@ -3688,7 +3718,7 @@
   1.134  
   1.135          @Override
   1.136          protected void evaluate() {
   1.137 -            loadBinaryOperands(assignNode.lhs(), assignNode.rhs(), TypeBounds.UNBOUNDED.notWiderThan(assignNode.getWidestOperandType()), true);
   1.138 +            loadBinaryOperands(assignNode.lhs(), assignNode.rhs(), TypeBounds.UNBOUNDED.notWiderThan(assignNode.getWidestOperandType()), true, false);
   1.139              op();
   1.140          }
   1.141      }
   1.142 @@ -3811,6 +3841,7 @@
   1.143                  @Override
   1.144                  void loadStack() {
   1.145                      final TypeBounds operandBounds;
   1.146 +                    boolean forceConversionSeparation = false;
   1.147                      if(numericBounds.narrowest == Type.NUMBER) {
   1.148                          // Result should be double always. Propagate it into the operands so we don't have lots of I2D
   1.149                          // and L2D after operand evaluation.
   1.150 @@ -3828,9 +3859,10 @@
   1.151                              // Non-optimistic, non-FP subtraction or multiplication. Allow them to overflow.
   1.152                              operandBounds = new TypeBounds(Type.narrowest(node.getWidestOperandType(),
   1.153                                      numericBounds.widest), Type.NUMBER);
   1.154 +                            forceConversionSeparation = node.getWidestOperationType().narrowerThan(numericBounds.widest);
   1.155                          }
   1.156                      }
   1.157 -                    loadBinaryOperands(node.lhs(), node.rhs(), operandBounds, false);
   1.158 +                    loadBinaryOperands(node.lhs(), node.rhs(), operandBounds, false, forceConversionSeparation);
   1.159                  }
   1.160  
   1.161                  @Override
     2.1 --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
     2.2 +++ b/test/script/basic/JDK-8058610.js	Tue Oct 21 14:27:49 2014 +0200
     2.3 @@ -0,0 +1,77 @@
     2.4 +/*
     2.5 + * Copyright (c) 2014 Oracle and/or its affiliates. All rights reserved.
     2.6 + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
     2.7 + * 
     2.8 + * This code is free software; you can redistribute it and/or modify it
     2.9 + * under the terms of the GNU General Public License version 2 only, as
    2.10 + * published by the Free Software Foundation.
    2.11 + * 
    2.12 + * This code is distributed in the hope that it will be useful, but WITHOUT
    2.13 + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
    2.14 + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
    2.15 + * version 2 for more details (a copy is included in the LICENSE file that
    2.16 + * accompanied this code).
    2.17 + * 
    2.18 + * You should have received a copy of the GNU General Public License version
    2.19 + * 2 along with this work; if not, write to the Free Software Foundation,
    2.20 + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
    2.21 + * 
    2.22 + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
    2.23 + * or visit www.oracle.com if you need additional information or have any
    2.24 + * questions.
    2.25 + */
    2.26 +
    2.27 +/**
    2.28 + * JDK-8058610: must not let long operations overflow
    2.29 + *
    2.30 + * @test
    2.31 + * @run
    2.32 + */
    2.33 +
    2.34 +function mul(x) { 
    2.35 +    return x.foo * x.bar; 
    2.36 +} 
    2.37 +print("=== mul ===")
    2.38 +print(mul({foo: 2147483647,  bar: 2147483647})); // 2^31
    2.39 +print(mul({foo: 17179869184, bar: 2147483647})); // 2^34
    2.40 +
    2.41 +function self_mul(x) {
    2.42 +    return x.foo *= x.bar; 
    2.43 +}
    2.44 +print("=== self_mul ===")
    2.45 +print(self_mul({foo: 2147483647,  bar: 2147483647})); // 2^31
    2.46 +print(self_mul({foo: 17179869184, bar: 2147483647})); // 2^34
    2.47 +
    2.48 +// We'll need to use this function to obtain long values larger in 
    2.49 +// magnitude than those precisely representable in a double (2^53), 
    2.50 +// as Nashorn's parser will reify such literals as a double. For 
    2.51 +// overflow on add and sub we need (2^63)-1.
    2.52 +var parseLong = Java.type("java.lang.Long").parseLong;
    2.53 +
    2.54 +function sub(x) {
    2.55 +    return x.foo - x.bar;
    2.56 +}
    2.57 +print("=== sub ===")
    2.58 +print(sub({foo: 2147483647,  bar: -2147483647})); // 2^31
    2.59 +print(sub({foo: parseLong("9223372036854775807"), bar: parseLong("-9223372036854775807")})); // 2^63-1
    2.60 +
    2.61 +function self_sub(x) {
    2.62 +    return x.foo -= x.bar;
    2.63 +}
    2.64 +print("=== self_sub ===")
    2.65 +print(self_sub({foo: 2147483647,  bar: -2147483647})); // 2^31
    2.66 +print(self_sub({foo: parseLong("9223372036854775807"), bar: parseLong("-9223372036854775807")})); // 2^63-1
    2.67 +
    2.68 +function add(x) {
    2.69 +    return x.foo + x.bar;
    2.70 +}
    2.71 +print("=== add ===")
    2.72 +print(add({foo: 2147483647,  bar: 2147483647})); // 2^31
    2.73 +print(add({foo: parseLong("9223372036854775807"), bar: parseLong("9223372036854775807")})); // 2^63-1
    2.74 +
    2.75 +function self_add(x) {
    2.76 +    return x.foo += x.bar;
    2.77 +}
    2.78 +print("=== self_add ===")
    2.79 +print(self_add({foo: 2147483647,  bar: 2147483647})); // 2^31
    2.80 +print(self_add({foo: parseLong("9223372036854775807"), bar: parseLong("9223372036854775807")})); // 2^63-1
     3.1 --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
     3.2 +++ b/test/script/basic/JDK-8058610.js.EXPECTED	Tue Oct 21 14:27:49 2014 +0200
     3.3 @@ -0,0 +1,18 @@
     3.4 +=== mul ===
     3.5 +4611686014132420600
     3.6 +36893488130239234000
     3.7 +=== self_mul ===
     3.8 +4611686014132420600
     3.9 +36893488130239234000
    3.10 +=== sub ===
    3.11 +4294967294
    3.12 +18446744073709552000
    3.13 +=== self_sub ===
    3.14 +4294967294
    3.15 +18446744073709552000
    3.16 +=== add ===
    3.17 +4294967294
    3.18 +18446744073709552000
    3.19 +=== self_add ===
    3.20 +4294967294
    3.21 +18446744073709552000

mercurial