Bug 1772506 part 2 - Add frame pointer to all rectifier frames. r=iain

We were doing this on 32-bit x86 but not other platforms.

This complicates the epilogue code a bit because we have to pop the frame pointer
now, but once we have frame pointers everywhere we can simplify this to the standard
epilogue sequence.

On x64, replace the use of `r9` with `FramePointer`. This is now very similar to x86.

Differential Revision: https://phabricator.services.mozilla.com/D148253

js/src/jit/BaselineBailouts.cpp

@@ -456,52 +456,11 @@ class MOZ_STACK_CLASS BaselineStackBuilder {
       return virtualPointerAtStackOffset(offset);
     }
 
+    // Rectifier - the FramePointer is pushed as the first value in the frame.
     MOZ_ASSERT(type == FrameType::Rectifier);
-    // Rectifier - behaviour depends on the frame preceding the rectifier frame,
-    // and whether the arch is x86 or not.  The x86 rectifier frame saves the
-    // frame pointer, so we can calculate it directly.  For other archs, the
-    // previous frame pointer is stored on the stack in the frame that precedes
-    // the rectifier frame.
     size_t priorOffset =
-        JitFrameLayout::Size() + topFrame->prevFrameLocalSize();
+        JitFrameLayout::Size() + topFrame->prevFrameLocalSize() - sizeof(void*);
-#if defined(JS_CODEGEN_X86)
-    // On X86, the FramePointer is pushed as the first value in the Rectifier
-    // frame.
-    priorOffset -= sizeof(void*);
     return virtualPointerAtStackOffset(priorOffset);
-#elif defined(JS_CODEGEN_ARM) || defined(JS_CODEGEN_ARM64) ||   \
-    defined(JS_CODEGEN_MIPS32) || defined(JS_CODEGEN_MIPS64) || \
-    defined(JS_CODEGEN_X64) || defined(JS_CODEGEN_LOONG64)
-    // On X64, ARM, ARM64, MIPS and LoongArch, the frame pointer save location
-    // depends on the caller of the rectifier frame.
-    BufferPointer<RectifierFrameLayout> priorFrame =
-        pointerAtStackOffset<RectifierFrameLayout>(priorOffset);
-    FrameType priorType = priorFrame->prevType();
-    MOZ_ASSERT(JSJitFrameIter::isEntry(priorType) ||
-               priorType == FrameType::IonJS ||
-               priorType == FrameType::BaselineStub);
-
-    // If the frame preceding the rectifier is an IonJS or entry frame,
-    // then once again the frame pointer does not matter.
-    if (priorType == FrameType::IonJS || JSJitFrameIter::isEntry(priorType)) {
-      return nullptr;
-    }
-
-    // Otherwise, the frame preceding the rectifier is a BaselineStub frame.
-    //  let X = STACK_START_ADDR + JitFrameLayout::Size() + PREV_FRAME_SIZE
-    //      X + RectifierFrameLayout::Size()
-    //        + ((RectifierFrameLayout*) X)->prevFrameLocalSize()
-    //        - BaselineStubFrameLayout::reverseOffsetOfSavedFramePtr()
-    size_t extraOffset =
-        RectifierFrameLayout::Size() + priorFrame->prevFrameLocalSize() +
-        BaselineStubFrameLayout::reverseOffsetOfSavedFramePtr();
-    return virtualPointerAtStackOffset(priorOffset + extraOffset);
-#elif defined(JS_CODEGEN_NONE)
-    (void)priorOffset;
-    MOZ_CRASH();
-#else
-#  error "Bad architecture!"
-#endif
   }
 };
 
