Closed Bug 1112162 Opened 5 years ago Closed 5 years ago

Align16: Rectifier frame should keep the stack aligned.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(3 files)

Currently, when there is an underflow of arguments, we create a rectifier
frame which allocates enough space to pad the argument vector with undefined
values.  We should add extra padding to the rectifier frame in order to keep
the same alignment across Jit calls.
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Kannan: Why do we have a frame pointer as part of the rectifier frame on x86?

Is there more of these frame pointers, in Ion frames for example?  I hope not, because we are working on doing the alignment of the JitFrameLayout so far.
Flags: needinfo?(kvijayan)
Note, the rectifier frame is not aligned, but this test check that the size
of the rectifier frame will not change the alignment of the callee.

This assertion add also other tests to ensure that we do not consume too
much stack for doing the stack alignment, and that there is enough space to
contain all formal arguments (otherwise this might be a security issue).
Attachment #8551827 - Flags: review?(benj)
This patch is adding extra undefined values as a padding for aligning the
stack as expected by the part 2.
Attachment #8551829 - Flags: review?(benj)
MIPS & ARM are working seamlessly as the JitStackAlignment is of 8, and everything which is pushed on the stack is either a Value (sizeof == 8) or a JitFrameLayout (sizeof == 16).

Apparently, only x86 has to push the FramePointer on the stack.
Comment on attachment 8551827 [details] [diff] [review]
part 2 - Add assertion & test case to verify that we keep the same alignment across the rectifier frame.

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

Nice

::: js/src/builtin/TestingFunctions.cpp
@@ +1341,5 @@
>      return true;
>  }
>  
>  static bool
> +TestingFunc_assertJitStackInvariants(JSContext *cx, unsigned argc, jsval *vp)

Wasn't it supposed to be part of another patch?

::: js/src/jit/JitFrames.cpp
@@ +2726,5 @@
> +
> +            if (frames.prevType() == JitFrame_Rectifier) {
> +                size_t calleeFp = reinterpret_cast<size_t>(frames.fp());
> +                size_t callerFp = reinterpret_cast<size_t>(frames.prevFp());
> +                size_t frameSize = callerFp - calleeFp;

before this, MOZ_ASSERT(callerFp >= calleeFp);
Attachment #8551827 - Flags: review?(benj) → review+
Comment on attachment 8551829 [details] [diff] [review]
part 1.x64 - Add padding in the rectifier frame to keep the stack alignment.

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

Mostly comments about comments, looks good to me, so r+

::: js/src/jit/x64/Trampoline-x64.cpp
@@ +371,5 @@
>      // Do not erase the frame pointer in this function.
>  
>      MacroAssembler masm(cx);
> +    // Caller:
> +    // [arg2] [arg1] [this] [[argc] [callee] [descr] [raddr]] <rsp

Please explain somewhere that the "<" symbol means "points to".

@@ +396,5 @@
> +    static_assert(JitStackAlignment % sizeof(Value) == 0,
> +      "Ensure that we can pad the stack by pushing extra UndefinedValue");
> +
> +    const uint32_t alignment = JitStackAlignment / sizeof(Value);
> +    MOZ_ASSERT(IsPowerOfTwo(alignment));

This could be a static_assert instead (if the compiler can statically inline IsPowerOfTwo?)

@@ +413,5 @@
> +    // '------ #r8 -------'
> +    //
> +    // Rectifier frame:
> +    // [undef] [undef] [undef] [arg2] [arg1] [this] [[argc] [callee] [descr] [raddr]]
> +    // '------- #rcx --------' '------ #r8 -------'

This ascii art is nice, but wrong: at this precise point, rcx contains the number of formals + this, JitStack-aligned.  The comment is correct after the subq below, though.

@@ +415,5 @@
> +    // Rectifier frame:
> +    // [undef] [undef] [undef] [arg2] [arg1] [this] [[argc] [callee] [descr] [raddr]]
> +    // '------- #rcx --------' '------ #r8 -------'
> +
> +    // Load the number of |undefined|s to push into %rcx.

nit: Load *into %rcx* the number of |undefined|s to push.

@@ +416,5 @@
> +    // [undef] [undef] [undef] [arg2] [arg1] [this] [[argc] [callee] [descr] [raddr]]
> +    // '------- #rcx --------' '------ #r8 -------'
> +
> +    // Load the number of |undefined|s to push into %rcx.
> +    masm.subq(r8, rcx);

Can you move this (subq and comment) next to the previous addl/andl where we also manipulate rcx, please?

@@ +429,5 @@
>          masm.j(Assembler::NonZero, &undefLoopTop);
>      }
>  
>      // Get the topmost argument.
> +    static_assert(sizeof(Value) == 8, "Index arguments on the stack");

