From 4aeccdb23e418f5523e6bf9c71d80bed9be91d76 Mon Sep 17 00:00:00 2001 From: "yurys@chromium.org" Date: Thu, 27 Jun 2013 09:28:11 +0000 Subject: [PATCH] Do not iterate stack handlers in SafeStackFrameIterator CPU profiler doesn't use stack handlers so there is no need to iterate through them while traversing stack. This change SafeStackFrameIterator always iterate only frames and removes checks corresponding to the handlers iteration. The problem described in the bug occurred because of a false assumption in SafeStackFrameIterator that if Isolate::c_entry_fp is not NULL then the top frame on the stack is always a C++ frame. It is false because we may have entered JS code again, in which case JS_ENTRY code stub generated by JSEntryStub::GenerateBody() will save current c_entry_fp value but not reset it to NULL and after that it will create ENTRY stack frame and JS_ENTRY handler on the stack and put the latter into Isolate::handler(top). This means that if we start iterating from c_entry_fp frame and try to compare the frame's sp with Isolate::handler()->address() it will turn out that frame->sp() > handler->address() and the condition in SafeStackFrameIterator::CanIterateHandles is not held. BUG=252097 R=loislo@chromium.org, svenpanne@chromium.org Review URL: https://codereview.chromium.org/17589022 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@15348 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/frames.cc | 13 +--------- src/frames.h | 1 - test/cctest/test-cpu-profiler.cc | 51 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 13 deletions(-) diff --git a/src/frames.cc b/src/frames.cc index 49a08ad..acdcbdf 100644 --- a/src/frames.cc +++ b/src/frames.cc @@ -112,8 +112,7 @@ StackFrameIterator::StackFrameIterator(Isolate* isolate, frame_(NULL), handler_(NULL), thread_(use_top ? isolate_->thread_local_top() : NULL), fp_(use_top ? NULL : fp), sp_(sp), - advance_(use_top ? &StackFrameIterator::AdvanceWithHandler : - &StackFrameIterator::AdvanceWithoutHandler), + advance_(&StackFrameIterator::AdvanceWithoutHandler), can_access_heap_objects_(false) { if (use_top || fp != NULL) { Reset(); @@ -299,7 +298,6 @@ void SafeStackFrameIterator::AdvanceOneFrame() { Address last_sp = last_frame->sp(), last_fp = last_frame->fp(); // Before advancing to the next stack frame, perform pointer validity tests iteration_done_ = !IsValidFrame(last_frame) || - !CanIterateHandles(last_frame, iterator_.handler()) || !IsValidCaller(last_frame); if (iteration_done_) return; @@ -311,15 +309,6 @@ void SafeStackFrameIterator::AdvanceOneFrame() { } -bool SafeStackFrameIterator::CanIterateHandles(StackFrame* frame, - StackHandler* handler) { - // If StackIterator iterates over StackHandles, verify that - // StackHandlerIterator can be instantiated (see StackHandlerIterator - // constructor.) - return !is_valid_top_ || (frame->sp() <= handler->address()); -} - - bool SafeStackFrameIterator::IsValidFrame(StackFrame* frame) const { return IsValidStackAddress(frame->sp()) && IsValidStackAddress(frame->fp()); } diff --git a/src/frames.h b/src/frames.h index 4d4b7ea..91a8b12 100644 --- a/src/frames.h +++ b/src/frames.h @@ -919,7 +919,6 @@ class SafeStackFrameIterator BASE_EMBEDDED { bool IsValidStackAddress(Address addr) const { return stack_validator_.IsValid(addr); } - bool CanIterateHandles(StackFrame* frame, StackHandler* handler); bool IsValidFrame(StackFrame* frame) const; bool IsValidCaller(StackFrame* frame); static bool IsValidTop(Isolate* isolate, diff --git a/test/cctest/test-cpu-profiler.cc b/test/cctest/test-cpu-profiler.cc index 2184122..9a2496a 100644 --- a/test/cctest/test-cpu-profiler.cc +++ b/test/cctest/test-cpu-profiler.cc @@ -914,3 +914,54 @@ TEST(NativeMethodMonomorphicIC) { cpu_profiler->DeleteAllCpuProfiles(); } + + +static const char* bound_function_test_source = "function foo(iterations) {\n" +" var r = 0;\n" +" for (var i = 0; i < iterations; i++) { r += i; }\n" +" return r;\n" +"}\n" +"function start(duration) {\n" +" var callback = foo.bind(this);\n" +" var start = Date.now();\n" +" while (Date.now() - start < duration) {\n" +" callback(10 * 1000);\n" +" }\n" +"}"; + + +TEST(BoundFunctionCall) { + LocalContext env; + v8::HandleScope scope(env->GetIsolate()); + + v8::Script::Compile(v8::String::New(bound_function_test_source))->Run(); + v8::Local function = v8::Local::Cast( + env->Global()->Get(v8::String::New("start"))); + + v8::CpuProfiler* cpu_profiler = env->GetIsolate()->GetCpuProfiler(); + v8::Local profile_name = v8::String::New("my_profile"); + + cpu_profiler->StartCpuProfiling(profile_name); + int32_t duration_ms = 100; + v8::Handle args[] = { v8::Integer::New(duration_ms) }; + function->Call(env->Global(), ARRAY_SIZE(args), args); + const v8::CpuProfile* profile = cpu_profiler->StopCpuProfiling(profile_name); + + CHECK_NE(NULL, profile); + // Dump collected profile to have a better diagnostic in case of failure. + reinterpret_cast( + const_cast(profile))->Print(); + + const v8::CpuProfileNode* root = profile->GetTopDownRoot(); + ScopedVector > names(3); + names[0] = v8::String::New(ProfileGenerator::kGarbageCollectorEntryName); + names[1] = v8::String::New(ProfileGenerator::kProgramEntryName); + names[2] = v8::String::New("start"); + // Don't allow |foo| node to be at the top level. + CheckChildrenNames(root, names); + + const v8::CpuProfileNode* startNode = GetChild(root, "start"); + GetChild(startNode, "foo"); + + cpu_profiler->DeleteAllCpuProfiles(); +} -- 2.7.4