Closed Bug 940635 Opened 6 years ago Closed 6 years ago

IonMonkey: (false == null) incorrectly evaluates to true

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jruderman, Assigned: bhackett)

References

(Blocks 2 open bugs)

Details

(Keywords: regression, testcase, Whiteboard: [qa-])

Attachments

(1 file)

function f(y) {
    return (y > 0) == y;
}
assertEq(f(0), true);
assertEq(f(0), true);
assertEq(f(null), false);
assertEq(f(null), false);


With --ion-eager:
a.js:7:0 Error: Assertion failed: got true, expected false


The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/3efe3f3d2c25
user:        Jan de Mooij
date:        Wed Jun 19 19:10:04 2013 +0200
summary:     Bug 882111 - Don't push an interpreter frame when calling into the JITs. r=djvj
Flags: needinfo?(jdemooij)
Regression from bug 862103. We specialize the comparison as Compare_Int32, but this doesn't handle |null| correctly, looks like.
Blocks: 862103
Flags: needinfo?(jdemooij) → needinfo?(bhackett1024)
Blocks: 945651
Attached patch patchSplinter Review
Similar to how Compare_Double needs to extra requirements to specify how its operands can be coerced in a generic scenario, Compare_Int32 can't use all coercions permitted by MToInt32.
Assignee: nobody → bhackett1024
Attachment #8342580 - Flags: review?(jdemooij)
Flags: needinfo?(bhackett1024)
Comment on attachment 8342580 [details] [diff] [review]
patch

Review of attachment 8342580 [details] [diff] [review]:
-----------------------------------------------------------------

Shu, I think you refactored the convertValueToInt code recently and I have a lot of reviews, would you mind taking this one?
Attachment #8342580 - Flags: review?(jdemooij) → review?(shu)
Comment on attachment 8342580 [details] [diff] [review]
patch

Review of attachment 8342580 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Let me recap what the patch does below to make sure I understand correctly:

 - Add a new IntConversionInputKind controls whether coercion of input is allowed in MToInt32
 - Existing behavior in the IRs that use MToInt32 is unchanged.
 - MCompare now only allows coercions for comparing int32s if the operands are some combination of int32 and bool.

::: js/src/jit/BaselineInspector.cpp
@@ +231,5 @@
> +            first->isCompare_Int32WithBoolean()
> +            ? first->toCompare_Int32WithBoolean()
> +            : (second && second->isCompare_Int32WithBoolean())
> +              ? second->toCompare_Int32WithBoolean()
> +              : nullptr;

Nit: I'd prefer parentheses around hook expressions for better eyeballing.

::: js/src/jit/IonMacroAssembler.h
@@ +1186,5 @@
>  
> +    enum IntConversionInputKind {
> +        IntConversion_NumbersOnly,
> +        IntConversion_Any
> +    };

Curious, why did you make this a new type?

::: js/src/jit/MIR.h
@@ +3052,5 @@
>    public:
>      INSTRUCTION_HEADER(ToInt32)
> +    static MToInt32 *New(TempAllocator &alloc, MDefinition *def,
> +                         MacroAssembler::IntConversionInputKind conversion =
> +                             MacroAssembler::IntConversion_Any)

Nit: I'm actually not clear on the style here -- should this line be flush with the |MacroAssembler| above?
Attachment #8342580 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #4)
> Curious, why did you make this a new type?

Which inputs the conversion should tolerate is orthogonal to IntConversionBehavior, which is about what outputs to generate.  Adding a new enum seemed better than doubling the number of values in the existing one.

https://hg.mozilla.org/integration/mozilla-inbound/rev/70ec3658b113
https://hg.mozilla.org/mozilla-central/rev/70ec3658b113
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.