Note: There are a few cases of duplicates in user autocompletion which are being worked on.

IonMonkey: Implement a maximum recursion depth.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sstangl, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 558751 [details]
Test case recursing infinitely.

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

Updated

6 years ago
Depends on: 707845
(Reporter)

Comment 1

6 years ago
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.
(Reporter)

Comment 2

6 years ago
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.
Attachment #579845 - Attachment is obsolete: true
Attachment #581082 - Flags: review?(dvander)
(Reporter)

Comment 3

6 years ago
Patch gets applied on top of changes from Bug 708441.
Depends on: 708441
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.)
Attachment #581082 - Flags: review?(dvander)
(Reporter)

Comment 5

6 years ago
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.
Attachment #581082 - Attachment is obsolete: true
Attachment #581838 - Flags: review?(dvander)
(Reporter)

Comment 6

6 years ago
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.
(Reporter)

Comment 7

6 years ago
Created attachment 581900 [details] [diff] [review]
Enforce a maximum stack depth, v3.

Same as v2, but rebased onto FunctionInfo changes.
Attachment #581838 - Attachment is obsolete: true
Attachment #581838 - Flags: review?(dvander)
Attachment #581900 - Flags: review?(dvander)
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?
Attachment #581900 - Flags: review?(dvander) → review+
(Reporter)

Comment 9

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

Implemented as MCheckOverRecursed, before the non-OSR MStart, in order to use callVM().
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Comment 10

6 years ago
http://hg.mozilla.org/projects/ionmonkey/rev/f5207516ab9c

Fix some ARM bustage.
You need to log in before you can comment on or make changes to this bug.