Bug 940635 - Don't allow Int32 conversions of both LHS and RHS in comparisons when their types are not actually known, r=shu.
authorBrian Hackett <bhackett1024@gmail.com>
Sat, 07 Dec 2013 18:18:16 -0800
changeset 175074 70ec3658b113ffa3ee5d6a78aaa0a112cc1e378e
parent 175073 3c63decb4e7ebe42df60c98434d70731a8463097
child 175075 2bb7e7192ced466e7dc05c2853e024cdaf2c7c5c
push id445
push userffxbld
push dateMon, 10 Mar 2014 22:05:19 +0000
treeherdermozilla-release@dc38b741b04e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersshu
bugs940635
milestone28.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 940635 - Don't allow Int32 conversions of both LHS and RHS in comparisons when their types are not actually known, r=shu.
js/src/jit-test/tests/ion/bug940635.js
js/src/jit/BaselineInspector.cpp
js/src/jit/CodeGenerator.cpp
js/src/jit/IonAnalysis.cpp
js/src/jit/IonMacroAssembler.cpp
js/src/jit/IonMacroAssembler.h
js/src/jit/Lowering.cpp
js/src/jit/MIR.cpp
js/src/jit/MIR.h
js/src/jit/TypePolicy.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/ion/bug940635.js
@@ -0,0 +1,7 @@
+function f(y) {
+    return (y > 0) == y;
+}
+assertEq(f(0), true);
+assertEq(f(0), true);
+assertEq(f(null), false);
+assertEq(f(null), false);
--- a/js/src/jit/BaselineInspector.cpp
+++ b/js/src/jit/BaselineInspector.cpp
@@ -221,18 +221,30 @@ CanUseInt32Compare(ICStub::Kind kind)
 
 MCompare::CompareType
 BaselineInspector::expectedCompareType(jsbytecode *pc)
 {
     ICStub *first = monomorphicStub(pc), *second = nullptr;
     if (!first && !dimorphicStub(pc, &first, &second))
         return MCompare::Compare_Unknown;
 
-    if (CanUseInt32Compare(first->kind()) && (!second || CanUseInt32Compare(second->kind())))
+    if (CanUseInt32Compare(first->kind()) && (!second || CanUseInt32Compare(second->kind()))) {
+        ICCompare_Int32WithBoolean *coerce =
+            first->isCompare_Int32WithBoolean()
+            ? first->toCompare_Int32WithBoolean()
+            : ((second && second->isCompare_Int32WithBoolean())
+               ? second->toCompare_Int32WithBoolean()
+               : nullptr);
+        if (coerce) {
+            return coerce->lhsIsInt32()
+                   ? MCompare::Compare_Int32MaybeCoerceRHS
+                   : MCompare::Compare_Int32MaybeCoerceLHS;
+        }
         return MCompare::Compare_Int32;
+    }
 
     if (CanUseDoubleCompare(first->kind()) && (!second || CanUseDoubleCompare(second->kind()))) {
         ICCompare_NumberWithUndefined *coerce =
             first->isCompare_NumberWithUndefined()
             ? first->toCompare_NumberWithUndefined()
             : (second && second->isCompare_NumberWithUndefined())
               ? second->toCompare_NumberWithUndefined()
               : nullptr;
--- a/js/src/jit/CodeGenerator.cpp
+++ b/js/src/jit/CodeGenerator.cpp
@@ -196,17 +196,18 @@ CodeGenerator::visitValueToInt32(LValueT
             stringRejoin = nullptr;
         }
 
         masm.truncateValueToInt32(operand, input, stringEntry, stringRejoin, oolDouble->entry(),
                                   stringReg, temp, output, &fails);
         masm.bind(oolDouble->rejoin());
     } else {
         masm.convertValueToInt32(operand, input, temp, output, &fails,
-                                 lir->mirNormal()->canBeNegativeZero());
+                                 lir->mirNormal()->canBeNegativeZero(),
+                                 lir->mirNormal()->conversion());
     }
 
     return bailoutFrom(&fails, lir->snapshot());
 }
 
 bool
 CodeGenerator::visitValueToDouble(LValueToDouble *lir)
 {
--- a/js/src/jit/IonAnalysis.cpp
+++ b/js/src/jit/IonAnalysis.cpp
@@ -1444,17 +1444,17 @@ jit::ExtractLinearInequality(MTest *test
         return false;
 
     MCompare *compare = test->getOperand(0)->toCompare();
 
     MDefinition *lhs = compare->getOperand(0);
     MDefinition *rhs = compare->getOperand(1);
 
     // TODO: optimize Compare_UInt32
-    if (compare->compareType() != MCompare::Compare_Int32)
+    if (!compare->isInt32Comparison())
         return false;
 
     JS_ASSERT(lhs->type() == MIRType_Int32);
     JS_ASSERT(rhs->type() == MIRType_Int32);
 
     JSOp jsop = compare->jsop();
     if (direction == FALSE_BRANCH)
         jsop = analyze::NegateCompareOp(jsop);
--- a/js/src/jit/IonMacroAssembler.cpp
+++ b/js/src/jit/IonMacroAssembler.cpp
@@ -1558,48 +1558,56 @@ MacroAssembler::convertDoubleToInt(Float
     }
 }
 
 void
 MacroAssembler::convertValueToInt(ValueOperand value, MDefinition *maybeInput,
                                   Label *handleStringEntry, Label *handleStringRejoin,
                                   Label *truncateDoubleSlow,
                                   Register stringReg, FloatRegister temp, Register output,
-                                  Label *fail, IntConversionBehavior behavior)
+                                  Label *fail, IntConversionBehavior behavior,
+                                  IntConversionInputKind conversion)
 {
     Register tag = splitTagForTest(value);
     bool handleStrings = (behavior == IntConversion_Truncate ||
                           behavior == IntConversion_ClampToUint8) &&
                          handleStringEntry &&
                          handleStringRejoin;
     bool zeroObjects = behavior == IntConversion_ClampToUint8;
 
+    JS_ASSERT_IF(handleStrings || zeroObjects, conversion == IntConversion_Any);
+
     Label done, isInt32, isBool, isDouble, isNull, isString;
 
     branchEqualTypeIfNeeded(MIRType_Int32, maybeInput, tag, &isInt32);
-    branchEqualTypeIfNeeded(MIRType_Boolean, maybeInput, tag, &isBool);
+    if (conversion == IntConversion_Any)
+        branchEqualTypeIfNeeded(MIRType_Boolean, maybeInput, tag, &isBool);
     branchEqualTypeIfNeeded(MIRType_Double, maybeInput, tag, &isDouble);
 
-    // If we are not truncating, we fail for anything that's not
-    // null. Otherwise we might be able to handle strings and objects.
-    switch (behavior) {
-      case IntConversion_Normal:
-      case IntConversion_NegativeZeroCheck:
-        branchTestNull(Assembler::NotEqual, tag, fail);
-        break;
+    if (conversion == IntConversion_Any) {
+        // If we are not truncating, we fail for anything that's not
+        // null. Otherwise we might be able to handle strings and objects.
+        switch (behavior) {
+          case IntConversion_Normal:
+          case IntConversion_NegativeZeroCheck:
+            branchTestNull(Assembler::NotEqual, tag, fail);
+            break;
 
-      case IntConversion_Truncate:
-      case IntConversion_ClampToUint8:
-        branchEqualTypeIfNeeded(MIRType_Null, maybeInput, tag, &isNull);
-        if (handleStrings)
-            branchEqualTypeIfNeeded(MIRType_String, maybeInput, tag, &isString);
-        if (zeroObjects)
-            branchEqualTypeIfNeeded(MIRType_Object, maybeInput, tag, &isNull);
-        branchTestUndefined(Assembler::NotEqual, tag, fail);
-        break;
+          case IntConversion_Truncate:
+          case IntConversion_ClampToUint8:
+            branchEqualTypeIfNeeded(MIRType_Null, maybeInput, tag, &isNull);
+            if (handleStrings)
+                branchEqualTypeIfNeeded(MIRType_String, maybeInput, tag, &isString);
+            if (zeroObjects)
+                branchEqualTypeIfNeeded(MIRType_Object, maybeInput, tag, &isNull);
+            branchTestUndefined(Assembler::NotEqual, tag, fail);
+            break;
+        }
+    } else {
+        jump(fail);
     }
 
     // The value is null or undefined in truncation contexts - just emit 0.
     if (isNull.used())
         bind(&isNull);
     mov(ImmWord(0), output);
     jump(&done);
 
--- a/js/src/jit/IonMacroAssembler.h
+++ b/js/src/jit/IonMacroAssembler.h
@@ -1169,31 +1169,37 @@ class MacroAssembler : public MacroAssem
 
     enum IntConversionBehavior {
         IntConversion_Normal,
         IntConversion_NegativeZeroCheck,
         IntConversion_Truncate,
         IntConversion_ClampToUint8,
     };
 
+    enum IntConversionInputKind {
+        IntConversion_NumbersOnly,
+        IntConversion_Any
+    };
+
     //
     // Functions for converting values to int.
     //
     void convertDoubleToInt(FloatRegister src, Register output, FloatRegister temp,
                             Label *truncateFail, Label *fail, IntConversionBehavior behavior);
 
     // Strings may be handled by providing labels to jump to when the behavior
     // is truncation or clamping. The subroutine, usually an OOL call, is
     // passed the unboxed string in |stringReg| and should convert it to a
     // double store into |temp|.
     void convertValueToInt(ValueOperand value, MDefinition *input,
                            Label *handleStringEntry, Label *handleStringRejoin,
                            Label *truncateDoubleSlow,
                            Register stringReg, FloatRegister temp, Register output,
-                           Label *fail, IntConversionBehavior behavior);
+                           Label *fail, IntConversionBehavior behavior,
+                           IntConversionInputKind conversion = IntConversion_Any);
     void convertValueToInt(ValueOperand value, FloatRegister temp, Register output, Label *fail,
                            IntConversionBehavior behavior)
     {
         convertValueToInt(value, nullptr, nullptr, nullptr, nullptr, InvalidReg, temp, output,
                           fail, behavior);
     }
     bool convertValueToInt(JSContext *cx, const Value &v, Register output, Label *fail,
                            IntConversionBehavior behavior);
@@ -1209,22 +1215,23 @@ class MacroAssembler : public MacroAssem
                              bool negativeZeroCheck)
     {
         convertValueToInt(value, temp, output, fail, negativeZeroCheck
                           ? IntConversion_NegativeZeroCheck
                           : IntConversion_Normal);
     }
     void convertValueToInt32(ValueOperand value, MDefinition *input,
                              FloatRegister temp, Register output, Label *fail,
-                             bool negativeZeroCheck)
+                             bool negativeZeroCheck, IntConversionInputKind conversion = IntConversion_Any)
     {
         convertValueToInt(value, input, nullptr, nullptr, nullptr, InvalidReg, temp, output, fail,
                           negativeZeroCheck
                           ? IntConversion_NegativeZeroCheck
-                          : IntConversion_Normal);
+                          : IntConversion_Normal,
+                          conversion);
     }
     bool convertValueToInt32(JSContext *cx, const Value &v, Register output, Label *fail,
                              bool negativeZeroCheck)
     {
         return convertValueToInt(cx, v, output, fail, negativeZeroCheck
                                  ? IntConversion_NegativeZeroCheck
                                  : IntConversion_Normal);
     }
