Closed Bug 1538083 Opened 7 months ago Closed 6 months ago

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

Categories

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

ARM64
All
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- disabled
firefox66 --- disabled
firefox67 --- fixed
firefox68 --- fixed

People

(Reporter: gkw, Assigned: sstangl)

References

(Blocks 3 open bugs)

Details

(Keywords: regression, testcase, Whiteboard: [fuzzblocker])

Attachments

(1 file)

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...

Flags: needinfo?(sstangl)
Flags: needinfo?(nicolas.b.pierron)

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

Summary: Differential Testing: Different output message involving IonMonkey on ARM64 and Math.trunc/Math.tan/2^33 → Differential Testing: Different output message involving IonMonkey on ARM64 and Math.trunc

P2, this reproduces. I'll take it.

Assignee: nobody → sstangl
Priority: -- → P2

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.

Status: NEW → ASSIGNED
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(sstangl)
Pushed by sstangl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f14763229806
Fix -0 handling in ARM64 visitTrunc(). r=nbp
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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

Flags: needinfo?(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:
Flags: needinfo?(sstangl)
Attachment #9056251 - Flags: approval-mozilla-beta?

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

OK for uplift for beta 12.

Attachment #9056251 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.