[Interpreter] Adds stackcheck in InterpreterPushArgsAndCall/Construct builtins.

src/builtins/ia32/builtins-ia32.cc

Index: src/builtins/ia32/builtins-ia32.cc
diff --git a/src/builtins/ia32/builtins-ia32.cc b/src/builtins/ia32/builtins-ia32.cc
index 00edd35cdca8cffc4b5190f19b70b5fdd7ebe682..3ecd9c5123175c180a059b85a2e4bd627053b0ac 100644
--- a/src/builtins/ia32/builtins-ia32.cc
+++ b/src/builtins/ia32/builtins-ia32.cc
@@ -703,6 +703,28 @@ void Builtins::Generate_InterpreterMarkBaselineOnReturn(MacroAssembler* masm) {
   __ ret(0);
 }
 
+static void Generate_StackOverflowCheck(MacroAssembler* masm, Register num_args,
+                                        Register scratch1, Register scratch2,
+                                        Label* stack_overflow) {
+  // Check the stack for overflow. We are not trying to catch
+  // interruptions (e.g. debug break and preemption) here, so the "real stack
+  // limit" is checked.
+  ExternalReference real_stack_limit =
+      ExternalReference::address_of_real_stack_limit(masm->isolate());
+  __ mov(scratch1, Operand::StaticVariable(real_stack_limit));
+  // Make scratch2 the space we have left. The stack might already be overflowed
+  // here which will cause scratch2 to become negative.
+  __ mov(scratch2, esp);
+  __ sub(scratch2, scratch1);
+  // Make scratch1 the space we need for the array when it is unrolled onto the
+  // stack.
+  __ mov(scratch1, num_args);
+  __ shl(scratch1, kPointerSizeLog2);
+  // Check if the arguments will overflow the stack.
+  __ cmp(scratch2, scratch1);
+  __ j(less_equal, stack_overflow);  // Signed comparison.
+}
+
 static void Generate_InterpreterPushArgs(MacroAssembler* masm,
                                          Register array_limit,
                                          Register start_address) {
@@ -732,18 +754,25 @@ void Builtins::Generate_InterpreterPushArgsAndCallImpl(
   //           they are to be pushed onto the stack.
   //  -- edi : the target to call (can be any Object).
   // -----------------------------------
+  Label stack_overflow;
+  // Compute the expected number of arguments.
+  __ mov(ecx, eax);
+  __ add(ecx, Immediate(1));  // Add one for receiver.
+
+  // Add a stack check before pushing the arguments. We need an extra register
+  // to perform a stack check. So push it onto the stack temporarily. This
+  // might cause stack overflow, but it will be detected by the check.
+  __ Push(edi);
+  Generate_StackOverflowCheck(masm, ecx, edx, edi, &stack_overflow);
+  __ Pop(edi);
 
   // Pop return address to allow tail-call after pushing arguments.
   __ Pop(edx);
 
   // Find the address of the last argument.
-  __ mov(ecx, eax);
-  __ add(ecx, Immediate(1));  // Add one for receiver.
   __ shl(ecx, kPointerSizeLog2);
   __ neg(ecx);
   __ add(ecx, ebx);
-
-  // TODO(mythria): Add a stack check before pushing the arguments.
   Generate_InterpreterPushArgs(masm, ecx, ebx);
 
   // Call the target.
@@ -759,6 +788,16 @@ void Builtins::Generate_InterpreterPushArgsAndCallImpl(
                                               tail_call_mode),
             RelocInfo::CODE_TARGET);
   }
