1094052 - Assertion failure: Input shouldn't be negative zero., at jit/IonMacroAssembler.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

for (var j = 0; j < 9; j++) {
    Math.sign(-0) > (function() {})
}

asserts js debug shell on m-c changeset 62990ec7ad78 with --ion-eager --no-threads --ion-check-range-analysis at Assertion failure: Input shouldn't be negative zero., at jit/IonMacroAssembler.cpp.

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/f718ec4b4cb0
user:        Dan Gohman
date:        Tue Nov 04 10:26:30 2014 -0800
summary:     Bug 1073928 - IonMonkey: Represent negative zero explicitly in range analysis r=nbp,h4writer

Dan, is bug 1073928 a possible regressor?

Comment 1

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

Assertion failure: Input shouldn't be negative zero., at /Users/skywalker/trees/mozilla-central/js/src/jit/IonMacroAssembler.cpp:1524
Process 86646 stopped
* thread #1: tid = 0x6692a, 0x00000001050a005f, queue = 'com.apple.main-thread', stop reason = EXC_BREAKPOINT (code=EXC_I386_BPT, subcode=0x0)
    frame #0: 0x00000001050a005f
-> 0x1050a005f:  movabsq $0x101f5b060, %rax
   0x1050a0069:  pushq  %rcx
   0x1050a006a:  movabsq $0x101e15c50, %rcx
   0x1050a0074:  cmpl   $0x0, 0x9c(%rcx)
(lldb) bt
* thread #1: tid = 0x6692a, 0x00000001050a005f, queue = 'com.apple.main-thread', stop reason = EXC_BREAKPOINT (code=EXC_I386_BPT, subcode=0x0)
  * frame #0: 0x00000001050a005f
    frame #1: 0x0000000105068761
(lldb)

Setting this as [fuzzblocker] because this occurs very often.

Comment 2

[Dan Gohman [:sunfish]]

Attachment: range-math-sign-negative-zero.patch

Yes. Math.sign(-0) is -0, so its range needs to reflect that.

The attached patch fixes it. I also added a jsapi-tests test for it, and to take another step toward bug 892702.

The main fix is just the change to use the form of Range constructor which allows opRange.canBeNegativeZero() to be passed in. I also added min/max logic for the bounds so that the test for Math.sign(1) looks nicer.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8517459 [details] [diff] [review]
range-math-sign-negative-zero.patch

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

::: js/src/jit/RangeAnalysis.cpp
@@ +1120,5 @@
> +{
> +    return op->canBeNaN()
> +           ? nullptr
> +           : new(alloc) Range(Max(Min(op->lower_, 1), -1),
> +                              Max(Min(op->upper_, 1), -1),

nice!

@@ +1122,5 @@
> +           ? nullptr
> +           : new(alloc) Range(Max(Min(op->lower_, 1), -1),
> +                              Max(Min(op->upper_, 1), -1),
> +                              Range::ExcludesFractionalParts,
> +                              op->canBeNegativeZero(),

nit: Use the enum constructor to wrap this boolean.  We should avoid implicit constructors (even if this is an enum).

::: js/src/jsapi-tests/testJitRangeAnalysis.cpp
@@ +15,5 @@
> +
> +using namespace js;
> +using namespace js::jit;
> +
> +BEGIN_TEST(testJitRangeAnalysis_MathSign)

Awesome!

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8517459 [details] [diff] [review]
range-math-sign-negative-zero.patch

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

::: js/src/jit/RangeAnalysis.cpp
@@ +1117,5 @@
>  
> +Range *
> +Range::sign(TempAllocator &alloc, const Range *op)
> +{
> +    return op->canBeNaN()

can you do:

if (op->canBeNan())
    return nullptr;

return new ...

Comment 5

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aad4ae442e00

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aad4ae442e00
https://hg.mozilla.org/mozilla-central/rev/0fc375b5e7d7