825716 - IonMonkey: Differential Testing: Getting different output w/without --ion-eager with %

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

(function() {
    print(["" % null])
})()

On js debug shell on m-c changeset 0d771761b9b3, there is a difference in output with and without --ion-eager.

$ ./js-opt-64-dm-0d771761b9b3-darwin 
js> (function() {
    print(["" % null])
})()
NaN
js> 


$ ./js-opt-64-dm-0d771761b9b3-darwin --ion-eager 
js> (function() {
    print(["" % null])
})()
0
js>

Comment 1

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

Due to skipped revisions, the first bad revision could be any of:
changeset:   106741:6cd206b37176
parent:      106740:b63bb39ed1c0
parent:      103644:a0240c1043ee
user:        David Anderson
date:        Wed Aug 29 17:51:24 2012 -0700
summary:     Merge from mozilla-central.

changeset:   106742:7bf95bb09233
parent:      106741:6cd206b37176
parent:      103794:706174d31a02
user:        David Anderson
date:        Wed Aug 29 17:57:37 2012 -0700
summary:     Merge from mozilla-central.

changeset:   106747:003feda8a0b3
parent:      106742:7bf95bb09233
parent:      106746:630296b1c46d
user:        David Anderson
date:        Wed Aug 29 17:58:13 2012 -0700
summary:     Merge.

changeset:   106748:8f2d38db4b56
user:        David Anderson
date:        Wed Aug 29 18:04:42 2012 -0700
summary:     Fix merge bustage.


This likely came in with the IonMonkey landing, which made it to 18.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

Some bugs in MMod::foldsTo:

"x == NaN" is always false:

    // NaN % x -> NaN
    if (lhsConstant && lhsd == NaN)
        return lhs();

    // x % NaN -> NaN
    if (rhsConstant && rhsd == NaN)
        return rhs();

Then it assumes y is not NaN, but this does not hold if rhsConstant == false.

    // NOTE: y cannot be NaN, 0, or -0 at this point
    // x % y -> x (where x == 0 || x == -0)
    if (lhsConstant && (lhsd == 0))
        return TryFold(this, lhs());

Furthermore, these are all edge cases, so the attached patch just removes them.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 696876 [details] [diff] [review]
Patch

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

Yay edge-case removal!

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a38fd747831a

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a38fd747831a

Comment 6

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

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected