Closed Bug 1094052 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox36 --- affected

People

(Reporter: gkw, Assigned: sunfish)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [fuzzblocker][jsbugmon:update])

Attachments

(1 file)

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?
Flags: needinfo?(sunfish)
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.
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.
Assignee: nobody → sunfish
Flags: needinfo?(sunfish)
Attachment #8517459 - Flags: review?(nicolas.b.pierron)
Attachment #8517459 - Flags: review?(hv1989)
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!
Attachment #8517459 - Flags: review?(nicolas.b.pierron) → review+
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 ...
Attachment #8517459 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/mozilla-central/rev/aad4ae442e00
https://hg.mozilla.org/mozilla-central/rev/0fc375b5e7d7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.