Closed Bug 1112159 Opened 6 years ago Closed 6 years ago

Align16: Dynamically align the C++ -> Ion trampoline.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(6 files)

We need to align the C++ -> Jit trampoline such that the first frame is always
aligned on 16 bytes.
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Comment on attachment 8548291 [details] [diff] [review]
part 4 - Align arm entry frame.

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

::: js/src/jit/arm/Assembler-arm.h
@@ +161,5 @@
>    "the constant sections of the code buffer.  Thus it should be larger than the "
>    "alignment for SIMD constants.");
>  
> +static_assert(StackAlignment % SimdMemoryAlignment == 0,
> +  "Stack alignment should be larger than any of the alignment which are used for "

"any of the alingments"

::: js/src/jit/arm/Trampoline-arm.cpp
@@ +161,5 @@
> +    // At the end the register r4, is a pointer to the stack where the first
> +    // argument is expected by the Jit frame.
> +    //
> +    aasm->as_sub(r4, sp, O2RegImmShift(r1, LSL, 3));    // r4 = sp - argc*8
> +    masm.ma_and(Imm32(~(StackAlignment - 1)), r4, r4);

It would probably be nicer to use as_bic here rather than ma_and, but either is fine.
Attachment #8548291 - Flags: review?(mrosenberg) → review+
Comment on attachment 8548287 [details] [diff] [review]
part 1 - Add a testing function to check the stack alignment.

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

::: js/src/builtin/TestingFunctions.cpp
@@ +26,5 @@
>  #include "js/StructuredClone.h"
>  #include "js/UbiNode.h"
>  #include "js/UbiNodeTraverse.h"
>  #include "js/Vector.h"
> +#include "jit/JitFrameIterator.h"

nit: make check-style

@@ +2438,5 @@
>  "  Force a bailout out of ionmonkey (if running in ionmonkey)."),
>  
> +    JS_FN_HELP("assertValidJitStack", testingFunc_assertValidJitStack, 0, 0,
> +"assertValidJitStack()",
> +"  Iterates the Jit stack and check that it is safe and efficient."),

s/that it is safe and efficient/that stack invariants hold

::: js/src/jit/JitFrames.cpp
@@ +2742,5 @@
>  #endif
>  }
>  
> +void
> +assertValidJitStack(JSContext *cx)

nit: Capital letter please?

"valid" is quite vague, considering it actually check invariants and the function right above is called "checkInvariants", how about CheckJitStackInvariants, or something better you could think of?