+
+  __ bind(&stack_overflow);
+  {
+    // Pop the temporary registers, so that return address is on top of stack.
+    __ Pop(edi);

[rmcilroy, 2016/09/12 14:43:17]

Not really keen on this pop. Could you not just use esi instead of edi above to
avoid the extra push?

[mythria, 2016/09/13 09:53:33]

As discussed offline, we can't use esi, because it holds context. So we can't
avoid this pop.

+
+    __ TailCallRuntime(Runtime::kThrowStackOverflow);
+
+    __ int3();
+  }
 }
 
 namespace {
@@ -768,10 +807,10 @@ namespace {
 // original values are restored after the use.
 void Generate_InterpreterPushArgsAndReturnAddress(
     MacroAssembler* masm, Register num_args, Register start_addr,
-    Register scratch1, Register scratch2, bool receiver_in_args) {
+    Register scratch1, Register scratch2, bool receiver_in_args,
+    Label* stack_overflow) {
   // Store scratch2, scratch1 onto the stack. We need to restore the original
-  // values
-  // so store scratch2, scratch1 temporarily on stack.
+  // values so store scratch2, scratch1 temporarily on stack.
   __ Push(scratch2);
   __ Push(scratch1);
 
@@ -787,12 +826,29 @@ void Generate_InterpreterPushArgsAndReturnAddress(
   // | scratch2    |            | arg 0         |
   // | return addr |            | receiver slot |
 
+  // Check for stack overflow before we push arguments.
+  // Check the stack for overflow. We are not trying to catch
+  // interruptions (e.g. debug break and preemption) here, so the "real stack
+  // limit" is checked.
+  ExternalReference real_stack_limit =
+      ExternalReference::address_of_real_stack_limit(masm->isolate());
+  __ mov(scratch1, Operand::StaticVariable(real_stack_limit));
+  // Make scratch2 the space we have left. The stack might already be overflowed
+  // here which will cause scratch2 to become negative.
+  __ mov(scratch2, esp);
+  __ sub(scratch2, scratch1);
+
+  // Compute expected number of arguments.
+  __ mov(scratch1, num_args);
+  __ add(scratch1, Immediate(1));  // Add one for receiver.
+  __ shl(scratch1, kPointerSizeLog2);
+
+  __ cmp(scratch2, scratch1);
+  __ j(less_equal, stack_overflow);  // Signed comparison.
+
   // First increment the stack pointer to the correct location.
   // we need additional slots for arguments and the receiver.
   // Step 1 - compute the required increment to the stack.
-  __ mov(scratch1, num_args);
-  __ shl(scratch1, kPointerSizeLog2);
-  __ add(scratch1, Immediate(kPointerSize));
 
 #ifdef _MSC_VER
   // TODO(mythria): Move it to macro assembler.
@@ -813,7 +869,6 @@ void Generate_InterpreterPushArgsAndReturnAddress(
   __ bind(&update_stack_pointer);
 #endif
 
-  // TODO(mythria): Add a stack check before updating the stack pointer.
 
   // Step 1 - Update the stack pointer.
   __ sub(esp, scratch1);
@@ -879,11 +934,13 @@ void Builtins::Generate_InterpreterPushArgsAndConstructImpl(
   //           arguments should be consecutive above this, in the same order as
   //           they are to be pushed onto the stack.
   // -----------------------------------
+  Label stack_overflow;
 
   // Push arguments and move return address to the top of stack.
   // The eax register is readonly. The ecx register will be modified. The edx
   // and edi registers will be modified but restored to their original values.
-  Generate_InterpreterPushArgsAndReturnAddress(masm, eax, ecx, edx, edi, false);
+  Generate_InterpreterPushArgsAndReturnAddress(masm, eax, ecx, edx, edi, false,
+                                               &stack_overflow);
 
   __ AssertUndefinedOrAllocationSite(ebx);
   if (construct_type == CallableType::kJSFunction) {
@@ -901,6 +958,17 @@ void Builtins::Generate_InterpreterPushArgsAndConstructImpl(
     // Call the constructor with unmodified eax, edi, edx values.
     __ Jump(masm->isolate()->builtins()->Construct(), RelocInfo::CODE_TARGET);
   }
+
+  __ bind(&stack_overflow);
+  {
+    // Pop the temporary registers, so that return address is on top of stack.
+    __ Pop(edx);
+    __ Pop(edi);

[rmcilroy, 2016/09/12 14:43:17]

Really don't like this - it's impossible to see that these have been pushed
since they are pushed as scratch2 / scratch 1. Can you rework to avoid using
extra registers, or at least move the pushes to the same function as they are
poped

[mythria, 2016/09/13 09:53:33]

I reworked the code. I don't think we can avoid using registers, but I reworked
it so that Pushes and Pops happen in this function. We do not have any free
registers. Since we have to pop the return address before we push anything onto
the stack, we need some free registers. Two is the minimum we need. In the new
code, I use three to make the InterpreterPushArgsCopyArguments loop simpler. I
can revert back to the older version of using only two temporary registers.

+
+    __ TailCallRuntime(Runtime::kThrowStackOverflow);
+
+    __ int3();
+  }
 }
 
 // static
@@ -914,17 +982,30 @@ void Builtins::Generate_InterpreterPushArgsAndConstructArray(
   //           arguments should be consecutive above this, in the same order as
   //           they are to be pushed onto the stack.
   // -----------------------------------
+  Label stack_overflow;
 
   // Push arguments and move return address to the top of stack.
   // The eax register is readonly. The ecx register will be modified. The edx
   // and edi registers will be modified but restored to their original values.
-  Generate_InterpreterPushArgsAndReturnAddress(masm, eax, ecx, edx, ebx, true);
+  Generate_InterpreterPushArgsAndReturnAddress(masm, eax, ecx, edx, ebx, true,
+                                               &stack_overflow);
 
   // Array constructor expects constructor in edi. It is same as edx here.
   __ Move(edi, edx);
 
   ArrayConstructorStub stub(masm->isolate());
   __ TailCallStub(&stub);
+
+  __ bind(&stack_overflow);
+  {
+    // Pop the temporary registers, so that return address is on top of stack.
+    __ Pop(edx);
+    __ Pop(edi);
+
+    __ TailCallRuntime(Runtime::kThrowStackOverflow);
+
+    __ int3();
+  }
 }
 
 void Builtins::Generate_InterpreterEnterBytecodeDispatch(MacroAssembler* masm) {
@@ -2150,32 +2231,6 @@ void Builtins::Generate_StringConstructor_ConstructStub(MacroAssembler* masm) {
   }
 }
 
-static void ArgumentsAdaptorStackCheck(MacroAssembler* masm,
-                                       Label* stack_overflow) {
-  // ----------- S t a t e -------------
-  //  -- eax : actual number of arguments
-  //  -- ebx : expected number of arguments
-  //  -- edx : new target (passed through to callee)
-  // -----------------------------------
-  // Check the stack for overflow. We are not trying to catch
-  // interruptions (e.g. debug break and preemption) here, so the "real stack
-  // limit" is checked.
-  ExternalReference real_stack_limit =
-      ExternalReference::address_of_real_stack_limit(masm->isolate());
-  __ mov(edi, Operand::StaticVariable(real_stack_limit));
-  // Make ecx the space we have left. The stack might already be overflowed
-  // here which will cause ecx to become negative.
-  __ mov(ecx, esp);
-  __ sub(ecx, edi);
-  // Make edi the space we need for the array when it is unrolled onto the
-  // stack.
-  __ mov(edi, ebx);
-  __ shl(edi, kPointerSizeLog2);
-  // Check if the arguments will overflow the stack.
-  __ cmp(ecx, edi);
-  __ j(less_equal, stack_overflow);  // Signed comparison.
-}
-
 static void EnterArgumentsAdaptorFrame(MacroAssembler* masm) {
   __ push(ebp);
   __ mov(ebp, esp);
@@ -2922,7 +2977,9 @@ void Builtins::Generate_ArgumentsAdaptorTrampoline(MacroAssembler* masm) {
   {  // Enough parameters: Actual >= expected.
     __ bind(&enough);
     EnterArgumentsAdaptorFrame(masm);
-    ArgumentsAdaptorStackCheck(masm, &stack_overflow);
+    // edi is used as a scratch register. It should be restored from the frame
+    // when needed.
+    Generate_StackOverflowCheck(masm, ebx, ecx, edi, &stack_overflow);
 
     // Copy receiver and all expected arguments.
     const int offset = StandardFrameConstants::kCallerSPOffset;
@@ -2943,7 +3000,9 @@ void Builtins::Generate_ArgumentsAdaptorTrampoline(MacroAssembler* masm) {
   {  // Too few parameters: Actual < expected.
     __ bind(&too_few);
     EnterArgumentsAdaptorFrame(masm);
-    ArgumentsAdaptorStackCheck(masm, &stack_overflow);
+    // edi is used as a scratch register. It should be restored from the frame
+    // when needed.
+    Generate_StackOverflowCheck(masm, ebx, ecx, edi, &stack_overflow);
 
     // Remember expected arguments in ecx.
     __ mov(ecx, ebx);