1687661 - Issue involving IonMonkey and Math.trunc

Status: RESOLVED FIXED

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

Description

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

let x = [0, 0 / 0, 0.1, 1]
function f(x, y) {
    return Math.trunc(+(y ? x : y) || ~y);
}
for (let i = 0; i < 3; i++) {
    f(x[i], 0);
    f(x[i], 0);
    f(x[i], 1);
    if (i !== 2) {
        f(x[i], 1);
    } else {
        assertEq(f(x[i], 1), 0);
    }
}

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

Assertion fails on latest m-c rev a3cd8f83fefa with -fuzzing-safe --differential-testing --no-threads --ion-eager, using GCC 10.2.0, 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 680f46ca1194, the parent of the potential regressor on both sets of flags.

$ ./js-dbg-64-linux-x86_64-a3cd8f83fefa --fuzzing-safe --differential-testing --no-threads --ion-eager failed-assert.js 
failed-assert.js:12:17 Error: Assertion failed: got 0.1, expected 0
Stack:
  @failed-assert.js:12:0

May be related to bug 1686702.

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

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

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/256684d262e2
user:        Iain Ireland
date:        Fri Jan 15 17:59:56 2021 +0000
summary:     Bug 1686702: Fold to MToNumberInt32 r=jandem

Comment 2

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

Set release status flags based on info from the regressing bug 1686702

Comment 3

[Iain Ireland [:iain]]

My patch in bug 1686702 is missing a setGuard.

We have nodes like this:

26 phi bitnot23:Int32 unbox20:Double
...
31 unbox phi26 to Int32 (fallible)
32 return unbox32:Int32

In TypeAnalyzer, we see that phi26 has inputs of different types and insert a ToDouble in the previous block. As a result, unbox31 is trying to unbox a Double, so we insert a Box. But then return32, which uses BoxInputsPolicy, calls BoxAt on its operand. Because it is an Unbox, this code kicks in and looks past the Unbox, giving us the result:

26 phi toDouble33:Double unbox20:Double 
...
34 box phi26:Double 
31 unbox box34 to Int32 (fallible)
32 return box34:Value       

The return no longer directly depends on the unbox. That's not an immediate problem, because the unbox is fallible and therefore a guard. However, in GVN the code that I added kicks in and converts box34 and unbox31 into tonumberint32 phi27:Double, which is not a guard. It has no uses, so DCE eliminates it.

I don't think this can lead to type confusion, because it only occurs when the conversion has no uses. I think that this is just a correctness issue, but I'm not 100% certain.

Patch coming shortly.

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1687661: Mark ToNumberInt32 as guard r=jandem

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1687661: Add test r=jandem

Depends on D102468

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Iain Ireland [:iain] from comment #3)

I don't think this can lead to type confusion, because it only occurs when the conversion has no uses. I think that this is just a correctness issue, but I'm not 100% certain.

Should we unhide this?

Comment 7

[Iain Ireland [:iain]]

I don't think it would hurt to keep this hidden until I land the fix.

Comment 8

[Andrew McCreight [:mccr8]]

Thanks. I'll just mark this as sec-audit then.

Comment 9

[Nicolas B. Pierron [:nbp]]

In practice, off-by-1 issues caused by truncation could lead to bigger problem which allow to read beyond boundaries in some cases, such as when the error flow in Range Analysis and causes bad hypothesis.

We should indeed be careful with such errors.

I don't think this can lead to type confusion, because it only occurs when the conversion has no uses. I think that this is just a correctness issue, but I'm not 100% certain.

How something which has no uses can flow in the returned value of a function? Or this is a different issue than the bad assertion?

Comment 10

[Iain Ireland [:iain]]

nbp: See my description above. The function returns Math.trunc(X), but so far we've only seen Int32 values for X, so Math.trunc is transpiled to a fallible Unbox<int32>.

31 unbox phi26 to Int32 (fallible)
32 return unbox32:Int32

TypeAnalysis makes a series of individually reasonable changes that end up with us boxing the return value earlier, and not depending on the unbox except as a guard.

34 box phi26:Double 
31 unbox box34 to Int32 (fallible)
32 return box34:Value  

This will, unfortunately, always bail, even though (based on baseline IC information) we know that the double value is extremely likely to be an integer. This can cause bailout loops. In bug 1686702 I added some code in MUnbox::foldsTo that converts Unbox<int32>(Box<double>(X)) to ToNumberInt32(X), which will check whether the double can be represented as an Int32 before bailing.

However, I was not marking the ToNumberInt32 as a guard. In this case, it had no explicit uses, so DCE removed it, which meant that we didn't bail out when we should have.

There shouldn't be any concerns about bad values flowing into RangeAnalysis, because if it had any uses then DCE would not remove it.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/561fc15b994b57c6f1877843053928ec8ce20975
https://hg.mozilla.org/integration/autoland/rev/a9b27906568d98731f700c97e593b4252cc793e2
https://hg.mozilla.org/mozilla-central/rev/561fc15b994b
https://hg.mozilla.org/mozilla-central/rev/a9b27906568d

Comment 12

[Christian Holler (:decoder)]

(In reply to Iain Ireland [:iain] from comment #10)

There shouldn't be any concerns about bad values flowing into RangeAnalysis, because if it had any uses then DCE would not remove it.

Sounds like this is not s-s, can we unhide this bug :iain?

Comment 13

[Tom Ritter [:tjr]]

Similarly, if so, we should remove sec-audit