685097 - IonMonkey: Implement a maximum recursion depth.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: Test case recursing infinitely.

From Bug 670484, ion function calls need a way to raise error upon over-recursion.

Comment 1

[Sean Stangl [:sstangl]]

Attachment: WIP: Handle a maximum recursion depth.

WIP patch for handling a maximum recursion depth. Very simple. The limit for IonMonkey is on the length of the C stack. I have not changed the default max length for the time being.

The limit check is very relaxed, since we need slack on the end to call functions after the limit has been reached.

Requires updating once bug 707845 is fixed.

Comment 2

[Sean Stangl [:sstangl]]

Attachment: Handle a maximum recursion depth.

Very simple. Uses the preexisting C stack depth. Expanding the default stack limit will be a separate bug.

Comment 3

[Sean Stangl [:sstangl]]

Patch gets applied on top of changes from Bug 708441.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 581082 [details] [diff] [review]
Handle a maximum recursion depth.

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

::: js/src/ion/CodeGenerator.cpp
@@ +373,5 @@
> +
> +    // Conditional forward (unlikely) branch.
> +    masm.branchPtr(Assembler::BelowOrEqual, StackPointer, limit, &overflow);
> +    // Unconditional forward (likely) branch.
> +    masm.jump(&noOverflow);

I think we should use an out-of-line path instead. It might also simplify things to generate the over-recursion check after the prologue instead of before (we could just drop the stack in the out-of-line path).

::: js/src/ion/x64/MacroAssembler-x64.h
@@ +289,4 @@
>      void loadPtr(const Address &address, Register dest) {
>          movq(Operand(address), dest);
>      }
> +    void loadPtr(void *address, Register dest) {

This should be |ImmWord Address| or something.

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +468,5 @@
>      masm.setupUnalignedABICall(f.argc(), temp);
>  
>      // Initialize and set the context parameter.
> +    masm.movq(ImmWord(&JS_THREAD_DATA(cx)->ionJSContext), ArgReg0);
> +    masm.movq(Operand(ArgReg0, 0x0), ArgReg0);

I actually prefer the old way because it's easier to read and reverse engineer the assembly. There shouldn't be any performance distance between the two.

::: js/src/ion/x86/Assembler-x86.h
@@ +261,4 @@
>              JS_NOT_REACHED("unexpected operand kind");
>          }
>      }
> +    void movl(void *addr, Register dest) {

No |void *| in assembler functions, since it's too easy for a gcthing to leak in and not get traced. Use Operand(void *) which is x86 specific. movl should already support this mode.

(Of course, Operand(void *) doesn't protect against accidental gcthings either, it probably should.)

Comment 5

[Sean Stangl [:sstangl]]

Attachment: Enforce a maximum stack depth, v2.

This uses an OOL path for the failure case. The new mechanism for OOL paths is actually tasteful.

Comment 6

[Sean Stangl [:sstangl]]

The actual patch will need #ifdefs around the code in generate() to disable generateOverRecursedCheck() on ARM -- we need CallVM on ARM. I'll file a follow-up bug when that moment arrives.

Comment 7

[Sean Stangl [:sstangl]]

Attachment: Enforce a maximum stack depth, v3.

Same as v2, but rebased onto FunctionInfo changes.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 581900 [details] [diff] [review]
Enforce a maximum stack depth, v3.

>+    // Generate the VMFunction calling wrapper.
>+    IonCompartment *ion = gen->cx->compartment->ionCompartment();
>+    IonCode *wrapper = ion->generateVMWrapper(gen->cx, ReportOverRecursedInfo);
>+    if (!wrapper)
>+        return false;
>+
>+    uint32 descriptor = MakeFrameDescriptor(masm.framePushed(), IonFrame_JS);
>+    masm.push(Imm32(descriptor));
>+
>+    // Call the function, which will throw.
>+    masm.call(wrapper);

All of this can just be callVM(), right?

Comment 9

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/projects/ionmonkey/rev/3841d6743781

Implemented as MCheckOverRecursed, before the non-OSR MStart, in order to use callVM().

Comment 10

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/projects/ionmonkey/rev/f5207516ab9c

Fix some ARM bustage.