From 1228c556b699e0670227c85096f79e026e7e816a Mon Sep 17 00:00:00 2001 From: Georgia Kouveli Date: Wed, 29 Nov 2017 17:04:39 +0000 Subject: [PATCH] [arm64] ArgumentsAdaptorTrampoline fix for jssp removal. Even though a previous patch made the number of slots pushed/claimed on the stack aligned, the boundary between frames was not a multiple of two slots as well. We were pushing the number of arguments (which belongs in the ArgumentAdaptor frame) together with the arguments to pass to the callee (which belong to the frame of the callee). Those need to be separated so we can drop the arguments without messing up the alignment. Bug: v8:6644 Change-Id: I259c58db33a7c2726e5a3c74bcd67496f607d1d0 Reviewed-on: https://chromium-review.googlesource.com/793047 Reviewed-by: Ross McIlroy Commit-Queue: Georgia Kouveli Cr-Commit-Position: refs/heads/master@{#49759} --- src/builtins/arm/builtins-arm.cc | 11 +++---- src/builtins/arm64/builtins-arm64.cc | 41 ++++++++++++++------------ src/builtins/ia32/builtins-ia32.cc | 2 ++ src/builtins/mips/builtins-mips.cc | 13 ++++---- src/builtins/mips64/builtins-mips64.cc | 13 ++++---- src/builtins/ppc/builtins-ppc.cc | 13 ++++---- src/builtins/s390/builtins-s390.cc | 16 +++++----- src/builtins/x64/builtins-x64.cc | 2 ++ src/deoptimizer.cc | 4 +++ src/frame-constants.h | 3 +- 10 files changed, 68 insertions(+), 50 deletions(-) diff --git a/src/builtins/arm/builtins-arm.cc b/src/builtins/arm/builtins-arm.cc index 1c31009d93..dc2bdf7686 100644 --- a/src/builtins/arm/builtins-arm.cc +++ b/src/builtins/arm/builtins-arm.cc @@ -1799,8 +1799,9 @@ static void EnterArgumentsAdaptorFrame(MacroAssembler* masm) { __ mov(r4, Operand(StackFrame::TypeToMarker(StackFrame::ARGUMENTS_ADAPTOR))); __ stm(db_w, sp, r0.bit() | r1.bit() | r4.bit() | fp.bit() | lr.bit()); + __ Push(Smi::kZero); // Padding. __ add(fp, sp, - Operand(StandardFrameConstants::kFixedFrameSizeFromFp + kPointerSize)); + Operand(ArgumentsAdaptorFrameConstants::kFixedFrameSizeFromFp)); } static void LeaveArgumentsAdaptorFrame(MacroAssembler* masm) { @@ -1809,8 +1810,7 @@ static void LeaveArgumentsAdaptorFrame(MacroAssembler* masm) { // ----------------------------------- // Get the number of arguments passed (as a smi), tear down the frame and // then tear down the parameters. - __ ldr(r1, MemOperand(fp, -(StandardFrameConstants::kFixedFrameSizeFromFp + - kPointerSize))); + __ ldr(r1, MemOperand(fp, ArgumentsAdaptorFrameConstants::kLengthOffset)); __ LeaveFrame(StackFrame::ARGUMENTS_ADAPTOR); __ add(sp, sp, Operand::PointerOffsetFromSmiKey(r1)); @@ -2434,8 +2434,9 @@ void Builtins::Generate_ArgumentsAdaptorTrampoline(MacroAssembler* masm) { __ LoadRoot(scratch, Heap::kUndefinedValueRootIndex); __ sub(r4, fp, Operand(r2, LSL, kPointerSizeLog2)); // Adjust for frame. - __ sub(r4, r4, Operand(StandardFrameConstants::kFixedFrameSizeFromFp + - 2 * kPointerSize)); + __ sub(r4, r4, + Operand(ArgumentsAdaptorFrameConstants::kFixedFrameSizeFromFp + + kPointerSize)); Label fill; __ bind(&fill); diff --git a/src/builtins/arm64/builtins-arm64.cc b/src/builtins/arm64/builtins-arm64.cc index 875f261835..e4f22d0088 100644 --- a/src/builtins/arm64/builtins-arm64.cc +++ b/src/builtins/arm64/builtins-arm64.cc @@ -2064,10 +2064,9 @@ static void EnterArgumentsAdaptorFrame(MacroAssembler* masm) { __ Push(lr, fp); __ Mov(x11, StackFrame::TypeToMarker(StackFrame::ARGUMENTS_ADAPTOR)); __ Push(x11, x1); // x1: function - // We do not yet push the number of arguments, to maintain a 16-byte aligned - // stack pointer. This is done in step (3) in - // Generate_ArgumentsAdaptorTrampoline. - __ Add(fp, jssp, StandardFrameConstants::kFixedFrameSizeFromFp); + __ SmiTag(x11, x0); // x0: number of arguments. + __ Push(x11, padreg); + __ Add(fp, jssp, ArgumentsAdaptorFrameConstants::kFixedFrameSizeFromFp); } static void LeaveArgumentsAdaptorFrame(MacroAssembler* masm) { @@ -2076,8 +2075,7 @@ static void LeaveArgumentsAdaptorFrame(MacroAssembler* masm) { // ----------------------------------- // Get the number of arguments passed (as a smi), tear down the frame and // then drop the parameters and the receiver. - __ Ldr(x10, MemOperand(fp, -(StandardFrameConstants::kFixedFrameSizeFromFp + - kPointerSize))); + __ Ldr(x10, MemOperand(fp, ArgumentsAdaptorFrameConstants::kLengthOffset)); __ Mov(jssp, fp); __ Pop(fp, lr); @@ -2651,14 +2649,16 @@ void Builtins::Generate_ArgumentsAdaptorTrampoline(MacroAssembler* masm) { // 4 | num of | | // | actual args | | // |- - - - - - - - -| | - // [5] | [padding] | | + // 5 | padding | | // |-----------------+---- | - // 5+pad | receiver | ^ | + // [6] | [padding] | ^ | + // |- - - - - - - - -| | | + // 6+pad | receiver | | | // | (parameter 0) | | | // |- - - - - - - - -| | | - // 6+pad | parameter 1 | | | + // 7+pad | parameter 1 | | | // |- - - - - - - - -| Frame slots ----> expected args - // 7+pad | parameter 2 | | | + // 8+pad | parameter 2 | | | // |- - - - - - - - -| | | // | | | | // ... | ... | | | @@ -2671,7 +2671,8 @@ void Builtins::Generate_ArgumentsAdaptorTrampoline(MacroAssembler* masm) { // | [undefined] | v <-- stack ptr v // -----+-----------------+--------------------------------- // - // There is an optional slot of padding to ensure stack alignment. + // There is an optional slot of padding above the receiver to ensure stack + // alignment of the arguments. // If the number of expected arguments is larger than the number of actual // arguments, the remaining expected slots will be filled with undefined. @@ -2695,10 +2696,10 @@ void Builtins::Generate_ArgumentsAdaptorTrampoline(MacroAssembler* masm) { Register argc_unused_actual = x14; Register scratch1 = x15, scratch2 = x16; - // We need slots for the expected arguments, with two extra slots for the - // number of actual arguments and the receiver. + // We need slots for the expected arguments, with one extra slot for the + // receiver. __ RecordComment("-- Stack check --"); - __ Add(scratch1, argc_expected, 2); + __ Add(scratch1, argc_expected, 1); Generate_StackOverflowCheck(masm, scratch1, &stack_overflow); // Round up number of slots to be even, to maintain stack alignment. @@ -2738,7 +2739,9 @@ void Builtins::Generate_ArgumentsAdaptorTrampoline(MacroAssembler* masm) { __ Bind(&enough_arguments); // (2) Copy all of the actual arguments, or as many as we need. + Label skip_copy; __ RecordComment("-- Copy actual arguments --"); + __ Cbz(argc_to_copy, &skip_copy); __ Add(copy_end, copy_to, Operand(argc_to_copy, LSL, kPointerSizeLog2)); __ Add(copy_from, fp, 2 * kPointerSize); // Adjust for difference between actual and expected arguments. @@ -2755,12 +2758,12 @@ void Builtins::Generate_ArgumentsAdaptorTrampoline(MacroAssembler* masm) { __ Stp(scratch1, scratch2, MemOperand(copy_to, 2 * kPointerSize, PostIndex)); __ Cmp(copy_end, copy_to); __ B(hi, ©_2_by_2); + __ Bind(&skip_copy); - // (3) Store number of actual arguments and padding. The padding might be - // unnecessary, in which case it will be overwritten by the receiver. - __ RecordComment("-- Store number of args and padding --"); - __ SmiTag(scratch1, argc_actual); - __ Stp(xzr, scratch1, MemOperand(fp, -4 * kPointerSize)); + // (3) Store padding, which might be overwritten by the receiver, if it is not + // necessary. + __ RecordComment("-- Store padding --"); + __ Str(padreg, MemOperand(fp, -5 * kPointerSize)); // (4) Store receiver. Calculate target address from jssp to avoid checking // for padding. Storing the receiver will overwrite either the extra slot diff --git a/src/builtins/ia32/builtins-ia32.cc b/src/builtins/ia32/builtins-ia32.cc index 7635bada49..3f6c44dee0 100644 --- a/src/builtins/ia32/builtins-ia32.cc +++ b/src/builtins/ia32/builtins-ia32.cc @@ -1875,6 +1875,8 @@ static void EnterArgumentsAdaptorFrame(MacroAssembler* masm) { STATIC_ASSERT(kSmiTagSize == 1); __ lea(edi, Operand(eax, eax, times_1, kSmiTag)); __ push(edi); + + __ Push(Immediate(0)); // Padding. } static void LeaveArgumentsAdaptorFrame(MacroAssembler* masm) { diff --git a/src/builtins/mips/builtins-mips.cc b/src/builtins/mips/builtins-mips.cc index 167bc1b829..5fe8c77e5f 100644 --- a/src/builtins/mips/builtins-mips.cc +++ b/src/builtins/mips/builtins-mips.cc @@ -1804,8 +1804,9 @@ static void EnterArgumentsAdaptorFrame(MacroAssembler* masm) { __ sll(a0, a0, kSmiTagSize); __ li(t0, Operand(StackFrame::TypeToMarker(StackFrame::ARGUMENTS_ADAPTOR))); __ MultiPush(a0.bit() | a1.bit() | t0.bit() | fp.bit() | ra.bit()); - __ Addu(fp, sp, Operand(StandardFrameConstants::kFixedFrameSizeFromFp + - kPointerSize)); + __ Push(Smi::kZero); // Padding. + __ Addu(fp, sp, + Operand(ArgumentsAdaptorFrameConstants::kFixedFrameSizeFromFp)); } static void LeaveArgumentsAdaptorFrame(MacroAssembler* masm) { @@ -1814,8 +1815,7 @@ static void LeaveArgumentsAdaptorFrame(MacroAssembler* masm) { // ----------------------------------- // Get the number of arguments passed (as a smi), tear down the frame and // then tear down the parameters. - __ lw(a1, MemOperand(fp, -(StandardFrameConstants::kFixedFrameSizeFromFp + - kPointerSize))); + __ lw(a1, MemOperand(fp, ArgumentsAdaptorFrameConstants::kLengthOffset)); __ mov(sp, fp); __ MultiPop(fp.bit() | ra.bit()); __ Lsa(sp, sp, a1, kPointerSizeLog2 - kSmiTagSize); @@ -2489,8 +2489,9 @@ void Builtins::Generate_ArgumentsAdaptorTrampoline(MacroAssembler* masm) { __ sll(t2, a2, kPointerSizeLog2); __ Subu(t1, fp, Operand(t2)); // Adjust for frame. - __ Subu(t1, t1, Operand(StandardFrameConstants::kFixedFrameSizeFromFp + - 2 * kPointerSize)); + __ Subu(t1, t1, + Operand(ArgumentsAdaptorFrameConstants::kFixedFrameSizeFromFp + + kPointerSize)); Label fill; __ bind(&fill); diff --git a/src/builtins/mips64/builtins-mips64.cc b/src/builtins/mips64/builtins-mips64.cc index 811ae637ad..5c7210787a 100644 --- a/src/builtins/mips64/builtins-mips64.cc +++ b/src/builtins/mips64/builtins-mips64.cc @@ -1820,8 +1820,9 @@ static void EnterArgumentsAdaptorFrame(MacroAssembler* masm) { __ dsll32(a0, a0, 0); __ li(a4, Operand(StackFrame::TypeToMarker(StackFrame::ARGUMENTS_ADAPTOR))); __ MultiPush(a0.bit() | a1.bit() | a4.bit() | fp.bit() | ra.bit()); - __ Daddu(fp, sp, Operand(StandardFrameConstants::kFixedFrameSizeFromFp + - kPointerSize)); + __ Push(Smi::kZero); // Padding. + __ Daddu(fp, sp, + Operand(ArgumentsAdaptorFrameConstants::kFixedFrameSizeFromFp)); } static void LeaveArgumentsAdaptorFrame(MacroAssembler* masm) { @@ -1830,8 +1831,7 @@ static void LeaveArgumentsAdaptorFrame(MacroAssembler* masm) { // ----------------------------------- // Get the number of arguments passed (as a smi), tear down the frame and // then tear down the parameters. - __ Ld(a1, MemOperand(fp, -(StandardFrameConstants::kFixedFrameSizeFromFp + - kPointerSize))); + __ Ld(a1, MemOperand(fp, ArgumentsAdaptorFrameConstants::kLengthOffset)); __ mov(sp, fp); __ MultiPop(fp.bit() | ra.bit()); __ SmiScale(a4, a1, kPointerSizeLog2); @@ -2510,8 +2510,9 @@ void Builtins::Generate_ArgumentsAdaptorTrampoline(MacroAssembler* masm) { __ dsll(a6, a2, kPointerSizeLog2); __ Dsubu(a4, fp, Operand(a6)); // Adjust for frame. - __ Dsubu(a4, a4, Operand(StandardFrameConstants::kFixedFrameSizeFromFp + - 2 * kPointerSize)); + __ Dsubu(a4, a4, + Operand(ArgumentsAdaptorFrameConstants::kFixedFrameSizeFromFp + + kPointerSize)); Label fill; __ bind(&fill); diff --git a/src/builtins/ppc/builtins-ppc.cc b/src/builtins/ppc/builtins-ppc.cc index e0db87cc0c..e652aa334e 100644 --- a/src/builtins/ppc/builtins-ppc.cc +++ b/src/builtins/ppc/builtins-ppc.cc @@ -1867,8 +1867,9 @@ static void EnterArgumentsAdaptorFrame(MacroAssembler* masm) { } else { __ Push(fp, r7, r4, r3); } - __ addi(fp, sp, Operand(StandardFrameConstants::kFixedFrameSizeFromFp + - kPointerSize)); + __ Push(Smi::kZero); // Padding. + __ addi(fp, sp, + Operand(ArgumentsAdaptorFrameConstants::kFixedFrameSizeFromFp)); } static void LeaveArgumentsAdaptorFrame(MacroAssembler* masm) { @@ -1877,8 +1878,7 @@ static void LeaveArgumentsAdaptorFrame(MacroAssembler* masm) { // ----------------------------------- // Get the number of arguments passed (as a smi), tear down the frame and // then tear down the parameters. - __ LoadP(r4, MemOperand(fp, -(StandardFrameConstants::kFixedFrameSizeFromFp + - kPointerSize))); + __ LoadP(r4, MemOperand(fp, ArgumentsAdaptorFrameConstants::kLengthOffset)); int stack_adjustment = kPointerSize; // adjust for receiver __ LeaveFrame(StackFrame::ARGUMENTS_ADAPTOR, stack_adjustment); __ SmiToPtrArrayOffset(r0, r4); @@ -2524,8 +2524,9 @@ void Builtins::Generate_ArgumentsAdaptorTrampoline(MacroAssembler* masm) { __ ShiftLeftImm(r7, r5, Operand(kPointerSizeLog2)); __ sub(r7, fp, r7); // Adjust for frame. - __ subi(r7, r7, Operand(StandardFrameConstants::kFixedFrameSizeFromFp + - 2 * kPointerSize)); + __ subi(r7, r7, + Operand(ArgumentsAdaptorFrameConstants::kFixedFrameSizeFromFp + + kPointerSize)); Label fill; __ bind(&fill); diff --git a/src/builtins/s390/builtins-s390.cc b/src/builtins/s390/builtins-s390.cc index 42c478bd42..974b958d43 100644 --- a/src/builtins/s390/builtins-s390.cc +++ b/src/builtins/s390/builtins-s390.cc @@ -1854,7 +1854,8 @@ static void EnterArgumentsAdaptorFrame(MacroAssembler* masm) { // Old FP <--- New FP // Argument Adapter SMI // Function - // ArgC as SMI <--- New SP + // ArgC as SMI + // Padding <--- New SP __ lay(sp, MemOperand(sp, -5 * kPointerSize)); // Cleanse the top nibble of 31-bit pointers. @@ -1864,8 +1865,9 @@ static void EnterArgumentsAdaptorFrame(MacroAssembler* masm) { __ StoreP(r6, MemOperand(sp, 2 * kPointerSize)); __ StoreP(r3, MemOperand(sp, 1 * kPointerSize)); __ StoreP(r2, MemOperand(sp, 0 * kPointerSize)); - __ la(fp, MemOperand(sp, StandardFrameConstants::kFixedFrameSizeFromFp + - kPointerSize)); + __ Push(Smi::kZero); // Padding. + __ la(fp, + MemOperand(sp, ArgumentsAdaptorFrameConstants::kFixedFrameSizeFromFp)); } static void LeaveArgumentsAdaptorFrame(MacroAssembler* masm) { @@ -1874,8 +1876,7 @@ static void LeaveArgumentsAdaptorFrame(MacroAssembler* masm) { // ----------------------------------- // Get the number of arguments passed (as a smi), tear down the frame and // then tear down the parameters. - __ LoadP(r3, MemOperand(fp, -(StandardFrameConstants::kFixedFrameSizeFromFp + - kPointerSize))); + __ LoadP(r3, MemOperand(fp, ArgumentsAdaptorFrameConstants::kLengthOffset)); int stack_adjustment = kPointerSize; // adjust for receiver __ LeaveFrame(StackFrame::ARGUMENTS_ADAPTOR, stack_adjustment); __ SmiToPtrArrayOffset(r3, r3); @@ -2522,8 +2523,9 @@ void Builtins::Generate_ArgumentsAdaptorTrampoline(MacroAssembler* masm) { __ ShiftLeftP(r6, r4, Operand(kPointerSizeLog2)); __ SubP(r6, fp, r6); // Adjust for frame. - __ SubP(r6, r6, Operand(StandardFrameConstants::kFixedFrameSizeFromFp + - 2 * kPointerSize)); + __ SubP(r6, r6, + Operand(ArgumentsAdaptorFrameConstants::kFixedFrameSizeFromFp + + kPointerSize)); Label fill; __ bind(&fill); diff --git a/src/builtins/x64/builtins-x64.cc b/src/builtins/x64/builtins-x64.cc index f2820fa410..8ea4db38c4 100644 --- a/src/builtins/x64/builtins-x64.cc +++ b/src/builtins/x64/builtins-x64.cc @@ -1863,6 +1863,8 @@ static void EnterArgumentsAdaptorFrame(MacroAssembler* masm) { // arguments and the receiver. __ Integer32ToSmi(r8, rax); __ Push(r8); + + __ Push(Immediate(0)); // Padding. } static void LeaveArgumentsAdaptorFrame(MacroAssembler* masm) { diff --git a/src/deoptimizer.cc b/src/deoptimizer.cc index 56a5f0e23c..e82ba38833 100644 --- a/src/deoptimizer.cc +++ b/src/deoptimizer.cc @@ -1080,6 +1080,10 @@ void Deoptimizer::DoComputeArgumentsAdaptorFrame( PrintF(trace_scope_->file(), "(%d)\n", height - 1); } + output_offset -= kPointerSize; + WriteValueToOutput(isolate()->heap()->the_hole_value(), 0, frame_index, + output_offset, "padding "); + DCHECK_EQ(0, output_offset); Builtins* builtins = isolate_->builtins(); diff --git a/src/frame-constants.h b/src/frame-constants.h index 8d2d1f8cc4..038ee2dd3b 100644 --- a/src/frame-constants.h +++ b/src/frame-constants.h @@ -217,7 +217,8 @@ class ArgumentsAdaptorFrameConstants : public TypedFrameConstants { // FP-relative. static const int kFunctionOffset = TYPED_FRAME_PUSHED_VALUE_OFFSET(0); static const int kLengthOffset = TYPED_FRAME_PUSHED_VALUE_OFFSET(1); - DEFINE_TYPED_FRAME_SIZES(2); + static const int kPaddingOffset = TYPED_FRAME_PUSHED_VALUE_OFFSET(2); + DEFINE_TYPED_FRAME_SIZES(3); }; class BuiltinFrameConstants : public TypedFrameConstants {