@@ -1150,14 +1109,15 @@ bool BaselineStackBuilder::buildRectifierFrame(uint32_t actualArgc,
 
   size_t startOfRectifierFrame = framePushed();
 
-  // On x86-only, the frame pointer is saved again in the rectifier frame.
+  if (!writePtr(prevFramePtr(), "PrevFramePtr")) {
-#if defined(JS_CODEGEN_X86)
-  if (!writePtr(prevFramePtr(), "PrevFramePtr-X86Only")) {
     return false;
   }
-  // Follow the same logic as in JitRuntime::generateArgumentsRectifier.
   prevFramePtr_ = virtualPointerAtStackOffset(0);
-  if (!writePtr(prevFramePtr(), "Padding-X86Only")) {
+
+#ifdef JS_NUNBOX32
+  // 32-bit platforms push an extra padding word. Follow the same logic as in
+  // JitRuntime::generateArgumentsRectifier.
+  if (!writePtr(prevFramePtr(), "Padding")) {
     return false;
   }
 #endif

js/src/jit/JitFrames.cpp

@@ -2468,10 +2468,7 @@ void AssertJitStackInvariants(JSContext* cx) {
                              "The rectifier frame should keep the alignment");
 
           size_t expectedFrameSize =
-              0
+              sizeof(void*) /* frame pointer */
-#if defined(JS_CODEGEN_X86)
-              + sizeof(void*) /* frame pointer */
-#endif
               + sizeof(Value) *
                     (frames.callee()->nargs() + 1 /* |this| argument */ +
                      frames.isConstructing() /* new.target */) +

js/src/jit/arm/Trampoline-arm.cpp

@@ -488,10 +488,19 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
   // Get the topmost argument.
   {
     ScratchRegisterScope scratch(masm);
-    masm.ma_alu(sp, lsl(r8, 3), r3, OpAdd);  // r3 <- r3 + nargs * 8
+    masm.ma_alu(sp, lsl(r8, 3), r3, OpAdd);  // r3 <- sp + nargs * 8
     masm.ma_add(r3, Imm32(sizeof(RectifierFrameLayout)), r3, scratch);
   }
 
+  // NOTE: if this changes, fix the Baseline bailout code too!
+  // See BaselineStackBuilder::calculatePrevFramePtr and
+  // BaselineStackBuilder::buildRectifierFrame (in BaselineBailouts.cpp).
+  static_assert(sizeof(Value) == 2 * sizeof(void*));
+  static_assert(JitStackAlignment == sizeof(Value));
+  masm.push(FramePointer);
+  masm.mov(StackPointer, FramePointer);
+  masm.push(FramePointer);  // Padding.
+
   {
     Label notConstructing;
 
@@ -537,7 +546,7 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
   }
 
   // translate the framesize from values into bytes
-  masm.as_add(r6, r6, Imm8(1));
+  masm.as_add(r6, r6, Imm8(2));  // 2 for |this| + frame pointer and padding
   masm.ma_lsl(Imm32(3), r6, r6);
 
   // Construct sizeDescriptor.
@@ -569,6 +578,8 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
       break;
   }
 
+  // frame pointer
+  // padding
   // arg1
   //  ...
   // argN
@@ -583,6 +594,8 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
     masm.ma_dtr(IsLoad, sp, Imm32(12), r4, scratch, PostIndex);
   }
 
+  // frame pointer
+  // padding
   // arg1
   //  ...
   // argN               <- sp now; r4 <- frame descriptor
@@ -591,9 +604,12 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
   // sizeDescriptor
   // return address
 
-  // Discard pushed arguments.
+  // Discard pushed arguments, but not the pushed frame pointer.
-  masm.ma_alu(sp, lsr(r4, FRAMESIZE_SHIFT), sp, OpAdd);
+  masm.rshift32(Imm32(FRAMESIZE_SHIFT), r4);
+  masm.sub32(Imm32(sizeof(void*)), r4);
+  masm.addPtr(r4, sp);
 
+  masm.pop(FramePointer);
   masm.ret();
 }
 

js/src/jit/arm64/Trampoline-arm64.cpp

@@ -444,6 +444,17 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
   masm.Add(x3, masm.GetStackPointer64(), Operand(x8, vixl::LSL, 3));
   masm.Add(x3, x3, Operand(sizeof(RectifierFrameLayout)));
 
+  // Save the frame pointer. Add 1 to x7 to account for this in frame alignment
+  // and frame size computations.
+  //
+  // NOTE: if this changes, fix the Baseline bailout code too!
+  // See BaselineStackBuilder::calculatePrevFramePtr and
+  // BaselineStackBuilder::buildRectifierFrame (in BaselineBailouts.cpp).
+  static_assert(sizeof(Value) == sizeof(void*));
+  masm.push(FramePointer);
+  masm.moveStackPtrTo(FramePointer);
+  masm.Add(x7, x7, Operand(1));
+
   // Pad to a multiple of 16 bytes. This neglects the |this| value,
   // which will also be pushed, because the rest of the frame will
   // round off that value. See pushes of |argc|, |callee| and |desc| below.
