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.

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.

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.

Unhiding based on previous comments.

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

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

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

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.

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 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 on attachment 9200509 [details]
Bug 1688346: Back out MSub::foldsTo on beta r=jandem

Approved for 86 beta 5, thanks.

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