Closed Bug 759442 Opened 12 years ago Closed 12 years ago

IonMonkey: Create rectifier frames for overflow of arguments too.

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: nbp, Assigned: nbp)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

We need to recover the number of actual arguments from the stack, because due to optimizations, we may not follow the bytecode byte-by-byte and use some shortcuts such as optimizing js_fun_call.

js_fun_call optimization is not compatible with the current implementation of numActualArg[1] which is looking at the bytecode to recover the number of actual arguments.  Bug 735406 suggest a complex solution to handle overflow of arguments which may be more practical for huge overflow of arguments because we do not want to consume to much space on the stack.

This issue suggest a fast and easy to implement solution which implies creating a rectifier frame for overflow of arguments.  The rectifier frame should then always contain the number of actual arguments, and the formal arguments.  Formal arguments are expected by Ion compiled scripts and the number of actual arguments will then be present iff we have a rectifier frame — which means if the number of formal differs from the number of actual args.  The actual number of arguments is given by the ArgumentsRectifierReg which should be pushed when entering the rectifier frame.

[1] This issue can exists between 2 ionized functions or if the function is inlined.  It can be detected at every location using numActualArgs, such as bailouts, f.arguments and lately with arguments.length.
Push the actual number of argument on the stack or store the actual number of arguments in the snapshots. This patch looks more like the optimization describe in Bug 758357, but it does not try to optimize the case of overflow of arguments since we do not have such issue yet.

Modifying the rectifier frame to also handle overflow of arguments is trickier than the current implementation, which add an extra field to all JS frames.
Attachment #628967 - Flags: review?(dvander)
Attachment #628967 - Flags: feedback?(sstangl)
Comment on attachment 628967 [details] [diff] [review]
Store actual arguments on the stack/snapshots. (x64 only)

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

::: js/src/ion/Ion.cpp
@@ +998,5 @@
>  
>          if (fp->hasOverflowArgs()) {
>              int formalArgc = argc;
>              Value *formalArgv = argv;
> +            argc = numActualArgs + 1;

This starts getting dangerous, so let's come up with an MAX_OVERFLOW_ARGS limit and make sure CheckFrame doesn't let us splat 50,000 arguments onto the stack.

I think a comment explaining why we pass both argc and numActualArgs is needed as well, since the case where they are almost equal with fp->hasOverflowArgs() is a little confusing.

::: js/src/ion/IonBuilder.cpp
@@ +2655,5 @@
> +    // Make sure we can recover the actual number of arguments out of the resume
> +    // point. This is used to recover overflow/underflow of arguments of inlined
> +    // frames.
> +    if (GET_ARGC(pc) - 1 == argc)
> +        inlineResumePoint->setFunCall();

Is this code reachable? I don't see anywhere that we can inline through jsop_funcall.

::: js/src/ion/IonFrames.cpp
@@ +794,2 @@
>          // Skip over non-argument slots, as well as |this|.
> +        unsigned skipCount = (si_.slots() - 1) - numActualArgs - 1;

Is this supposed to be numActualArgs_? Otherwise, it looks like numActualArgs_ can be assigned immediately.

::: js/src/ion/MIR.cpp
@@ +383,5 @@
>      return ins->toParameter()->index() == index_;
>  }
>  
>  MCall *
> +MCall::New(JSFunction *target, size_t argc, size_t numActualArgs, bool construct)

The distinction between "argc" and "numActualArgs" is not clear to me here, so a comment somewhere would be good.

::: js/src/ion/MIR.h
@@ +4384,5 @@
> +    Mode mode_:2;
> +
> +    // isFunCall remove one when set from the number of actual arguments.  This
> +    // is usefull for cases where inlining matters.
> +    bool isFunCall_:1;

Avoid bitfields like the plague. If we don't want to bloat RPs, maybe add the bit to MCall instead. (The call and RP are tied together so the bit is reachable from one or the other - MCall::resumePoint, or MResumePoint::stack[-1] to MCall.)

