1684085 - Incorrect truncation for Math.ceil(a/b) | 0

Status: RESOLVED FIXED

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

Description

[Iain Ireland [:iain]]

Found this while trying to figure out how one of my bailout changesets was breaking the cgc build, only to realize that I was just changing the timing of GC invalidations and exposing a pre-existing bug introduced by bug 1679121. A much simpler version of the testcase (run with --fast-warmup --no-threads):

with ({}) {}

function ceil(a, b) {
    return Math.ceil(a / b) | 0;
}

for (var i = 0; i < 50; i++) {
    ceil(5,5);
}

print(ceil(5,3));

It's a bit of a mess where several individually valid optimizations interact badly.

In tryAttachMathCeil, we have a fast-path if we only see int32 inputs, which generates a GuardToInt32. When the loop triggers a warp compilation of ceil, the transpiler will see that the MDiv feeding into the GuardToInt32 has type int32, and elide it. Later on, though, RangeAnalysis will look at the uses of the MDiv to determine whether it is safe to truncate the results. Because the only consumer is an MBitOr, we decide it's safe to truncate. But this means we don't generate a guard to verify that the division has no remainder, and print 1 instead of the correct result, 2.

I'm conservatively marking this s-s. I think it's probably just a correctness issue, but we're unlikely to land a patch before the new year, so if it's exploitable somehow, there's no sense advertising the bug.

Comment 1

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

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

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1684085: Add indirect truncate for Math.{ceil,floor,round} with int32 inputs. r=iain!

Use indirect truncations to fix the Warp equivalent of bug 1000605.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a511fb0c9fe1
Add indirect truncate for Math.{ceil,floor,round} with int32 inputs. r=iain

Comment 4

[Cristian Brindusan [:cbrindusan]]

Backed out changeset a511fb0c9fe1 (bug 1684085) for SM build bustage at math-indirect-truncate.js.

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=a511fb0c9fe17eb177f3361c5b6943eb1312fc42&selectedTaskRun=MjTzyLt6RmacUyY43GBHNQ.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=326233056&repo=autoland&lineNumber=40202

Comment 5

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/705fe2bb012c
Backed out changeset a511fb0c9fe1 for SM build bustage at math-indirect-truncate.js. CLOSED TREE

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae3cd38c893e
Add indirect truncate for Math.{ceil,floor,round} with int32 inputs. r=iain

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

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