1112161 - Align16: IonMonkey fun.apply should keep the stack aligned.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Currently when a fun.apply call is generated, we produce some assembly code
which does a copy of the actual number of arguments.  This actual number of
arguments might cause the stack to be unaligned, as Value are only 8 bytes
long, and not 16 bytes.  We should add some padding before making such calls,
and make sure that this padding is well considered when iterating over the
frames.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Rename copyreg to extraStackSpace.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: IonMonkey: Pad fun.apply(...) stack.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Assert that Ion's fun.apply calls are correctly aligned.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8556473 [details] [diff] [review]
IonMonkey: Pad fun.apply(...) stack.

Marking as obsolete as this patch seems to cause some correctness issues in the test suite, even if the stack is aligned properly … I 'll investigate.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: IonMonkey: Pad fun.apply(...) stack.

Unfortunately, this code rewrite emitPushArguments, in order to add the
padding, while correctly copying the arguments.  The previous patch was
wrong as we where not conditionally taking into account the offset induced
by the padding.

To consider, at runtime, the additional padding, then we need an extra
register, but as I recall the fun.apply function is already tight in terms
of registers on x86, which explains why I am saving registers on the stack.

As, we are already saving one register, I decided to save a second one
instead of re-doing the computation of the alignment.  One big difference
with the previous version is that the loop is only making the copy of the
arguments, and the padding is just a non-written reserved space (except in
debug mode).

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8556472 [details] [diff] [review]
Rename copyreg to extraStackSpace.

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

Nice, this actually helped me to understand code generation.  Can you also update the comment in Lowering.cpp at [0]?

[0] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/Lowering.cpp#491

::: js/src/jit/CodeGenerator.cpp
@@ +3110,5 @@
>      // Call with an Ion frame or a rectifier frame.
>      {
>          // Create the frame descriptor.
>          unsigned pushed = masm.framePushed();
> +        masm.addPtr(Imm32(pushed), extraStackSpace);

At this point, it's not only the extraStackSpace, but all stack space.  Feel free to use a dummy Register variable stackSpace (initialized with extraStackSpace), and use it from here.

@@ -3151,5 @@
>          // Finally call the function in objreg, as assigned by one of the paths above.
>          uint32_t callOffset = masm.callJit(objreg);
>          markSafepointAt(callOffset, apply);
>  
>          // Recover the number of arguments from the frame descriptor.

This comment is wrong, isn't it? It's the stack space occupied by all arguments which is retrieved here, not nargs.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8556474 [details] [diff] [review]
Assert that Ion's fun.apply calls are correctly aligned.

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

Looks good.

::: js/src/jit/JitFrames.cpp
@@ +2726,2 @@
>          for (; !frames.done(); ++frames) {
> +            prevFrameSize = frameSize;

Do you mind moving this right above the frameSize assignment, below?

@@ +2742,5 @@
>                    "The frame size is optimal");
>              }
>  
> +            if (frames.type() == JitFrame_Exit) {
> +                // For the moment, we do not keep the alignment an exit frame.

nit: this sentence seems to lack words

@@ +2744,5 @@
>  
> +            if (frames.type() == JitFrame_Exit) {
> +                // For the moment, we do not keep the alignment an exit frame.
> +                // Doing so would imply some additional logic for x86, as the
> +                // ExitFrameLayout is not a multiple of the JitStackAlignment.

Actually, if we don't keep the exit frame aligned, it's because we don't need to.  If we're aligned in Ion, we won't need special alignment as the Jit alignment subsumes the ABI alignment on all platforms.  If that's correct, could you rephrase this comment please?

@@ +2761,5 @@
> +                InlineFrameIterator lastInlinedFrame(cx, &frames);
> +                jsbytecode *pc = lastInlinedFrame.pc();
> +                if (JSOp(*pc) == JSOP_FUNAPPLY) {
> +                    MOZ_RELEASE_ASSERT(prevFrameSize % JitStackAlignment == 0,
> +                      "The ion frame should keep the alignment");

How about making the assert message more specific? Like "Inlined fun.apply frames should keep alignment".

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8556548 [details] [diff] [review]
IonMonkey: Pad fun.apply(...) stack.

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

Looks good to me.

::: js/src/jit/CodeGenerator.cpp
@@ +3012,5 @@
> +    if (alignment > 1) {
> +        MOZ_ASSERT(frameSize() % JitStackAlignment == 0,
> +            "Stack padding assumes that the frameSize is correct");
> +        MOZ_ASSERT(alignment == 2);
> +        Label alreadyPadded;

nit: please call this variable alreadyAligned or noPaddingNeeded.

@@ +3030,5 @@
> +    // cannot be merged with the previous test, as not all architectures can
> +    // write below their stack pointers.
> +    if (alignment > 1) {
> +        MOZ_ASSERT(alignment == 2);
> +        Label alreadyPadded;

ditto

@@ +3052,5 @@
> +    // Compute the source and destination offsets into the stack.
> +    size_t argvSrcOffset = frameSize() + JitFrameLayout::offsetOfActualArgs();
> +    size_t argvDstOffset = 0;
> +
> +    // Save the extract stack space, and re-use the register as a base.

nit: s/extract/extra

@@ +3074,4 @@
>          Label loop;
>          masm.bind(&loop);
>  
> +        // As argvIndex is offset by 1, and we use the decBranchPtr instruction

s/offset/off

Comment 9

[Nicolas B. Pierron [:nbp]]

(Try) https://treeherder.mozilla.org/#/jobs?repo=try&revision=220ed7bf0305
https://hg.mozilla.org/integration/mozilla-inbound/rev/c85e10ce7b0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b94e871cfe12
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7154cb4aad2

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c85e10ce7b0c
https://hg.mozilla.org/mozilla-central/rev/b94e871cfe12
https://hg.mozilla.org/mozilla-central/rev/b7154cb4aad2