882486 - IonMonkey: incorrect result with Math.abs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

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.

Comment 1

[Marty Rosenberg [:mjrosenb]]

Attachment: /home/mjrosenb/patches/MAbsFallible-r0.patch

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.

Comment 2

[Marty Rosenberg [:mjrosenb]]

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.

Comment 3

[Marty Rosenberg [:mjrosenb]]

Attachment: /home/mjrosenb/patches/moreThanTruncate-r0.patch

last patch was waaay off. I've actually tested this one, and it makes the error go away.

Comment 4

[Hannes Verschore [:h4writer]]

Bug 841666 introduced the bug, if I'm not mistaken, so prepare to backport to beta/aurora/nightly :(

Comment 5

[Marty Rosenberg [:mjrosenb]]

landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/219d5783b18f

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/219d5783b18f