831087 - IonMonkey: Differential Testing: Getting different output w/without --ion-eager with /=

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

try {
    for (y = 0; y < 1; y++) {
        x = y;
    }
} catch (e) {}

function f() {
    (x /= -9)
}
f()
print(uneval(this.x))


shows "-0" on js debug shell on m-c changeset 56ff556e74d9 without any CLI arguments but shows "0" with --ion-eager

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   114758:92c4255955d2
user:        Hannes Verschore
date:        Mon Dec 03 00:09:56 2012 +0100
summary:     Bug 816787: Remove negative zero check for truncated uses, r=mjrosenb

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Hannes, does bug 816787 seem like a likely regressor?

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Negative zero can only get removed when rhs is positive

The actual culprit is bug 736135. This landed before ionmonkey landed in FF18, therefore making it possible to encounter FF18+. Bug 816787 only makes it easier to get this faulty behaviour.

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 702725 [details] [diff] [review]
Negative zero can only get removed when rhs is positive

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

Looks like I forgot to qref, before posting.

::: js/src/ion/MIR.cpp
@@ +792,5 @@
>      // negative overflow check can be skipped.
>      if (lhs()->isConstant() && !lhs()->toConstant()->value().isInt32(INT32_MIN))
>          setCanBeNegativeZero(false);
>  
>      // If rhs is a constant int != -1, likewise.

// If rhs is a positive constant, likewise

@@ +793,5 @@
>      if (lhs()->isConstant() && !lhs()->toConstant()->value().isInt32(INT32_MIN))
>          setCanBeNegativeZero(false);
>  
>      // If rhs is a constant int != -1, likewise.
> +    if (rhs()->isConstant() && rhs()->toConstant()->value().toInt32() > 0)

if (rhs()->isConstant() && rhs()->toConstant()->value().toInt32() >= 0)

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Negative zero can only get removed when rhs is positive

Now the qref'ed patch with testcase. Sorry about the mess

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: Use canBeNegativeOverflow

Thanks Jan for noticing this. But regression range was correct. In that bug I wrongly changed two "canBeNegativeOverflow" to "canBeNegativeZero" !

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/445fd0026455

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Hannes, once this lands in m-c, we can request for landing on aurora.

Comment 8

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/445fd0026455

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 702750 [details] [diff] [review]
Use canBeNegativeOverflow

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 816787

User impact if declined: Wrongly reporting 0 instead of -0 for very very specific code running in IonMonkey.

Testing completed (on m-c, etc.): m-c

Risk to taking this patch (and alternatives if risky): I would say zero. This is two lines that are wrongly adjusted by bug 816787. This revert it to the original code.

String or UUID changes made by this patch: /

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 702750 [details] [diff] [review]
Use canBeNegativeOverflow

low risk regression fix, approving for aurora.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/26eda9e60b4d