871848 - Assertion failure: (objBits >> 47) == 0, at ../dist/include/js/Value.h with --ion-regalloc=stupid

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

x = [];
x[3] = 2;
function f() {
    eval("\
         function r() {\
            arguments\
         }\
         ")
};
Array.prototype.forEach.call(x, (function() {
    f()
    f()
}))

asserts js debug shell on m-c changeset 4ff8aa4a7295 with --no-baseline --no-jm --ion-eager --ion-regalloc=stupid at Assertion failure: (objBits >> 47) == 0, at ../dist/include/js/Value.h

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   123670:f516a56f4dba
user:        Brian Hackett
date:        Mon Mar 04 07:21:16 2013 -0700
summary:     Bug 846330 - Bail out before trying to eval scripts containing 'arguments' in Ion, r=jandem.

Comment 1

[Nicolas B. Pierron [:nbp]]

(gdb) 
Hardware watchpoint 4: *(JSObject **) 0x7fffefbfd128

Old value = (JSObject *) 0x7fffef133060 [object global] delegate
New value = (JSObject *) 0x1155cf0 Cannot access memory at address 0x6d007500670072
js::StackFrame::initFromBailout (this=0x7fffefbfd110, cx=0x1157c60, iter=...) at /home/nicolas/mozilla/ionmonkey/js/src/ion/Bailouts.cpp:113
113                 flags_ |= StackFrame::HAS_SCOPECHAIN;
(gdb) l
108         } else {
109             Value scopeChain = iter.read();
110             JS_ASSERT(scopeChain.isObject() || scopeChain.isUndefined());
111             if (scopeChain.isObject()) {
112                 scopeChain_ = &scopeChain.toObject();
113                 flags_ |= StackFrame::HAS_SCOPECHAIN;
114                 if (isFunctionFrame() && fun()->isHeavyweight())
115                     flags_ |= StackFrame::HAS_CALL_OBJ;
116             }

I am not able to reproduce the assertion, but a segfault while accessing the scope chain.
In my case, the problem seems to be that when we bailout from IonMonkey back to the interpreter, we are reading a value for the scope chain which seems to be invalid.

The snapshot iterator does not seems to return the right value.

My guess is that the LIR instruction for FilterArguments does not trigger a clobber of all volatile registers in the stupid register allocator, and thus "rdx" got overwritten by the call to  js::ion::FilterArguments.

// Scope chain slot in the snapshot.
(gdb) p slot.knownType()
$36 = JSVAL_TYPE_OBJECT
(gdb) p slot.reg()
$37 = {code_ = JSC::X86Registers::edx}

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Save volatile registers before inlined calls followed by a bailout.

This patch removes 9 of the 13 remaining failures(*) of « jit_test.py --args='--ion-regalloc=stupid' --ion ».

This patch does not make GetDynamicName and FilterArguments more efficient, but they do guarantee that if there is any live register used in the snapshot, they would get spilled around the call-site.

We need to spill them for 2 reasons:
 - Snapshots are taking inputs of the LIR.
 - Calls are supposed to happen at the output point of the LIR.

To reconcile the 2, we need to either keep them alive by using saveVolatile, as these calls are GC-free.

The second best solution would be to guard the result of both of these operations in a separated MIR, dedicated for doing a bailout after the call.  And in addition to that, we should mark these calls as LSafeCallInstruction (NoGC), such as the register allocator can find a better register allocation for these LIR.

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> This patch removes 9 of the 13 remaining failures(*) of « jit_test.py
> --args='--ion-regalloc=stupid' --ion ».

(*) From
FAILURES:
    --ion-eager /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/arguments/dynamicBindings.js
    --ion-eager /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/arguments/strict-eval-mutation.js
    --ion-eager /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/arguments/strict-eval.js
    --ion-eager /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/arguments/strict-nested-eval.js
    --ion-eager /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/arrow-functions/arguments-4.js
    --no-jm /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/asm.js/testCall.js
    --ion-eager /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/asm.js/testCall.js
    --no-jm /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/asm.js/testFFI.js
    --ion-eager /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/asm.js/testFFI.js
    --ion-eager /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/basic/testOverwrittenArgumentsWithUndefined.js
    --ion-eager /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/debug/Script-source-01.js
    --no-jm /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/debug/Script-source-01.js
    --ion-eager /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/ion/evalCallingName.js

to:

FAILURES:
    --no-jm /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/asm.js/testCall.js
    --ion-eager /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/asm.js/testCall.js
    --no-jm /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/asm.js/testFFI.js
    --ion-eager /home/nicolas/mozilla/ionmonkey/js/src/jit-test/tests/asm.js/testFFI.js

Comment 4

[Hannes Verschore [:h4writer]]

Not sure I'm the best reviewer for this. So feel free to distribute to somebody else. Though I can get myself up to speed with this part if needed ...

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 753551 [details] [diff] [review]
Save volatile registers before inlined calls followed by a bailout.

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

Clearing review, to trigger action. As told in previous comment, I don't think I'm the right reviewer for this. But if needed I can familiarize myself with the code.

Comment 6

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

Landed on inbound after asking nbp on IRC:

https://hg.mozilla.org/integration/mozilla-inbound/rev/29e698ae2b5c

Comment 7

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

This caused orange at:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=29e698ae2b5c&showall=1

I'll backout when the tree reopens.

Comment 8

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

Nevermind waiting till the tree reopens, I added the magic words for the backout in a CLOSED TREE:

https://hg.mozilla.org/integration/mozilla-inbound/rev/72a95da7099f

Comment 9

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

Backout landed on m-c some time ago: http://hg.mozilla.org/mozilla-central/rev/72a95da7099f

Comment 10

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

Brian/Nicolas, is this still needed? (I no longer seem to assert with the testcase in comment 0)

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #10)
> Brian/Nicolas, is this still needed? (I no longer seem to assert with the
> testcase in comment 0)

I have no precise idea why it does not reproduce, I guess this might be related to some improvement of the stupid allocator, such as Bug 949668.

Comment 12

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

-> WORKSFORME then.