1693062 - Differential output with Math functions and Ion

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

The following testcase produces differential results on mozilla-central revision 20210216-00b18dc4bfac (debug build, run with --no-threads --differential-testing --fuzzing-safe test.js):

function testMathyFunction (f, inputs) {
  for (var j = 0; j < inputs.length; ++j)      
    for (var k = 0; k < inputs.length; ++k)
      print(f(inputs[j], inputs[k]))
}
mathy0 = (function(x, y) {
 return Math.asinh(y | 0) | 0 ? Math.fround(~ x) : Math.hypot(Math.sqrt(2**53), Math.sqrt([, ...[]]));
});
testMathyFunction(mathy0, [Math.PI, 2**53]);

With --no-threads --differential-testing --fuzzing-safe:

-4
94906265.62425156
-1
94906265.62425156

With --no-threads --differential-testing --fuzzing-safe --ion-eager:

-4
94906265.62425156
-1
94906264

Marking s-s until investigated because this is a JIT optimization bug.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jan de Mooij [:jandem]]

Iain, can you take a look at this one?

Comment 3

[Iain Ireland [:iain]]

Looked at it earlier today. It's not s-s. We're losing precision by specializing a sqrt to Float32 in a corner case involving FirstExecution bailouts. I'm still working on pinning down the details.

Comment 4

[Daniel Veditz [:dveditz]]

Losing precision doesn't seem like a security problem.

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1693062: Disable float32 optimization for implicitly used definitions r=jandem

When the only consumer of Math.sqrt(2**53) was in code that had never been executed, the float32 optimization tried to convert it to float32 (in MSqrt::trySpecializeFloat32) because we didn't see any consumers that couldn't accept float32. When we bailed out, we recovered the result as a float instead of a double, and lost precision (94906264 vs 94906265.62425156). Checking for implicit uses solves the problem.

bug1693062-01.js tests CheckUsesAreFloat32Consumers. bug1693062-02.js tests TypeAnalyzer::markPhiConsumers. (The difference is whether the instruction we want to specialize is implicitly used itself, or whether it is used by an implicitly used phi.) These are the only places that canConsumeFloat32 is called outside of assertions.

Comment 6

[Nicolas B. Pierron [:nbp]]

Setting S3, because it is possible to have work-around, however, I seems to me that this would be something to backport if applicable.

Comment 7

[Iain Ireland [:iain]]

It's pretty hard to trigger this by mistake. If you do, you end up with slightly less precise results 10 times, then you bail out and recompile with a concrete consumer of the incorrectly specialized instruction. There are no security concerns.

I think it's fine to let this ride the trains.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f689d230b14
Disable float32 optimization for implicitly used definitions r=jandem

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/5f689d230b14

Comment 10

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Bug appears to be fixed on mozilla-central 20210218214637-b82bd51a119e but BugMon was unable to reproduce using mozilla-central 20210216031051-00b18dc4bfac.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 11

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

:iain, since this bug contains a bisection range, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.