@@ +4433,5 @@
>          return mode_;
>      }
> +    uint32 numActualArgs() const {
> +        JS_ASSERT(mode_ == Outer && js_CodeSpec[*pc_].format & JOF_INVOKE);
> +        return GET_ARGC(pc_) - (isFunCall_ ? 1 : 0);

We don't inline through a fun_call target, so when will this isFunCall_ case trigger?

::: js/src/ion/shared/CodeGenerator-shared.cpp
@@ +220,5 @@
>          jsbytecode *pc = mir->pc();
>          uint32 exprStack = mir->stackDepth() - block->info().ninvoke();
> +        uint32 numActualArgs = 0;
> +        if (it + 1 != end)
> +            numActualArgs = mir->numActualArgs();

Add a brief comment here explaining what numActualArgs means and why it's only set for (it + 1 != end)

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +136,5 @@
> +    // actual number of arguments without adding an extra argument to the enter
> +    // JIT.
> +    masm.movq(result, reg_argc);
> +    masm.unboxInt32(Operand(reg_argc, 0), reg_argc);
> +    masm.push(reg_argc);

Don't forget to update x86 and ARM as well.
Attachment #628967 - Flags: review?(dvander) → review+
(In reply to David Anderson [:dvander] from comment #2)
> ::: js/src/ion/IonBuilder.cpp
> @@ +2655,5 @@
> > +    // Make sure we can recover the actual number of arguments out of the resume
> > +    // point. This is used to recover overflow/underflow of arguments of inlined
> > +    // frames.
> > +    if (GET_ARGC(pc) - 1 == argc)
> > +        inlineResumePoint->setFunCall();
> 
> Is this code reachable? I don't see anywhere that we can inline through
> jsop_funcall.

hum … Good catch ! I was hoping that we were able to inline foo in “foo.call(a0, a1)”.

This means that the bit is not required and that the number of actual arguments for inline frames can be recovered from the JSScript.

> ::: js/src/ion/MIR.h
> @@ +4384,5 @@
> > +    Mode mode_:2;
> > +
> > +    // isFunCall remove one when set from the number of actual arguments.  This
> > +    // is usefull for cases where inlining matters.
> > +    bool isFunCall_:1;
> 
> Avoid bitfields like the plague. If we don't want to bloat RPs, maybe add
> the bit to MCall instead. (The call and RP are tied together so the bit is
> reachable from one or the other - MCall::resumePoint, or
> MResumePoint::stack[-1] to MCall.)

Ok, this bit should no longer exists since you highlighted the fact that we do not inline jsop_funcall calls.

But, the outerResumePoint contains the arguments of the call and cannot contain the MCall because we are still running inside the inlined function, and that we are not producing MCall to hold the return value when we inline.

> ::: js/src/ion/x64/Trampoline-x64.cpp
> @@ +136,5 @@
> > +    // actual number of arguments without adding an extra argument to the enter
> > +    // JIT.
> > +    masm.movq(result, reg_argc);
> > +    masm.unboxInt32(Operand(reg_argc, 0), reg_argc);
> > +    masm.push(reg_argc);
> 
> Don't forget to update x86 and ARM as well.

I will update this patch with the nits and the other archs.
Complete the previous patch with ARM and x86 support.
The logic is the same.
Attachment #632064 - Flags: review?(mrosenberg)
Attachment #632064 - Flags: review?(dvander)
Delta:
- Modify generateEnterJIT to make it less error prone on ARM and to use more macro-assembler instruction (more readable)
- Update generation of frames.
Attachment #632064 - Attachment is obsolete: true
Attachment #632064 - Flags: review?(mrosenberg)
Attachment #632064 - Flags: review?(dvander)
Attachment #632968 - Flags: review?(mrosenberg)
Attachment #632968 - Flags: review?(dvander)
Comment on attachment 632968 [details] [diff] [review]
Store actual arguments on the stack/snapshots. (arm / x86)

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

I haven't verified that all of the offsets are correct, but things will fail in pretty obvious ways if the offsets are not correct.

::: js/src/ion/arm/IonFrames-arm.h
@@ +92,5 @@
>  class IonJSFrameLayout : public IonEntryFrameLayout
>  {
>    protected:
>      void *calleeToken_;
> +    uintptr_t numActualArgs_;

this isn't a pointer, or pointer-like value, shouldn't it be a uint32?

@@ +212,5 @@
> +        return reinterpret_cast<IonNativeExitFrameLayout *>(footer());
> +    }
> +};
> +
> +class IonNativeExitFrameLayout

Since this works without inheriting from anything, I'm assuming it is correct.  Could you add in a comment explaining why this does not inherit from anything else?

@@ +218,5 @@
> +    IonExitFooterFrame footer_;
> +    IonExitFrameLayout exit_;
> +    uintptr_t argc_;
> +
> +    // We need to split the Value in 2 field of 32 bits, otherwise the C++

s/in/into/
s/field/fields/

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ -1357,1 @@
>  

It looks like two padding words are being removed here, which should be changing the layout of the structures that are around.  I don't remember the layouts of any structures being changed.

@@ +1366,1 @@
>  

oh, this is bad.  the leaveNoPool() needs to be after the instruction *after* the nop that was generated. This will work most of the time, but it will fail randomly.

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +157,5 @@
>      // 8, and not touch it otherwise
>      aasm->as_sub(sp, sp, Imm8(4));
>      aasm->as_orr(sp, sp, Imm8(4));
>  #endif
> +

it looks like you added a space in here

@@ +386,5 @@
>      // Including |this|, there are (|nargs| + 1) arguments to copy.
>      JS_ASSERT(ArgumentsRectifierReg == r8);
>  
> +    // Copy number of actual arguments into r0
> +    masm.ma_ldr(DTRAddr(sp, DtrOffImm(IonJSFrameLayout::offsetOfNumActualArgs())), r10);

Shouldn't this be r0?
Attachment #632968 - Flags: review?(mrosenberg) → review+
Comment on attachment 632968 [details] [diff] [review]
Store actual arguments on the stack/snapshots. (arm / x86)

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

::: js/src/ion/Ion.cpp
@@ +939,5 @@
>      JS_ASSERT(ion::IsEnabled(cx));
>      JS_ASSERT_IF(osrPc != NULL, (JSOp)*osrPc == JSOP_LOOPENTRY);
>  
> +    // Skip if there is too much arguments.
> +    if (fp->hasArgs() && fp->numActualArgs() <= js_IonOptions.maxStackArg)

Should this be >=?

::: js/src/ion/Ion.h
@@ +116,5 @@
>  
> +    // How many actual arguments are accepted on the C stack.
> +    //
> +    // Default: 4,096
> +    uint32 maxStackArg;

s/maxStackArg/maxStackArgs/

When possible should we have a test exploiting this boundary?

::: js/src/ion/LIR-Common.h
@@ +477,5 @@
>      MCall *mir() const {
>          return mir_->toCall();
>      }
>  
>      // TODO: Common this out with LCallGeneric.

-:TODO:

::: js/src/ion/x86/Trampoline-x86.cpp
@@ +134,5 @@
> +    // Push the number of actual arguments.  |result| is used to store the
> +    // actual number of arguments without adding an extra argument to the enter
> +
> +    // JIT.
> +    masm.mov(Operand(ebp, ARG_RESULT), eax);

JIT?

@@ +367,5 @@
>          masm.j(Assembler::NonZero, &undefLoopTop);
>      }
>  
>      // Get the topmost argument.
> +    BaseIndex b = BaseIndex(ebp, esi, TimesEight, sizeof(IonRectifierFrameLayout));