@@ -526,12 +537,13 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
   // Get the size of the stack frame, and clean up the later fixed frame.
   masm.Ldr(x4, MemOperand(masm.GetStackPointer64(), 24, vixl::PostIndex));
 
-  // Now that the size of the stack frame sans the fixed frame has been loaded,
+  // Discard pushed arguments, but not the pushed frame pointer.
-  // add that onto the stack pointer.
+  masm.rshift32(Imm32(FRAMESIZE_SHIFT), r4);
-  masm.Add(masm.GetStackPointer64(), masm.GetStackPointer64(),
+  masm.sub32(Imm32(sizeof(void*)), r4);
-           Operand(x4, vixl::LSR, FRAMESIZE_SHIFT));
+  masm.addToStackPtr(r4);
 
-  // Pop the return address from earlier and branch.
+  // Pop the frame pointer and return address from earlier and branch.
+  masm.pop(FramePointer);
   masm.ret();
 }
 

js/src/jit/loong64/Trampoline-loong64.cpp

@@ -410,6 +410,8 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
   // Caller:
   // [arg2] [arg1] [this] [[argc] [callee] [descr] [raddr]] <- sp
 
+#error "Port changes from bug 1772506"
+
   // Add |this|, in the counter of known arguments.
   masm.loadPtr(
       Address(StackPointer, RectifierFrameLayout::offsetOfNumActualArgs()), s3);

js/src/jit/mips32/Trampoline-mips32.cpp

@@ -432,6 +432,8 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
   }
   masm.pushReturnAddress();
 
+#error "Port changes from bug 1772506"
+
   Register numActArgsReg = t6;
   Register calleeTokenReg = t7;
   Register numArgsReg = t5;

js/src/jit/mips64/Trampoline-mips64.cpp

@@ -448,6 +448,8 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
   // Caller:
   // [arg2] [arg1] [this] [[argc] [callee] [descr] [raddr]] <- sp
 
+#error "Port changes from bug 1772506"
+
   // Add |this|, in the counter of known arguments.
   masm.loadPtr(
       Address(StackPointer, RectifierFrameLayout::offsetOfNumActualArgs()), s3);

js/src/jit/x64/Trampoline-x64.cpp

@@ -506,28 +506,34 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
   static_assert(
       sizeof(JitFrameLayout) % JitStackAlignment == 0,
       "No need to consider the JitFrameLayout for aligning the stack");
+  static_assert((sizeof(Value) + sizeof(void*)) % JitStackAlignment == 0,
+                "No need to consider |this| and the frame pointer for "
+                "aligning the stack");
   static_assert(
       JitStackAlignment % sizeof(Value) == 0,
       "Ensure that we can pad the stack by pushing extra UndefinedValue");
   static_assert(IsPowerOfTwo(JitStackValueAlignment),
                 "must have power of two for masm.andl to do its job");
 
-  masm.addl(
+  masm.addl(Imm32(JitStackValueAlignment - 1 /* for padding */), rcx);
-      Imm32(JitStackValueAlignment - 1 /* for padding */ + 1 /* for |this| */),
-      rcx);
   masm.addl(rdx, rcx);
   masm.andl(Imm32(~(JitStackValueAlignment - 1)), rcx);
 
   // Load the number of |undefined|s to push into %rcx.
   masm.subq(r8, rcx);
-  masm.subq(Imm32(1), rcx);  // TODO: remove in the next patch
+
+  // NOTE: if this changes, fix the Baseline bailout code too!
+  // See BaselineStackBuilder::calculatePrevFramePtr and
+  // BaselineStackBuilder::buildRectifierFrame (in BaselineBailouts.cpp).
+  masm.push(FramePointer);
+  masm.movq(rsp, FramePointer);
 
   // Caller:
-  // [arg2] [arg1] [this] [ [argc] [callee] [descr] [raddr] ] <- rsp <- r9
+  // [arg2] [arg1] [this] [ [argc] [callee] [descr] [raddr] ] <- rsp
   // '--- #r8 ---'
   //
   // Rectifier frame:
-  // [undef] [undef] [undef] [arg2] [arg1] [this] [ [argc] [callee]
+  // [rbp'] [undef] [undef] [undef] [arg2] [arg1] [this] [ [argc] [callee]
   //                                                          [descr] [raddr] ]
   //        '------- #rcx --------' '--- #r8 ---'
 
@@ -536,8 +542,6 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
 
   masm.moveValue(UndefinedValue(), ValueOperand(r10));
 
