Last Comment Bug 685097 - IonMonkey: Implement a maximum recursion depth.
: IonMonkey: Implement a maximum recursion depth.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: general
:
:
Mentors:
Depends on: 707845 708441
Blocks: 670484
  Show dependency treegraph
 
Reported: 2011-09-07 01:29 PDT by Sean Stangl [:sstangl]
Modified: 2011-12-16 16:33 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case recursing infinitely. (66 bytes, application/javascript)
2011-09-07 01:29 PDT, Sean Stangl [:sstangl]
no flags Details
WIP: Handle a maximum recursion depth. (9.89 KB, patch)
2011-12-07 14:46 PST, Sean Stangl [:sstangl]
no flags Details | Diff | Splinter Review
Handle a maximum recursion depth. (11.67 KB, patch)
2011-12-12 16:12 PST, Sean Stangl [:sstangl]
no flags Details | Diff | Splinter Review
Enforce a maximum stack depth, v2. (12.10 KB, patch)
2011-12-14 17:06 PST, Sean Stangl [:sstangl]
no flags Details | Diff | Splinter Review
Enforce a maximum stack depth, v3. (12.10 KB, patch)
2011-12-15 00:07 PST, Sean Stangl [:sstangl]
dvander: review+
Details | Diff | Splinter Review

Description Sean Stangl [:sstangl] 2011-09-07 01:29:46 PDT
Created attachment 558751 [details]
Test case recursing infinitely.

From Bug 670484, ion function calls need a way to raise error upon over-recursion.
Comment 1 Sean Stangl [:sstangl] 2011-12-07 14:46:36 PST
Created attachment 579845 [details] [diff] [review]
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] 2011-12-12 16:12:44 PST
Created attachment 581082 [details] [diff] [review]
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] 2011-12-12 16:13:33 PST
Patch gets applied on top of changes from Bug 708441.
Comment 4 David Anderson [:dvander] 2011-12-13 13:30:13 PST
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] 2011-12-14 17:06:36 PST
Created attachment 581838 [details] [diff] [review]
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] 2011-12-14 21:34:30 PST
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] 2011-12-15 00:07:00 PST
Created attachment 581900 [details] [diff] [review]
Enforce a maximum stack depth, v3.

Same as v2, but rebased onto FunctionInfo changes.
Comment 8 David Anderson [:dvander] 2011-12-16 13:52:33 PST
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] 2011-12-16 15:05:34 PST
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] 2011-12-16 16:33:46 PST
http://hg.mozilla.org/projects/ionmonkey/rev/f5207516ab9c

Fix some ARM bustage.

Note You need to log in before you can comment on or make changes to this bug.