Can you make the static_assert comment more explicit, like "TimesEight must be used to go through the argument's Value list"?

@@ +430,5 @@
>      }
>  
>      // Get the topmost argument.
> +    static_assert(sizeof(Value) == 8, "Index arguments on the stack");
> +    BaseIndex b = BaseIndex(r9, r8, TimesEight, sizeof(RectifierFrameLayout) - sizeof(Value));

How about adding a comment that the |- sizeof(Value)| is present because of |this|?  That's obvious right now, but it might not be in a few months.

@@ +435,3 @@
>      masm.lea(Operand(b), rcx);
>  
> +    // Copy & Push arguments, |nargs| + 1 times (to include |this|).

The previous comment still applies here, we don't have a |mov| for copying, so i'd say it's implied by the push?
Attachment #8551829 - Flags: review?(benj) → review+
Comment on attachment 8551855 [details] [diff] [review]
part 1.x86 - Add padding in the rectifier frame to keep the stack alignment.

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

Looks good to me, but I would like djvj to confirm this review, just to be sure this won't impact profiling in any ways.
Attachment #8551855 - Flags: review?(kvijayan)
Attachment #8551855 - Flags: review?(benj)
Attachment #8551855 - Flags: review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> ::: js/src/builtin/TestingFunctions.cpp
> @@ +1341,5 @@
> >      return true;
> >  }
> >  
> >  static bool
> > +TestingFunc_assertJitStackInvariants(JSContext *cx, unsigned argc, jsval *vp)
> 
> Wasn't it supposed to be part of another patch?

Yes, but I noticed after pushing the patch that I forgot this renaming.

Usually I do not miss comments, as I only add the r=<name> to the patch once I answered/fixed all the remarks from this person.
(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> > +    // Load the number of |undefined|s to push into %rcx.
> > +    masm.subq(r8, rcx);
> 
> Can you move this (subq and comment) next to the previous addl/andl where we
> also manipulate rcx, please?

I will do it, but notice that clustering instructions which have data-dependencies is a bad practice and might cause some slow down.

But I guess this would be out-of the way when (?) we add some instruction scheduling to our macro assembler.

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
> @@ +435,3 @@
> >      masm.lea(Operand(b), rcx);
> >  
> > +    // Copy & Push arguments, |nargs| + 1 times (to include |this|).
> 
> The previous comment still applies here, we don't have a |mov| for copying,
> so i'd say it's implied by the push?

The reason of the Copy, is that we do not Push some random value, but a Copy (Operand(rcx, 0)), that we masm.push() on the stack.
(In reply to Benjamin Bouvier [:bbouvier] from comment #8)
> Looks good to me, but I would like djvj to confirm this review, just to be
> sure this won't impact profiling in any ways.

This should not impact profiling in any way, as the ProfilingExitTailStub use the frame descriptor to step over the rectifier frame.
Comment on attachment 8551855 [details] [diff] [review]
part 1.x86 - Add padding in the rectifier frame to keep the stack alignment.

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

Yeah, this shouldn't really affect profiler stuff.  But just in case.. have you run this patch against jit-tests, with 'enableSPSProfiling()' on a debug build?
Attachment #8551855 - Flags: review?(kvijayan) → review+
https://hg.mozilla.org/mozilla-central/rev/a31fe829631e
https://hg.mozilla.org/mozilla-central/rev/e19c170e727f
https://hg.mozilla.org/mozilla-central/rev/85f601fa7b46
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.