@@ +2747,5 @@
> +{
> +    for (JitActivationIterator activations(cx->runtime()); !activations.done(); ++activations) {
> +        JitFrameIterator frames(activations);
> +        for (; !frames.done(); ++frames)
> +            ;

style-nit: not sure about the style guide, i'd prefer putting an empty {} here instead
Attachment #8548287 - Flags: review?(benj) → review+
Comment on attachment 8548292 [details] [diff] [review]
part 5 - Assert mips entry frame is aligned.

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

::: js/src/jit/mips/Assembler-mips.h
@@ +161,5 @@
>  // MIPS CPUs can only load multibyte data that is "naturally"
>  // four-byte-aligned, sp register should be eight-byte-aligned.
>  static const uint32_t ABIStackAlignment = 8;
>  static const uint32_t CodeAlignment = 4;
> +static const uint32_t StackAlignment = 8;

It seems to me that StackAlignment is the same as ABIStackAlignment. I think it was actually renamed some time ago. Why not use ABIStackAlignment?
Attachment #8548292 - Flags: review?(branislav.rankov) → review+
(In reply to Branislav Rankov [:rankov] from comment #8)
> Comment on attachment 8548292 [details] [diff] [review]
> part 5 - Assert mips entry frame is aligned.
> 
> Review of attachment 8548292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/mips/Assembler-mips.h
> @@ +161,5 @@
> >  // MIPS CPUs can only load multibyte data that is "naturally"
> >  // four-byte-aligned, sp register should be eight-byte-aligned.
> >  static const uint32_t ABIStackAlignment = 8;
> >  static const uint32_t CodeAlignment = 4;
> > +static const uint32_t StackAlignment = 8;
> 
> It seems to me that StackAlignment is the same as ABIStackAlignment. I think
> it was actually renamed some time ago. Why not use ABIStackAlignment?

Because we are dealing with 2 different way of generating code, and each has its own stack alignment which happens to be the same at the moment.
Comment on attachment 8548288 [details] [diff] [review]
part 2 - MacroAssembler::assertStackAlignment accepts an additional offset argument.

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

bit twiddling!

::: js/src/jit/MacroAssembler.h
@@ +1432,5 @@
> +        offset %= alignment;
> +        if (offset < 0)
> +            offset += alignment;
> +
> +        // Test if each bit from offset is present.

s/present/set

@@ +1435,5 @@
> +
> +        // Test if each bit from offset is present.
> +        uint32_t off = offset;
> +        while (off) {
> +            uint32_t lowestBit = ((off ^ (off - 1)) + 1) >> 1;

can we use mfbt's MathAlgorithm CountTrailingZeroes32 here instead? That really is clearer when reading the code...

uint32_t lowestBit = 1 << CountTrailingZeroes32(off);
Attachment #8548288 - Flags: review?(benj) → review+
Comment on attachment 8548290 [details] [diff] [review]
part 3 - Align x86/x64 entry frame.

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

Cancelling review, sorry. Not because the patch is wrong, but because i would like to understand it more. Questions are in the biggest review comment.

::: js/src/jit/x64/Assembler-x64.h
@@ +178,5 @@
>  static MOZ_CONSTEXPR_VAR Register PreBarrierReg = rdx;
>  
>  static const uint32_t ABIStackAlignment = 16;
>  static const uint32_t CodeAlignment = 16;
> +static const uint32_t StackAlignment = 16;

Wait, this should have a prefix as well, as ABIStackAlignment has one. How about JitStackAlignment?

::: js/src/jit/x64/Trampoline-x64.cpp
@@ +92,5 @@
>      masm.mov(rsp, r14);
>  
>      // Remember number of bytes occupied by argument vector
>      masm.mov(reg_argc, r13);
> +    masm.shll(Imm32(3), r13);   // r13 = argc * sizeof(Value)

Please static_assert(sizeof(Value) == 1 << 3);

@@ +97,4 @@
>  
> +    // Guarantee stack alignment of Jit frames.
> +    //
> +    // This code compensate for the offset created by the copy of the vector of

s/compensate/compensates

@@ +97,5 @@
>  
> +    // Guarantee stack alignment of Jit frames.
> +    //
> +    // This code compensate for the offset created by the copy of the vector of
> +    // arguments, such that the jit frame would be aligned once the return

s/would/will

@@ +102,5 @@
> +    // address is pushed on the stack.
> +    //
> +    // In the computation of the offset, we omit the size of the JitFrameLayout
> +    // which is pushed on the stack, as the JitFrameLayout size is a multiple of
> +    // the StackAlignment.

I don't see the connection between JitFrameLayout and this. Is it that we are pushing all members of JitFrameLayout onto the stack, and considering the stack size after having it pushed?

The previous comment talks about "argc, callee token, frame size and return address". Are these the members of JitFrameLayout? (members, including inherited members of JitFrameLayout don't seem to match these 4 values, because of either the members' names or their types)

Was it that the previous code (when we were subq'ing 8 to r12) was coincidentally correct?
Have you tried this patch on all platforms (especially win32, as there is the same change in the x86 trampoline)?

@@ +107,3 @@
>      masm.mov(rsp, r12);
>      masm.subq(r13, r12);
> +    MOZ_ASSERT(sizeof(JitFrameLayout) % StackAlignment == 0);

static_assert

@@ +108,5 @@
>      masm.subq(r13, r12);
> +    MOZ_ASSERT(sizeof(JitFrameLayout) % StackAlignment == 0);
> +    masm.andl(Imm32(StackAlignment - 1), r12);
> +    // r12 = (StackAlignment - 1) & (rsp - argc * sizeof(Value))
> +    masm.subq(r12, rsp);        // rsp -= r12

These last two comments don't seem very useful

::: js/src/jit/x86/Assembler-x86.h
@@ +108,5 @@
>  #else
>  static const uint32_t ABIStackAlignment = 4;
>  #endif
>  static const uint32_t CodeAlignment = 16;
> +static const uint32_t StackAlignment = 16;

see remark in Assembler-x64.h

::: js/src/jit/x86/Trampoline-x86.cpp
@@ +75,5 @@
> +    // Guarantee stack alignment of Jit frames.
> +    //
> +    // This code compensate for the offset created by the copy of the vector of
> +    // arguments, such that the jit frame would be aligned once the return
> +    // address is pushed on the stack.

Actually, the previous comment is more explicit than this part, at least the 4 first lines. Can we just keep these lines, instead of this rephrasing? Can we also copy this to the x64 trampoline code?

@@ +84,3 @@
>      masm.movl(esp, ecx);
>      masm.subl(eax, ecx);
> +    MOZ_ASSERT(sizeof(JitFrameLayout) % StackAlignment == 0);

static_assert
Attachment #8548290 - Flags: review?(benj)
Comment on attachment 8548287 [details] [diff] [review]
part 1 - Add a testing function to check the stack alignment.

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

::: js/src/jit/JitFrames.cpp
@@ +2747,5 @@
> +{
> +    for (JitActivationIterator activations(cx->runtime()); !activations.done(); ++activations) {
> +        JitFrameIterator frames(activations);
> +        for (; !frames.done(); ++frames)
> +            ;

I think we've typically used |continue;| for this purpose.
(In reply to Benjamin Bouvier [:bbouvier] from comment #11)
> ::: js/src/jit/x64/Assembler-x64.h
> @@ +178,5 @@
> >  static MOZ_CONSTEXPR_VAR Register PreBarrierReg = rdx;
> >  
> >  static const uint32_t ABIStackAlignment = 16;
> >  static const uint32_t CodeAlignment = 16;
> > +static const uint32_t StackAlignment = 16;
> 
> Wait, this should have a prefix as well, as ABIStackAlignment has one. How
> about JitStackAlignment?

sounds good, I will do that in all patches.

> @@ +102,5 @@
> > +    // address is pushed on the stack.
> > +    //
> > +    // In the computation of the offset, we omit the size of the JitFrameLayout
> > +    // which is pushed on the stack, as the JitFrameLayout size is a multiple of
> > +    // the StackAlignment.
> 
> I don't see the connection between JitFrameLayout and this. Is it that we
> are pushing all members of JitFrameLayout onto the stack, and considering
> the stack size after having it pushed?
> 
> The previous comment talks about "argc, callee token, frame size and return
> address". Are these the members of JitFrameLayout? (members, including
> inherited members of JitFrameLayout don't seem to match these 4 values,
> because of either the members' names or their types)

They probably did, a while ago.

> Was it that the previous code (when we were subq'ing 8 to r12) was
> coincidentally correct?

No, I have no idea why this was made as such, I guess the idea might have been that we want to follow the same convention as the ABI alignment. We do not want the ABI alignment, as we want that the zero of the register allocator stack offset to be aligned with everything.

> Have you tried this patch on all platforms (especially win32, as there is
> the same change in the x86 trampoline)?

I tested locally with x86, x64, arm and mips.  But win32 should not be an issue as the formula remains correct independently of the original stack alignment.

> ::: js/src/jit/x86/Trampoline-x86.cpp
> @@ +75,5 @@
> > +    // Guarantee stack alignment of Jit frames.
> > +    //
> > +    // This code compensate for the offset created by the copy of the vector of
> > +    // arguments, such that the jit frame would be aligned once the return
> > +    // address is pushed on the stack.
> 
> Actually, the previous comment is more explicit than this part, at least the
> 4 first lines. Can we just keep these lines, instead of this rephrasing? Can
> we also copy this to the x64 trampoline code?

The previous comment was wrong, and the code associated to it was also wrong.

The previous comment details the content of the JitFrameLayout saying that we don't care about the last 2 fields, but that we do care about the first 2, which is wrong as all elements of the JitFrameLayout are of identical size.

I find the current comment less verbose and more descriptive.  Of course you have to know what a JitFrameLayout is, but then you can dig that from the code.
Comment on attachment 8548290 [details] [diff] [review]
part 3 - Align x86/x64 entry frame.

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

(asking again for review, with the previous comment)
Attachment #8548290 - Flags: review?(benj)
Comment on attachment 8548290 [details] [diff] [review]
part 3 - Align x86/x64 entry frame.

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

OK now i get it with your explanations, thanks. r=me with the nits as described in the previous review + a few comments i'd like to see updated as well.

::: js/src/jit/x64/Trampoline-x64.cpp
@@ +28,3 @@
>   *   bool blah(void *code, int argc, Value *argv, JSObject *scopeChain,
>   *               Value *vp)
>   *   ...using standard x64 fastcall calling convention

while you're here, can you update this comment to make it look more like the one in the x86 equivalent piece of code? Having a precise signature makes this comment less stable in time, while if we ever change the "EnterJitCode" name, a simple search and replace will catch it.

::: js/src/jit/x86/Trampoline-x86.cpp
@@ +74,5 @@
>  
> +    // Guarantee stack alignment of Jit frames.
> +    //
> +    // This code compensate for the offset created by the copy of the vector of
> +    // arguments, such that the jit frame would be aligned once the return

(same nits here as in the x64 version)

@@ +78,5 @@
> +    // arguments, such that the jit frame would be aligned once the return
> +    // address is pushed on the stack.
> +    //
> +    // In the computation of the offset, we omit the size of the JitFrameLayout
> +    // which is pushed on the stack, as the JitFrameLayout size is a multiple of

I would be more specific here: we push attributes and inherited attributes of a JitFrameLayout onto the stack.  It really confused me to see the JitFrameLayout name here and not have a direct connection with the following code.

@@ +87,5 @@
>  
>      // ecx = ecx & 15, holds alignment.
> +    masm.andl(Imm32(StackAlignment - 1), ecx);
> +    // ecx = (StackAlignment - 1) & (esp - argc * sizeof(Value))
> +    masm.subl(ecx, esp);        // esp -= ecx

The last two comments don't seem useful here as well.
Attachment #8548290 - Flags: review?(benj) → review+
(In reply to Marty Rosenberg [:mjrosenb] from comment #6)
> ::: js/src/jit/arm/Trampoline-arm.cpp
> @@ +161,5 @@
> > +    // At the end the register r4, is a pointer to the stack where the first
> > +    // argument is expected by the Jit frame.
> > +    //
> > +    aasm->as_sub(r4, sp, O2RegImmShift(r1, LSL, 3));    // r4 = sp - argc*8
> > +    masm.ma_and(Imm32(~(StackAlignment - 1)), r4, r4);
> 
> It would probably be nicer to use as_bic here rather than ma_and, but either
> is fine.

I cannot see any other uses for as_bic in the tree, I think I will keep the and for the moment.
Also, I would prefer if we could convert this code to use less Assembler, and more MacroAssembler code, unfortunately I did not found any equivalent for the previous as_sub code.
Blocks: 1130910
You need to log in before you can comment on or make changes to this bug.