Closed Bug 897747 Opened 6 years ago Closed 6 years ago

IonMonkey: Incorrect result with MMod range analysis

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jruderman, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(1 file)

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].
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.
Summary: IonMonkey: Incorrect result with |or_| range analysis → IonMonkey: Incorrect result with MMod range analysis
Attached patch mmod-range.patchSplinter Review
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 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+
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
Closed: 6 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.