--- a/js/src/jit/Lowering.cpp
+++ b/js/src/jit/Lowering.cpp
@@ -764,30 +764,27 @@ LIRGenerator::visitTest(MTest *test)
             LAllocation rhs = useRegisterOrConstant(right);
             LCompareBAndBranch *lir = new(alloc()) LCompareBAndBranch(rhs, ifTrue, ifFalse);
             if (!useBox(lir, LCompareBAndBranch::Lhs, left))
                 return false;
             return add(lir, comp);
         }
 
         // Compare and branch Int32 or Object pointers.
-        if (comp->compareType() == MCompare::Compare_Int32 ||
+        if (comp->isInt32Comparison() ||
             comp->compareType() == MCompare::Compare_UInt32 ||
             comp->compareType() == MCompare::Compare_Object)
         {
             JSOp op = ReorderComparison(comp->jsop(), &left, &right);
             LAllocation lhs = useRegister(left);
             LAllocation rhs;
-            if (comp->compareType() == MCompare::Compare_Int32 ||
-                comp->compareType() == MCompare::Compare_UInt32)
-            {
+            if (comp->isInt32Comparison() || comp->compareType() == MCompare::Compare_UInt32)
                 rhs = useAnyOrConstant(right);
-            } else {
+            else
                 rhs = useRegister(right);
-            }
             LCompareAndBranch *lir = new(alloc()) LCompareAndBranch(op, lhs, rhs, ifTrue, ifFalse);
             return add(lir, comp);
         }
 
         // Compare and branch doubles.
         if (comp->isDoubleComparison()) {
             LAllocation lhs = useRegister(left);
             LAllocation rhs = useRegister(right);
@@ -953,24 +950,24 @@ LIRGenerator::visitCompare(MCompare *com
 
         LCompareB *lir = new(alloc()) LCompareB(useRegisterOrConstant(right));
         if (!useBox(lir, LCompareB::Lhs, left))
             return false;
         return define(lir, comp);
     }
 
     // Compare Int32 or Object pointers.
-    if (comp->compareType() == MCompare::Compare_Int32 ||
+    if (comp->isInt32Comparison() ||
         comp->compareType() == MCompare::Compare_UInt32 ||
         comp->compareType() == MCompare::Compare_Object)
     {
         JSOp op = ReorderComparison(comp->jsop(), &left, &right);
         LAllocation lhs = useRegister(left);
         LAllocation rhs;
-        if (comp->compareType() == MCompare::Compare_Int32 ||
+        if (comp->isInt32Comparison() ||
             comp->compareType() == MCompare::Compare_UInt32)
         {
             rhs = useAnyOrConstant(right);
         } else {
             rhs = useRegister(right);
         }
         return define(new(alloc()) LCompare(op, lhs, rhs), comp);
     }
--- a/js/src/jit/MIR.cpp
+++ b/js/src/jit/MIR.cpp
@@ -1718,16 +1718,19 @@ MCompare::inputType()
       case Compare_Undefined:
         return MIRType_Undefined;
       case Compare_Null:
         return MIRType_Null;
       case Compare_Boolean:
         return MIRType_Boolean;
       case Compare_UInt32:
       case Compare_Int32:
+      case Compare_Int32MaybeCoerceBoth:
+      case Compare_Int32MaybeCoerceLHS:
+      case Compare_Int32MaybeCoerceRHS:
         return MIRType_Int32;
       case Compare_Double:
       case Compare_DoubleMaybeCoerceLHS:
       case Compare_DoubleMaybeCoerceRHS:
         return MIRType_Double;
       case Compare_Float32:
         return MIRType_Float32;
       case Compare_String:
@@ -1804,26 +1807,26 @@ MCompare::infer(BaselineInspector *inspe
         compareType_ = Compare_UInt32;
         return;
     }
 
     // Integer to integer or boolean to boolean comparisons may be treated as Int32.
     if ((lhs == MIRType_Int32 && rhs == MIRType_Int32) ||
         (lhs == MIRType_Boolean && rhs == MIRType_Boolean))
     {
-        compareType_ = Compare_Int32;
+        compareType_ = Compare_Int32MaybeCoerceBoth;
         return;
     }
 
     // Loose/relational cross-integer/boolean comparisons may be treated as Int32.
     if (!strictEq &&
         (lhs == MIRType_Int32 || lhs == MIRType_Boolean) &&
         (rhs == MIRType_Int32 || rhs == MIRType_Boolean))
     {
-        compareType_ = Compare_Int32;
+        compareType_ = Compare_Int32MaybeCoerceBoth;
         return;
     }
 
     // Numeric comparisons against a double coerce to double.
     if (IsNumberType(lhs) && IsNumberType(rhs)) {
         compareType_ = Compare_Double;
         return;
     }
--- a/js/src/jit/MIR.h
+++ b/js/src/jit/MIR.h
@@ -2187,16 +2187,19 @@ class MCompare
         // String    compared to Boolean
         // Object    compared to Boolean
         // Value     compared to Boolean
         Compare_Boolean,
 
         // Int32   compared to Int32
         // Boolean compared to Boolean
         Compare_Int32,
+        Compare_Int32MaybeCoerceBoth,
+        Compare_Int32MaybeCoerceLHS,
+        Compare_Int32MaybeCoerceRHS,
 
         // Int32 compared as unsigneds
         Compare_UInt32,
 
         // Double compared to Double
         Compare_Double,
 
         Compare_DoubleMaybeCoerceLHS,
@@ -2253,16 +2256,22 @@ class MCompare
     bool tryFold(bool *result);
     bool evaluateConstantOperands(bool *result);
     MDefinition *foldsTo(TempAllocator &alloc, bool useValueNumbers);
 
     void infer(BaselineInspector *inspector, jsbytecode *pc);
     CompareType compareType() const {
         return compareType_;
     }
+    bool isInt32Comparison() const {
+        return compareType() == Compare_Int32 ||
+               compareType() == Compare_Int32MaybeCoerceBoth ||
+               compareType() == Compare_Int32MaybeCoerceLHS ||
+               compareType() == Compare_Int32MaybeCoerceRHS;
+    }
     bool isDoubleComparison() const {
         return compareType() == Compare_Double ||
                compareType() == Compare_DoubleMaybeCoerceLHS ||
                compareType() == Compare_DoubleMaybeCoerceRHS;
     }
     bool isFloat32Comparison() const {
         return compareType() == Compare_Float32;
     }
@@ -3036,30 +3045,34 @@ class MAsmJSUnsignedToFloat32
 // Converts a primitive (either typed or untyped) to an int32. If the input is
 // not primitive at runtime, a bailout occurs. If the input cannot be converted
 // to an int32 without loss (i.e. "5.5" or undefined) then a bailout occurs.
 class MToInt32
   : public MUnaryInstruction,
     public ToInt32Policy
 {
     bool canBeNegativeZero_;
-
-    MToInt32(MDefinition *def)
+    MacroAssembler::IntConversionInputKind conversion_;
+
+    MToInt32(MDefinition *def, MacroAssembler::IntConversionInputKind conversion)
       : MUnaryInstruction(def),
-        canBeNegativeZero_(true)
+        canBeNegativeZero_(true),
+        conversion_(conversion)
     {
         setResultType(MIRType_Int32);
         setMovable();
     }
 
   public:
     INSTRUCTION_HEADER(ToInt32)
-    static MToInt32 *New(TempAllocator &alloc, MDefinition *def)
-    {
-        return new(alloc) MToInt32(def);
+    static MToInt32 *New(TempAllocator &alloc, MDefinition *def,
+                         MacroAssembler::IntConversionInputKind conversion =
+                             MacroAssembler::IntConversion_Any)
+    {
+        return new(alloc) MToInt32(def, conversion);
     }
 
     MDefinition *foldsTo(TempAllocator &alloc, bool useValueNumbers);
 
     // this only has backwards information flow.
     void analyzeEdgeCasesBackward();
 
     bool canBeNegativeZero() const {
@@ -3068,16 +3081,20 @@ class MToInt32
     void setCanBeNegativeZero(bool negativeZero) {
         canBeNegativeZero_ = negativeZero;
     }
 
     TypePolicy *typePolicy() {
         return this;
     }
 
+    MacroAssembler::IntConversionInputKind conversion() const {
+        return conversion_;
+    }
+
     bool congruentTo(MDefinition *ins) const {
         return congruentIfOperandsEqual(ins);
     }
 
     AliasSet getAliasSet() const {
         return AliasSet::None();
     }
     void computeRange(TempAllocator &alloc);
--- a/js/src/jit/TypePolicy.cpp
+++ b/js/src/jit/TypePolicy.cpp
@@ -136,17 +136,17 @@ ComparePolicy::adjustInputs(TempAllocato
 
     // Compare_Boolean specialization is done for "Anything === Bool"
     // If the LHS is boolean, we set the specialization to Compare_Int32.
     // This matches other comparisons of the form bool === bool and
     // generated code of Compare_Int32 is more efficient.
     if (compare->compareType() == MCompare::Compare_Boolean &&
         def->getOperand(0)->type() == MIRType_Boolean)
     {
-       compare->setCompareType(MCompare::Compare_Int32);
+       compare->setCompareType(MCompare::Compare_Int32MaybeCoerceBoth);
     }
 
     // Compare_Boolean specialization is done for "Anything === Bool"
     // As of previous line Anything can't be Boolean
     if (compare->compareType() == MCompare::Compare_Boolean) {
         // Unbox rhs that is definitely Boolean
         MDefinition *rhs = def->getOperand(1);
         if (rhs->type() != MIRType_Boolean) {
@@ -237,19 +237,38 @@ ComparePolicy::adjustInputs(TempAllocato
             if (in->type() == MIRType_Null ||
                 (in->type() == MIRType_Boolean && convert == MToFloat32::NumbersOnly))
             {
                 in = boxAt(alloc, def, in);
             }
             replace = MToFloat32::New(alloc, in, convert);
             break;
           }
-          case MIRType_Int32:
-            replace = MToInt32::New(alloc, in);
+          case MIRType_Int32: {
+            MacroAssembler::IntConversionInputKind convert = MacroAssembler::IntConversion_NumbersOnly;
+            if (compare->compareType() == MCompare::Compare_Int32MaybeCoerceBoth ||
+                (compare->compareType() == MCompare::Compare_Int32MaybeCoerceLHS && i == 0) ||
+                (compare->compareType() == MCompare::Compare_Int32MaybeCoerceRHS && i == 1))
+            {
+                convert = MacroAssembler::IntConversion_Any;
+            }
+            if (convert == MacroAssembler::IntConversion_NumbersOnly) {
+                if (in->type() != MIRType_Int32 && in->type() != MIRType_Value)
+                    in = boxAt(alloc, def, in);
+            } else {
+                if (in->type() == MIRType_Undefined ||
+                    in->type() == MIRType_String ||
+                    in->type() == MIRType_Object)
+                {
+                    in = boxAt(alloc, def, in);
+                }
+            }
+            replace = MToInt32::New(alloc, in, convert);
             break;
+          }
           case MIRType_Object:
             replace = MUnbox::New(alloc, in, MIRType_Object, MUnbox::Infallible);
             break;
           case MIRType_String:
             replace = MUnbox::New(alloc, in, MIRType_String, MUnbox::Infallible);
             break;
           default:
             MOZ_ASSUME_UNREACHABLE("Unknown compare specialization");