1132390 - Crash [@ js::ToPrimitive] or [@ js::ToNumberSlow]

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

function f(x) {
    return (Math.hypot(
        (Math.round() > 0) / Math.round(x),
        Math.hypot(x, Math.log(x ? x >>> 0 : x >>> 0))
    ));
};
m = [, , , , , , , 0, , , , 0, , , , , 0, 0, , 0, , 0, 0, , , 0, 0, 0, , 0, 0, []]
for (var j = 0; j < 32; ++j) {
    for (var k = 0; k < 32; ++k) {
        print(f(m[j]))
    }
}

crashes js debug shell on m-c changeset 38058cb42a0e with --fuzzing-safe --no-threads --baseline-eager --ion-gvn=off --ion-regalloc=backtracking at js::ToPrimitive with js::ToNumberSlow on the stack.

Debug configure options:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-optimize --enable-more-deterministic --enable-nspr-build --32 -R ~/trees/mozilla-central" -r 38058cb42a0e

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/150ea8d298b9
user:        Brian Hackett
date:        Fri Jan 30 08:05:44 2015 -0700
summary:     Bug 1124377 - Try to provide scratch registers for memory->memory MoveGroup moves, r=sunfish.

Brian, is bug 1124377 a likely regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0xbeaa, 0x0080999d js-dbg-32-dm-nsprBuild-darwin-38058cb42a0e`js::ToPrimitive(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) [inlined] JS::Handle<JSObject*>::operator->(this=0x00000000) const at jsobj.h:134, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x4)
  * frame #0: 0x0080999d js-dbg-32-dm-nsprBuild-darwin-38058cb42a0e`js::ToPrimitive(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>) [inlined] JS::Handle<JSObject*>::operator->(this=0x00000000) const at jsobj.h:134
    frame #1: 0x0080999d js-dbg-32-dm-nsprBuild-darwin-38058cb42a0e`js::ToPrimitive(cx=0x01e012c0, obj=JS::HandleObject at 0xbfffec34, hint=JSTYPE_NUMBER, vp=JS::MutableHandleValue at 0xbfffec3c) + 29 at jsobj.cpp:3319
    frame #2: 0x007f9bd0 js-dbg-32-dm-nsprBuild-darwin-38058cb42a0e`js::ToNumberSlow(js::ExclusiveContext*, JS::Value, double*) [inlined] JS::MutableHandle<JS::Value>::MutableHandle(root=0x01e012f4, preferredType=<unavailable>) + 74 at jsobjinlines.h:467
    frame #3: 0x007f9b86 js-dbg-32-dm-nsprBuild-darwin-38058cb42a0e`js::ToNumberSlow(cx=0x01e012c0, v=Value at 0xbfffec80, out=0xbfffeda0) + 182 at jsnum.cpp:1541
    frame #4: 0x007f9e4d js-dbg-32-dm-nsprBuild-darwin-38058cb42a0e`js::ToNumberSlow(cx=0x01e012c0, out=0xbfffed18, v=<unavailable>) + 45 at jsnum.cpp:1555
(lldb)

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

When considering whether a register can be used as a scratch register in a move group, we need to look not just at the operands of that move group but the operands of any immediately adjacent move groups.

Comment 3

[Dan Gohman [:sunfish]]

Comment on attachment 8564362 [details] [diff] [review]
patch

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

Ok, I debugged this a little myself and convinced myself that your approach here is reasonable, with some comments here. r+, provided that addressing the comments is straightforward.

::: js/src/jit/BacktrackingAllocator.cpp
@@ +1512,5 @@
>                      bool found = false;
> +                    LInstructionIterator niter(iter);
> +                    for (niter++; niter != block->end(); niter++) {
> +                        if (niter->isMoveGroup()) {
> +                            if (niter->toMoveGroup()->uses(alloc)) {

This checks whether the register is used or defined in a prior MoveGroup. Is it necessary to check both, or would it be sufficient to check just defines? Or, do you think it doesn't matter enough to be that precise?

@@ +1522,5 @@
>                          }
>                      }
> +                    if (iter != block->begin()) {
> +                        LInstructionIterator riter(iter);
> +                        for (riter--; riter != block->begin(); riter--) {

This reverse iteration will miss a MoveGroup at the beginning of the block. This code would be clearer using LInstructionReverseIterator to do the reverse iteration.

@@ +1524,5 @@
> +                    if (iter != block->begin()) {
> +                        LInstructionIterator riter(iter);
> +                        for (riter--; riter != block->begin(); riter--) {
> +                            if (riter->isMoveGroup()) {
> +                                if (riter->toMoveGroup()->uses(alloc)) {

Similar to above, this checks whether the register is used or defined in a latter MoveGroup; would it be sufficient to just check uses?

Additionally, if we see a def and we haven't seen a use yet, then we should know that the register isn't live before that, so we can skip any further MoveGroups. However, I guess this situation is going to be pretty rare and not worth worrying about.

::: js/src/jit/LIR-Common.h
@@ +156,5 @@
> +
> +    bool uses(LAllocation alloc) {
> +        for (size_t i = 0; i < numMoves(); i++) {
> +            LMove move = getMove(i);
> +            if (*move.from() == alloc || *move.to() == alloc)

Since this is platform-independent code, it would be appropriate to check for register aliasing here, rather than just equality. However, since this code is only used on x86, perhaps we can make it x86-only.

Comment 4

[Brian Hackett [Laid off!]]

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

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cc27fc8a8bdc