Materializing a frame element on the stack by pushing it can cause the

stack pointer to change by more than one in a corner case.  If we push
a constant smi larger than 16 bits, we push it via a temporary
register.  Allocating the temporary can cause a register to be spilled
from the frame somewhere above the stack pointer.

As a fix, do not use pushes to materialize ranges of elements of size
larger than one.

Review URL: http://codereview.chromium.org/92121

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1785 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/arm/virtual-frame-arm.cc

@@ -71,6 +71,16 @@ void VirtualFrame::SyncElementByPushing(int index) {
 }
 
 
+void VirtualFrame::SyncRange(int begin, int end) {
+  // All elements are in memory on ARM (ie, synced).
+#ifdef DEBUG
+  for (int i = begin; i <= end; i++) {
+    ASSERT(elements_[i].is_synced());
+  }
+#endif
+}
+
+
 void VirtualFrame::MergeTo(VirtualFrame* expected) {
   Comment cmnt(masm_, "[ Merge frame");
   // We should always be merging the code generator's current frame to an

src/ia32/virtual-frame-ia32.cc

@@ -158,6 +158,28 @@ void VirtualFrame::SyncElementByPushing(int index) {
 }
 
 
+// Clear the dirty bits for the range of elements in
+// [min(stack_pointer_ + 1,begin), end].
+void VirtualFrame::SyncRange(int begin, int end) {
+  ASSERT(begin >= 0);
+  ASSERT(end < elements_.length());
+  // Sync elements below the range if they have not been materialized
+  // on the stack.
+  int start = Min(begin, stack_pointer_ + 1);
+
+  // If positive we have to adjust the stack pointer.
+  int delta = end - stack_pointer_;
+  if (delta > 0) {
+    stack_pointer_ = end;
+    __ sub(Operand(esp), Immediate(delta * kPointerSize));
+  }
+
+  for (int i = start; i <= end; i++) {
+    if (!elements_[i].is_synced()) SyncElementBelowStackPointer(i);
+  }
+}
+
+
 void VirtualFrame::MergeTo(VirtualFrame* expected) {
   Comment cmnt(masm_, "[ Merge frame");
   // We should always be merging the code generator's current frame to an
@@ -467,7 +489,7 @@ void VirtualFrame::AllocateStackSlots(int count) {
     // we sync them with the actual frame to allocate space for spilling
     // them later.  First sync everything above the stack pointer so we can
     // use pushes to allocate and initialize the locals.
-    SyncRange(stack_pointer_ + 1, elements_.length());
+    SyncRange(stack_pointer_ + 1, elements_.length() - 1);
     Handle<Object> undefined = Factory::undefined_value();
     FrameElement initial_value =
         FrameElement::ConstantElement(undefined, FrameElement::SYNCED);

src/virtual-frame.cc

@@ -213,40 +213,14 @@ void VirtualFrame::SpillElementAt(int index) {
 }
 
 
-// Clear the dirty bits for the range of elements in
-// [min(stack_pointer_ + 1,begin), end).
-void VirtualFrame::SyncRange(int begin, int end) {
-  ASSERT(begin >= 0);
-  ASSERT(end <= elements_.length());
-  if (begin > stack_pointer_) {
-    // Elements between stack_pointer_ + 1 and begin must also be synced.
-    for (int i = stack_pointer_ + 1; i < end; i++) {
-      SyncElementByPushing(i);
-    }
-  } else if (end <= stack_pointer_ + 1) {
-    for (int i = begin; i < end; i++) {
-      if (!elements_[i].is_synced()) {
-          SyncElementBelowStackPointer(i);
-      }
-    }
-  } else {
-    // Split into two ranges that each satisfy a condition above.
-    SyncRange(begin, stack_pointer_ + 1);
-    SyncRange(stack_pointer_ + 1, end);
-  }
-}
-
-
 // Clear the dirty bit for the element at a given index.
 void VirtualFrame::SyncElementAt(int index) {
   if (index <= stack_pointer_) {
-    if (!elements_[index].is_synced()) {
-      SyncElementBelowStackPointer(index);
-    }
+    if (!elements_[index].is_synced()) SyncElementBelowStackPointer(index);
+  } else if (index == stack_pointer_ + 1) {
+    SyncElementByPushing(index);
   } else {
-    for (int i = stack_pointer_ + 1; i <= index; i++) {
-      SyncElementByPushing(i);
-    }
+    SyncRange(stack_pointer_ + 1, index);
   }
 }
 
@@ -310,7 +284,7 @@ void VirtualFrame::PrepareForCall(int spilled_args, int dropped_args) {
   ASSERT(height() >= spilled_args);
   ASSERT(dropped_args <= spilled_args);
 
-  SyncRange(0, elements_.length());
+  SyncRange(0, elements_.length() - 1);
   // Spill registers.
   for (int i = 0; i < kNumRegisters; i++) {
     if (is_used(i)) {

test/cctest/test-log-ia32.cc

@@ -62,28 +62,6 @@ static void DoTraceHideCEntryFPAddress(unsigned int fp) {
 }
 
 
-static void CheckRetAddrIsInFunction(const char* func_name,
-                                     unsigned int ret_addr,
-                                     unsigned int func_start_addr,
-                                     unsigned int func_len) {
-  printf("CheckRetAddrIsInFunction \"%s\": %08x %08x %08x\n",
-         func_name, func_start_addr, ret_addr, func_start_addr + func_len);
-  CHECK_GE(ret_addr, func_start_addr);
-  CHECK_GE(func_start_addr + func_len, ret_addr);
-}
-
-
-static void CheckRetAddrIsInJSFunction(const char* func_name,
-                                       unsigned int ret_addr,
-                                       Handle<JSFunction> func) {
-  v8::internal::Code* func_code = func->code();
-  CheckRetAddrIsInFunction(
-      func_name, ret_addr,
-      reinterpret_cast<unsigned int>(func_code->instruction_start()),
-      func_code->ExecutableSize());
-}
-
-
 // --- T r a c e   E x t e n s i o n ---
 
 class TraceExtension : public v8::Extension {
@@ -141,146 +119,6 @@ static TraceExtension kTraceExtension;
 v8::DeclareExtension kTraceExtensionDeclaration(&kTraceExtension);
 
 
-static void InitializeVM() {
-  if (env.IsEmpty()) {
-    v8::HandleScope scope;
-    const char* extensions[] = { "v8/trace" };
-    v8::ExtensionConfiguration config(1, extensions);
-    env = v8::Context::New(&config);
-  }
-  v8::HandleScope scope;
-  env->Enter();
-}
-
-
-static Handle<JSFunction> CompileFunction(const char* source) {
-  return v8::Utils::OpenHandle(*Script::Compile(String::New(source)));
-}
-
-
-static void CompileRun(const char* source) {
-  Script::Compile(String::New(source))->Run();
-}
-
-
-static Local<Value> GetGlobalProperty(const char* name) {
-  return env->Global()->Get(String::New(name));
-}
-
-
-static Handle<JSFunction> GetGlobalJSFunction(const char* name) {
-  Handle<JSFunction> js_func(JSFunction::cast(
-                                 *(v8::Utils::OpenHandle(
-                                       *GetGlobalProperty(name)))));
-  return js_func;
-}
-
-
-static void CheckRetAddrIsInJSFunction(const char* func_name,
-                                       unsigned int ret_addr) {
-  CheckRetAddrIsInJSFunction(func_name, ret_addr,
-                             GetGlobalJSFunction(func_name));
-}
-
-
-static void SetGlobalProperty(const char* name, Local<Value> value) {
-  env->Global()->Set(String::New(name), value);
-}
-
-
-static bool Patch(byte* from,
-                  size_t num,
-                  byte* original,
-                  byte* patch,
-                  size_t patch_len) {
-  byte* to = from + num;
-  do {
-    from = static_cast<byte*>(memchr(from, *original, to - from));
-    CHECK(from != NULL);
-    if (memcmp(original, from, patch_len) == 0) {
-      memcpy(from, patch, patch_len);
-      return true;
-    } else {
-      from++;
-    }
-  } while (to - from > 0);
-  return false;
-}
-
-
-// Creates a global function named 'func_name' that calls the tracing
-// function 'trace_func_name' with an actual EBP register value,
-// shifted right to be presented as Smi.
-static void CreateTraceCallerFunction(const char* func_name,
-                                      const char* trace_func_name) {
-  ::v8::internal::EmbeddedVector<char, 256> trace_call_buf;
-  ::v8::internal::OS::SNPrintF(trace_call_buf, "%s(0x6666);", trace_func_name);
-  Handle<JSFunction> func = CompileFunction(trace_call_buf.start());
-  CHECK(!func.is_null());
-  v8::internal::Code* func_code = func->code();
-  CHECK(func_code->IsCode());
-
-  // push 0xcccc (= 0x6666 << 1)
-  byte original[] = { 0x68, 0xcc, 0xcc, 0x00, 0x00 };
-  // mov eax,ebp; shr eax; push eax;
-  byte patch[] = { 0x89, 0xe8, 0xd1, 0xe8, 0x50 };
-  // Patch generated code to replace pushing of a constant with
-  // pushing of ebp contents in a Smi
-  CHECK(Patch(func_code->instruction_start(),
-              func_code->instruction_size(),
-              original, patch, sizeof(patch)));
-
-  SetGlobalProperty(func_name, v8::ToApi<Value>(func));
-}
-
-
-TEST(CFromJSStackTrace) {
-  TickSample sample;
-  StackTracer tracer(reinterpret_cast<unsigned int>(&sample));
-  InitTraceEnv(&tracer, &sample);
-
-  InitializeVM();
-  v8::HandleScope scope;
-  CreateTraceCallerFunction("JSFuncDoTrace", "trace");
-  CompileRun(
-      "function JSTrace() {"
-      "  JSFuncDoTrace();"
-      "};\n"
-      "JSTrace();");
-  CHECK_GT(sample.frames_count, 1);
-  // Stack sampling will start from the first JS function, i.e. "JSFuncDoTrace"
-  CheckRetAddrIsInJSFunction("JSFuncDoTrace",
-                             reinterpret_cast<unsigned int>(sample.stack[0]));
-  CheckRetAddrIsInJSFunction("JSTrace",
-                             reinterpret_cast<unsigned int>(sample.stack[1]));
-}
-
-
-TEST(PureJSStackTrace) {
-  TickSample sample;
-  StackTracer tracer(reinterpret_cast<unsigned int>(&sample));
-  InitTraceEnv(&tracer, &sample);
-
-  InitializeVM();
-  v8::HandleScope scope;
-  CreateTraceCallerFunction("JSFuncDoTrace", "js_trace");
-  CompileRun(
-      "function JSTrace() {"
-      "  JSFuncDoTrace();"
-      "};\n"
-      "function OuterJSTrace() {"
-      "  JSTrace();"
-      "};\n"
-      "OuterJSTrace();");
-  CHECK_GT(sample.frames_count, 1);
-  // Stack sampling will start from the caller of JSFuncDoTrace, i.e. "JSTrace"
-  CheckRetAddrIsInJSFunction("JSTrace",
-                             reinterpret_cast<unsigned int>(sample.stack[0]));
-  CheckRetAddrIsInJSFunction("OuterJSTrace",
-                             reinterpret_cast<unsigned int>(sample.stack[1]));
-}
-
-
 static void CFuncDoTrace() {
   unsigned int fp;
 #ifdef __GNUC__