-  masm.movq(rsp, r9);  // Save %rsp.
-
   // Push undefined. (including the padding)
   {
     Label undefLoopTop;
@@ -551,8 +555,10 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
   // Get the topmost argument.
   static_assert(sizeof(Value) == 8, "TimesEight is used to skip arguments");
 
-  // Get the topmost argument.
+  // Get the topmost argument. We did a push of %rbp earlier, so be sure to
-  BaseIndex b(r9, r8, TimesEight, sizeof(RectifierFrameLayout));
+  // account for this in the offset.
+  BaseIndex b(FramePointer, r8, TimesEight,
+              sizeof(RectifierFrameLayout) + sizeof(void*));
   masm.lea(Operand(b), rcx);
 
   // Push arguments, |nargs| + 1 times (to include |this|).
@@ -578,9 +584,11 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
     // thisFrame[numFormals] = prevFrame[argc]
     ValueOperand newTarget(r10);
 
-    // +1 for |this|. We want vp[argc], so don't subtract 1
+    // Load vp[argc]. Add sizeof(Value) for |this| and sizeof(void*) for the
-    BaseIndex newTargetSrc(r9, rdx, TimesEight,
+    // saved frame pointer.
-                           sizeof(RectifierFrameLayout) + sizeof(Value));
+    BaseIndex newTargetSrc(
+        FramePointer, rdx, TimesEight,
+        sizeof(RectifierFrameLayout) + sizeof(Value) + sizeof(void*));
     masm.loadValue(newTargetSrc, newTarget);
 
     // Again, 1 for |this|
@@ -591,15 +599,16 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
   }
 
   // Caller:
-  // [arg2] [arg1] [this] [ [argc] [callee] [descr] [raddr] ] <- r9
+  // [arg2] [arg1] [this] [ [argc] [callee] [descr] [raddr] ]
   //
   //
   // Rectifier frame:
-  // [undef] [undef] [undef] [arg2] [arg1] [this] <- rsp [ [argc] [callee]
+  // [rbp'] <- rbp [undef] [undef] [undef] [arg2] [arg1] [this] <- rsp [ [argc]
-  //                                                       [descr] [raddr] ]
+  //                                                 [callee] [descr] [raddr] ]
   //
 
-  // Construct descriptor.
+  // Construct descriptor, accounting for pushed frame pointer above
+  masm.lea(Operand(FramePointer, sizeof(void*)), r9);
   masm.subq(rsp, r9);
   masm.makeFrameDescriptor(r9, FrameType::Rectifier, JitFrameLayout::Size());
 
@@ -634,8 +643,12 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
   masm.shrq(Imm32(FRAMESIZE_SHIFT), r9);
   masm.pop(r11);  // Discard calleeToken.
   masm.pop(r11);  // Discard numActualArgs.
-  masm.addq(r9, rsp);  // Discard pushed arguments.
 
+  // Discard pushed arguments, but not the pushed frame pointer.
+  BaseIndex unwind(rsp, r9, TimesOne, -int32_t(sizeof(void*)));
+  masm.lea(Operand(unwind), rsp);
+
+  masm.pop(FramePointer);
   masm.ret();
 }
 

js/src/jit/x86/Trampoline-x86.cpp

@@ -453,10 +453,8 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
 
   masm.moveValue(UndefinedValue(), ValueOperand(ebx, edi));
 
-  // NOTE: The fact that x86 ArgumentsRectifier saves the FramePointer
+  // NOTE: if this changes, fix the Baseline bailout code too!
-  // is relied upon by the baseline bailout code.  If this changes,
+  // See BaselineStackBuilder::calculatePrevFramePtr and
-  // fix that code!  See the |#if defined(JS_CODEGEN_X86) portions of
-  // BaselineStackBuilder::calculatePrevFramePtr and
   // BaselineStackBuilder::buildRectifierFrame (in BaselineBailouts.cpp).
   masm.push(FramePointer);
   masm.movl(esp, FramePointer);  // Save %esp.
@@ -484,7 +482,7 @@ void JitRuntime::generateArgumentsRectifier(MacroAssembler& masm,
   }
 
   // Get the topmost argument. We did a push of %ebp earlier, so be sure to
-  // account for this in the offset
+  // account for this in the offset.
   BaseIndex b(FramePointer, esi, TimesEight,
               sizeof(RectifierFrameLayout) + sizeof(void*));
   masm.lea(Operand(b), ecx);