Closed Bug 882486 Opened 9 years ago Closed 9 years ago

IonMonkey: incorrect result with Math.abs

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox22 --- affected
firefox23 --- affected
firefox24 --- fixed

People

(Reporter: jruderman, Assigned: mjrosenb)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files)

function f(x, y) {
  return Math.abs(x & (0xffffffff - y)) % 3;
}
assertEq(f(0, 0), 0);
assertEq(f(0x80000000, 0), 2);


With --ion-eager: got -2, expected 2


Found by the asm differential testing in jsfunfuzz, which I ran with --ion-eager just for kicks.  OdinMonkey got it right!


The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/4c0d13ce4c4a
user:        Jan de Mooij
date:        Fri Apr 12 14:16:57 2013 +0200
summary:     Bug 859257 - Mark DoublePun values in ToIntWidth as volatile to work around a Clang bug. r=jwalden


That patch is sketchy (see http://blog.regehr.org/archives/959), but it would surprise me to see it cause new problems.
My guess is the issue is in MAbs, where we expect that the argument being an integer means that the result will also be an integer.  We explicitly disable the INT_MIN check during codegen when we detect that the argument will fit into an integer.  As far as I can tell, this code has been wrong since it landed in http://hg.mozilla.org/mozilla-central/rev/a4a0aa798038
Asking h4writer for review, since he wrote that code, and would know if my assumptions are *way* off.
Attachment #761824 - Flags: review?(hv1989)
Comment on attachment 761824 [details] [diff] [review]
/home/mjrosenb/patches/MAbsFallible-r0.patch

whoops.  turns out this patch was created against a repo with some other issues, and this doesn't actually fix anything.
Attachment #761824 - Flags: review?(hv1989)
last patch was waaay off. I've actually tested this one, and it makes the error go away.
Attachment #761945 - Flags: review?(hv1989)
Attachment #761945 - Flags: review?(hv1989) → review+
Bug 841666 introduced the bug, if I'm not mistaken, so prepare to backport to beta/aurora/nightly :(
https://hg.mozilla.org/mozilla-central/rev/219d5783b18f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Assignee: general → mrosenberg
You need to log in before you can comment on or make changes to this bug.