521152 - TM: factorial computed incorrectly on ARM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

These scripts incorrectly give -2102132736 instead of 2432902008176640000 when running with -j on ARM.  Tested using rev 3a72a9e5ccd7+.

Smells of int/double confusion.  Related to bug 499889?

Tail-recursive version:

function factorial_tail(n, acc) { if (n == 0) { return acc; } return factorial_tail(n - 1, acc * n); }
print(factorial_tail(20, 1));

Iterative version:

var acc = 1;
for (var i=1; i<=20; ++i)
  acc *= i;
print(acc);

Comment 1

[Andreas Gal :gal]

Great testcase. Should be an easy fix. Trying to reproduce.

Comment 2

[Andreas Gal :gal]

Looks like ARM doesn't set the overflow flag on multiply. That is very sad.

Comment 3

[Andreas Gal :gal]

  004042afa8   mul r9,r10,r9          r0(state) r7(sp) r9(ld2) r10(ld3)
      ov1 = ov mul1       # codegen'd with the xt
      xt1: xt ov1 -> pc=0x2ba9a3 imacpc=(nil) sp+16 rp+0 (GuardID=002)
  004042afac   bvs 0x4042bed0         r0(state) r7(sp) r9(mul1) r10(ld3)

Comment 4

[Andreas Gal :gal]

Jacob, what can we do here? The overflow check can be pessimistic so we could check for (a | b) & 0xffff0000 != 0 for example.

Comment 5

[Andreas Gal :gal]

Attachment: disable integer multiplication on ARM

This will fix the issue until we have some code in place in the ARM backend.

Comment 6

[Andreas Gal :gal]

If we are considering vlad's arm firefox stuff part of the 1.9.2 cycle than this should block 1.9.2, but it doesn't have to block the upcoming x86 beta.

Comment 7

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/43ee7e151862

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/43ee7e151862

Comment 9

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ba365fe2fecc

Comment 10

[Christian Holler (:decoder)]

Bug in removed tracer code, setting in-testsuite- flag.