981325 - Differential Testing: Different output message involving Math.fround

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

function f(x) {
    return Math.fround() ? x : x >> 0
}
for (var j = 0; j < 2; ++j) {
    print(f() !== Math.fround(0))
}

shows the following difference in output on m-c rev 1d34741edfd4, on a 64-bit debug deterministic shell.

$ ./js-dbg-64-dm-darwin-1d34741edfd4 --fuzzing-safe --ion-parallel-compile=off 4334.js
false
false

$ ./js-dbg-64-dm-darwin-1d34741edfd4 --fuzzing-safe --ion-parallel-compile=off --ion-eager 4334.js
false
true

My configure flags are:

CC="clang -Qunused-arguments" AR=ar CXX="clang++ -Qunused-arguments" sh ./configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --disable-threadsafe

Comment 1

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/39bfcadd6492
user:        Hannes Verschore
date:        Tue Nov 26 23:21:18 2013 +0100
summary:     Bug 942105 - IonMonkey: Remove the inlineUseCountRatio option, r=jandem

Hannes, is bug 942105 a likely regressor?

Comment 2

[Hannes Verschore [:h4writer]]

That revision changed some heuristics about when to inline. The revision probably just masks the regressions range, because it needs inlining to cause this behaviour.

The following code shows the same error, before the mentioned revision:

function f(x) {
    return Math.fround() ? x : x >> 0
}
for (var j = 0; j < 20; ++j)                                                              
    f();                                                                                              
                                                                                                      
for (var j = 0; j < 2; ++j) {                                                                         
    print(f() !== Math.fround(0))                                                                     
}

Comment 3

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

If so, this goes back to prior to Sep 2013, rev 37e29c27e6e8.

Jan, any ideas on how to move this forward?

Comment 4

[Hannes Verschore [:h4writer]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #3)
> If so, this goes back to prior to Sep 2013, rev 37e29c27e6e8.
> 
> Jan, any ideas on how to move this forward?

I don't think this is correct. I see the following that would make much more sense.

The first bad revision is:
changeset:   146523:8621bdc40841
user:        Benjamin Bouvier <[e-mail]>
date:        Wed Sep 11 02:10:17 2013 -0700
summary:     Bug 900257: Inline Math.fround in IonMonkey; r=sstangl

bbouvier could you have a look?

Comment 5

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

Oh, perhaps I got misunderstood. I meant it seems to go back prior to rev 37e29c27e6e8 (end-Sep).

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Fix + patch

One line fix: Ion considered that Float32 is a simple primitive type (while Double isn't), thus the comparison was carried out by comparing JS::Value bits, and we were comparing Int32Value(0) and DoubleValue(0), which are bitwise different.

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 8388562 [details] [diff] [review]
Fix + patch

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

Looking at the patch this is probably from the inception of float32. Could you mark the right affected flags and also try to push this upstream (after inbound is green)?
I would try to get it to beta and aurora. I haven't heard people complaining about this in release, so we can leave it for now.

::: js/src/jit/MIR.cpp
@@ +1765,5 @@
>  
>  static bool
>  ObjectOrSimplePrimitive(MDefinition *op)
>  {
>      // Return true if op is either undefined/null/bolean/int32 or an object.

While you are touching this code, can you also:
s/bolean/boolean

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks!

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Patch + test for uplift

Got r+ from h4writer.

[Approval Request Comment]
Regression caused by (bug #): bug 900257
User impact if declined: correctness issues
Testing completed (on m-c, etc.): m-i and m-c completed
Risk to taking this patch (and alternatives if risky): low if not no risk at all
String or IDL/UUID changes made by this patch: n/a

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8390456 [details] [diff] [review]
Patch + test for uplift

28 is going to published next week. So, release won't be updated.
For beta, I leave the call to Lukas.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/76ce85f8e0e3

Assuming this gets denied for beta (it probably will), might we still want this on b2g28 (v1.3)?

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8390456 [details] [diff] [review]
Patch + test for uplift

We can't take on any additional, untested risk to the FF28 final builds.  This will have to ride the trains from 29.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8390456 [details] [diff] [review]
Patch + test for uplift

See approval request comment above.