1688346 - Issue involving IonMonkey and Math.trunc

Status: RESOLVED FIXED

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

Description

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

function f(x) {
    return Math.trunc(x) - Math.trunc(x);
}
f(0);
f(0);
f(0);
f(0);
f(0);
f(0);
f(0);
f(0);
f(0);
f(0.1);
f(1 / 0);
assertEq(f(1 / 0), NaN);

AR=ar sh ./configure --enable-debug --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Assertion fails on latest m-c rev 5dc361e890c3 with -fuzzing-safe --differential-testing --no-threads --ion-eager, passes when run with --fuzzing-safe --differential-testing --no-threads --baseline-eager --no-ion. --disable-bailout-loop-check does not seem to make the issue go away. Also passes when run on rev f4af0087a1b4, the parent of the potential regressor on both sets of flags.

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/2650cedbdcc5
user:        Iain Ireland
date:        Sat Jan 09 00:48:34 2021 +0000
summary:     Bug 1673497: Part 19: Invalidate warp lazily r=jandem

Unsure how bad this is, so marking s-s and setting needinfo? from :iain. Please open up if this is not the case.

Comment 1

[Iain Ireland [:iain]]

The regressor here is bug 1679121 (part 4). Bug 1673497 changed the timing of when we invalidate the script, but that's only relevant here because --ion-eager is enabled. Here's a very slightly modified version of the test that works with --fast-warmup --no-threads:

function f(x) {
    return Math.trunc(x) - Math.trunc(x);
}
with ({}) {}
for (var i = 0; i < 50; i++) {
    f(0.1);
}
assertEq(f(NaN), NaN);

The problem is MSub::foldsTo, which folds x - x to 0 if x is Int32. MTrunc's result type is Int32, but if we fold it away then instead of bailing on NaN inputs, we incorrectly return 0.

This is a correctness problem, but at first glance I don't think it's exploitable.

anba: Can you think of a good way to rescue this optimization? If not, I think we might have to just remove it.

Comment 2

[André Bargull [:anba]]

Bug 1042823 had a similar issue which got fixed by adding a guarded MLimitedTruncate. This approach also seems to work here (, because using a guard instruction ensures MTrunc isn't DCE'ed away). And I also don't think this is exploitable.

Comment 3

[Christian Holler (:decoder)]

Unhiding based on previous comments.

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1688346: Preserve bailouts in MSub::foldsTo r=jandem

I'm using the GuardRangeBailouts flag here to match MCompare::tryFoldEqualOperands, which faces the same problem.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ecc869ad59e
Preserve bailouts in MSub::foldsTo r=jandem,nbp

Comment 6

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/6ecc869ad59e

Comment 7

[Pascal Chevrel:pascalc (PTO until April 26)]

This is an 86 regression, should we uplift this fix to beta?

Comment 8

[Iain Ireland [:iain]]

Not sure what the threshold is for uplifting non-ss correctness bugs.

Uplifting this fix to beta would be pretty safe, but it would be even safer just revert this patch in beta.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:iain, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 10

[Iain Ireland [:iain]]

Attachment: Bug 1688346: Back out MSub::foldsTo on beta r=jandem

Comment 11

[Iain Ireland [:iain]]

Comment on attachment 9200509 [details]
Bug 1688346: Back out MSub::foldsTo on beta r=jandem

Beta/Release Uplift Approval Request

• User impact if declined: Incorrect optimization leading to wrong results (0 vs NaN)
• 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): This patch backs out a new optimization that introduced a bug. The optimization has been fixed in Nightly, but for Beta it seems safest to back the optimization out entirely.
• String changes made/needed:

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9200509 [details]
Bug 1688346: Back out MSub::foldsTo on beta r=jandem

Approved for 86 beta 5, thanks.

Comment 13

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/aa113870f3aa