Why RectifierFrame instead of JSFrame?
Attachment #632968 - Flags: review?(dvander) → review+
(In reply to David Anderson [:dvander] from comment #7)
> Comment on attachment 632968 [details] [diff] [review]
> Store actual arguments on the stack/snapshots. (arm / x86)
> 
> Review of attachment 632968 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: js/src/ion/x86/Trampoline-x86.cpp
> @@ +367,5 @@
> >          masm.j(Assembler::NonZero, &undefLoopTop);
> >      }
> >  
> >      // Get the topmost argument.
> > +    BaseIndex b = BaseIndex(ebp, esi, TimesEight, sizeof(IonRectifierFrameLayout));
> 
> Why RectifierFrame instead of JSFrame?

Currently the Rectifier frame and the Ion JS frame are identical, except that the rectifier only hold the arguments for the next JS frame.  So any read made after the current stack pointer are read out of the rectifier frame.

Consequently, I replaced references to IonJSFrameLayout from generateArgumentsRectifier since this is inaccurate.
(In reply to Marty Rosenberg [:mjrosenb] from comment #6)
> Comment on attachment 632968 [details] [diff] [review]
> Store actual arguments on the stack/snapshots. (arm / x86)
> 
> Review of attachment 632968 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I haven't verified that all of the offsets are correct, but things will fail
> in pretty obvious ways if the offsets are not correct.
> 
> ::: js/src/ion/arm/IonFrames-arm.h
> @@ +92,5 @@
> >  class IonJSFrameLayout : public IonEntryFrameLayout
> >  {
> >    protected:
> >      void *calleeToken_;
> > +    uintptr_t numActualArgs_;
> 
> this isn't a pointer, or pointer-like value, shouldn't it be a uint32?

uintptr_t is an unsigned pointer which has the size of a pointer. I don't think this matter on most of the architecture that we are manipulating for the moment.  The right type would be size_t but it is easier to think with element of identical size.

> @@ +212,5 @@
> > +        return reinterpret_cast<IonNativeExitFrameLayout *>(footer());
> > +    }
> > +};
> > +
> > +class IonNativeExitFrameLayout
> 
> Since this works without inheriting from anything, I'm assuming it is
> correct.  Could you add in a comment explaining why this does not inherit
> from anything else?

The inheritance is only an implementation inheritance, no virtual are involved, so we can do a some casts to reinterpret the stack.  I do not inherit from the IonExitFrameLayout because this class is an offset of it which account for the footer.  C/C++ forbids extension of structure by the top, which means that the footer cannot be express as an extension of the previous and has to aggregate it.

> @@ +386,5 @@
> >      // Including |this|, there are (|nargs| + 1) arguments to copy.
> >      JS_ASSERT(ArgumentsRectifierReg == r8);
> >  
> > +    // Copy number of actual arguments into r0
> > +    masm.ma_ldr(DTRAddr(sp, DtrOffImm(IonJSFrameLayout::offsetOfNumActualArgs())), r10);
> 
> Shouldn't this be r0?

Nice catch.
https://hg.mozilla.org/projects/ionmonkey/rev/f8a6a5d28875
https://hg.mozilla.org/projects/ionmonkey/rev/ffabe9341aa2 (fix)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #628967 - Flags: feedback?(sstangl)
You need to log in before you can comment on or make changes to this bug.