892291 - OdinMonkey: Crash [@ js::Invoke] or [@ js::CallJSNative]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

x = {}
x.toString = (function(stdlib, n, heap) {
    "use asm"
    var imul = stdlib.Math.imul
    var Float32ArrayView = new stdlib.Float32Array(heap)
    function f() {
        return +(+0 * Float32ArrayView[(imul(-800, 0xf8ba1243) | 0) % (7 | 0xffffffff) >> 2])
    }
    return f
})(this, {}, ArrayBuffer(4096))
x + ''

crashes js opt shell on m-c changeset 3fc610532baf with --no-ti --no-ion --no-baseline at js::Invoke and crashes js debug shell at js::CallJSNative

Locking s-s just-in-case, autoBisect is running now. Setting needinfo from Benjamin since Luke seems to be away for awhile.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I can reproduce on Mac as well, hitting this: Program received signal EXC_ARITHMETIC, Arithmetic exception.

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/f1088655c731
user:        Jan de Mooij
date:        Tue Jul 09 10:23:58 2013 +0200
summary:     Bug 864400 - Optimize ModI for non-constant power-of-2 divisors. r=h4writer

This iteration took 337.518 seconds to run.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

jandem, is bug 864400 a possible regressor?

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Currently investigating. Reduced the test case to

function a(stdlib) {
  "use asm";
  var imul = stdlib.Math.imul;
  function f() {
    return ((imul(-800, 0xf8ba1243)|0) % -1)|0;
  }
  return f;  
}
var f = a(this)
f()

It appears that the code gen doesn't generate the negative test part as the range for RHS is +infinite, while RHS == -1. Weird.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: proposed fix + test case

(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
> It appears that the code gen doesn't generate the negative test part as the
> range for RHS is +infinite, while RHS == -1. Weird.

Actually, the code gen part didn't generate the negative test part for LHS, not RHS. But RHS is also negative, as it's a truncated integer multiplication. The negative test isn't generated as the range for LHS is positive (actually +Infinity), and the reason for that is that the range analysis didn't take into account the fact that the multiplication is truncated. In this case, if we overflow the range of int32, we can't tell in which direction the overflow will happen and hence we can't determine the range any more.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 774353 [details] [diff] [review]
proposed fix + test case

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

::: js/src/ion/RangeAnalysis.cpp
@@ +941,5 @@
>          canBeNegativeZero_ = Range::negativeZeroMul(&left, &right);
>      setRange(Range::mul(&left, &right));
> +
> +    // Truncated multiplications could overflow in both directions
> +    if (isTruncated() && (range()->isUpperInfinite() || range()->isLowerInfinite()))

if (isTruncated && !range->isInt32())

@@ +942,5 @@
>      setRange(Range::mul(&left, &right));
> +
> +    // Truncated multiplications could overflow in both directions
> +    if (isTruncated() && (range()->isUpperInfinite() || range()->isLowerInfinite()))
> +        setRange(NULL);

I think it would be sane if computeRange would at least compute a Range, even if we can infer it directly from the return type of the multiplication.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix addressing comments

Thanks for the shorter method in the test! Also, I added a full int32 range for this particular case.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b5b936ad0bf

Comment 9

[Jan de Mooij [:jandem]]

Thanks for fixing this, Benjamin! So this is not a regression from bug 864400 but it uncovered an older bug.

On beta and aurora, MMul::computeRange() returns early if isTruncated(), so we don't have to uplift this.

Comment 10

[Jan de Mooij [:jandem]]

Regression from bug 889451.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b5b936ad0bf

Comment 12

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.