From cc0c8d178f0cab1806fc07db7ee41215e9c6199a Mon Sep 17 00:00:00 2001 From: "kmillikin@chromium.org" Date: Fri, 24 Apr 2009 11:26:49 +0000 Subject: [PATCH] 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 | 10 +++ src/ia32/virtual-frame-ia32.cc | 24 +++++- src/virtual-frame.cc | 36 ++------- test/cctest/test-log-ia32.cc | 162 ----------------------------------------- 4 files changed, 38 insertions(+), 194 deletions(-) diff --git a/src/arm/virtual-frame-arm.cc b/src/arm/virtual-frame-arm.cc index b794d8b..43100f1 100644 --- a/src/arm/virtual-frame-arm.cc +++ b/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 diff --git a/src/ia32/virtual-frame-ia32.cc b/src/ia32/virtual-frame-ia32.cc index 3647ca5..656ef21 100644 --- a/src/ia32/virtual-frame-ia32.cc +++ b/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 undefined = Factory::undefined_value(); FrameElement initial_value = FrameElement::ConstantElement(undefined, FrameElement::SYNCED); diff --git a/src/virtual-frame.cc b/src/virtual-frame.cc index a03e31a..cef1d80 100644 --- a/src/virtual-frame.cc +++ b/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)) { diff --git a/test/cctest/test-log-ia32.cc b/test/cctest/test-log-ia32.cc index b171339..dde8512 100644 --- a/test/cctest/test-log-ia32.cc +++ b/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 func) { - v8::internal::Code* func_code = func->code(); - CheckRetAddrIsInFunction( - func_name, ret_addr, - reinterpret_cast(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 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 GetGlobalProperty(const char* name) { - return env->Global()->Get(String::New(name)); -} - - -static Handle GetGlobalJSFunction(const char* name) { - Handle 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) { - 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(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 trace_call_buf; - ::v8::internal::OS::SNPrintF(trace_call_buf, "%s(0x6666);", trace_func_name); - Handle 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(func)); -} - - -TEST(CFromJSStackTrace) { - TickSample sample; - StackTracer tracer(reinterpret_cast(&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(sample.stack[0])); - CheckRetAddrIsInJSFunction("JSTrace", - reinterpret_cast(sample.stack[1])); -} - - -TEST(PureJSStackTrace) { - TickSample sample; - StackTracer tracer(reinterpret_cast(&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(sample.stack[0])); - CheckRetAddrIsInJSFunction("OuterJSTrace", - reinterpret_cast(sample.stack[1])); -} - - static void CFuncDoTrace() { unsigned int fp; #ifdef __GNUC__ -- 2.7.4