IonMonkey: Incorrect result with MMod range analysis

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Unassigned)

Tracking

(Blocks: 1 bug, {testcase})

Trunk
mozilla25
x86_64
Mac OS X
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
function f(z)
{
  return (((0x80000000 | 0) % (0x80000001 | z)) | 0) % 100000
}
assertEq(f(0), -1);
assertEq(f(0), -1);

With --ion-eager, the second comparison fails: got 67295, expected -1.

Also affects OdinMonkey.

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/aeff293dadd3
user:        Dan Gohman
date:        Wed Jul 17 08:16:05 2013 -0700
summary:     Bug 891739 - IonMonkey: Make or_ and xor_ range analysis even more precise, and avoid calling js_bitscan_clz32 with a zero operand. r=nbp,waldo
It appears to be a bug uncovered in MMod::computeRange(). It is called with lhs=[INT_MIN,INT_MIN] and rhs=[INT_MIN,-1] and it computes a range of [0,0].

Comment 2

5 years ago
Yeah, that computation doesn't make any sense when the divisor is fully negative.  I think that could use the same treatment or_/xor_ got, to handle in separate cases a fully-negative divisor, a fully-positive divisor, and a divisor that could be negative or positive.  It also doesn't look like that code handles a potentially infinite rhs correctly.

Updated

5 years ago
Summary: IonMonkey: Incorrect result with |or_| range analysis → IonMonkey: Incorrect result with MMod range analysis
Created attachment 781050 [details] [diff] [review]
mmod-range.patch

This patch assumes Math.abs(x % y) == Math.abs(x) % Math.abs(y), which seems correct, but I'd appreciate confirmation :-).
Attachment #781050 - Flags: review?(jwalden+bmo)

Comment 4

5 years ago
Comment on attachment 781050 [details] [diff] [review]
mmod-range.patch

Review of attachment 781050 [details] [diff] [review]:
-----------------------------------------------------------------

It'd be good to have a test that gets mis-diagnosed by the |x % Infinity| case, in addition to this one.
Attachment #781050 - Flags: review?(jwalden+bmo) → review+

Comment 5

5 years ago
Actually Math.abs(x % y) is Math.abs(x) % y -- the sign of the result is the sign of the dividend.  But it's all the same as far as this patch is concerned.
https://hg.mozilla.org/mozilla-central/rev/5d14c20ba495
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.