1538083 - Differential Testing: Different output message involving IonMonkey on ARM64 and Math.trunc

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P2)

Description

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

x = [8589934592, -0];
for (let i = 0; i < 2; ++i) {
    print(uneval(Math.trunc(Math.tan(x[i]))));
}

Note that 8589934592 is 2^33 and 8589934591 (2^33 -1) does not seem to show the issue.

$ ./js-dbg-64-dm-armsim64-linux-x86_64-2c49e736571b --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js 
1
-0
$

$ ./js-dbg-64-dm-armsim64-linux-x86_64-2c49e736571b --fuzzing-safe --no-threads --ion-eager testcase.js 
1
0
$

Tested this on m-c rev 2c49e736571b.

My configure flags are:

AR=ar sh ./configure --enable-simulator=arm64 --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests --disable-cranelift

python3 -u -m funfuzz.js.compile_shell -b "--enable-debug --enable-more-deterministic --enable-simulator=arm64" -r 2c49e736571b

Setting needinfo? from Sean and Nicolas since this is IonMonkey on ARM64 (and does not show on a regular debug deterministic 64-bit shell). Also setting [fuzzblocker] because this is hard to differentiate from the other fuzzblocking compare_jit issues.

Bisection is underway...

Comment 1

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

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/afb2e1e1665f
user: Sean Stangl
date: Thu Mar 07 03:57:23 2019 +0000
summary: Bug 1528869 - Enable IonMonkey in the ARM64 shell, but keep it disabled in the browser. r=nbp

Comment 2

[Sean Stangl [:sstangl]]

P2, this reproduces. I'll take it.

Comment 3

[Sean Stangl [:sstangl]]

Attachment: Bug 1538083 - Fix -0 handling in ARM64 visitTrunc(). r=nbp

The existing truncation code did not correctly handle the case of negative zero.
The fix is to avoid using FCMP floating-point comparisons, and check
the sign bit explicitly in a GPR.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f14763229806
Fix -0 handling in ARM64 visitTrunc(). r=nbp

Comment 5

[Cosmin Sabou [:CosminS]]

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

Comment 6

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Do you think this is safe to uplift to beta or should we let it ride to release in 68?

Comment 7

[Sean Stangl [:sstangl]]

Comment on attachment 9056251 [details]
Bug 1538083 - Fix -0 handling in ARM64 visitTrunc(). r=nbp

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: Math calculations involving truncation of -0 will be incorrect.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fix was validated. But given that this functionality is currently broken in 64, even if it were still broken, it would just keep being broken.
• String changes made/needed:

Comment 8

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 9056251 [details]
Bug 1538083 - Fix -0 handling in ARM64 visitTrunc(). r=nbp

OK for uplift for beta 12.

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/releases/mozilla-beta/rev/31a6f6acd33f