Reviewers: loislo, Sven Panne,
Description:
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
Please review this at https://codereview.chromium.org/17589022/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files:
M src/frames.h
M src/frames.cc
M test/cctest/test-cpu-profiler.cc
Index: src/frames.cc
diff --git a/src/frames.cc b/src/frames.cc
index
80fe469d2c51d3921cb5e108c2d596afb37880ec..a547890db0681c5b575f230ac5121632074705d6
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());
}
Index: src/frames.h
diff --git a/src/frames.h b/src/frames.h
index
4d4b7ea83ad789dde3c5db9e2ae10f4641e9cfd9..91a8b12e28dfbabede86e6d97d09ab0291eca259
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,
Index: test/cctest/test-cpu-profiler.cc
diff --git a/test/cctest/test-cpu-profiler.cc
b/test/cctest/test-cpu-profiler.cc
index
2184122717770be2eb0203fd9933394828e7f0e5..7c2dcf3b9d3b332c3e4b92357234973cbeec4185
100644
--- a/test/cctest/test-cpu-profiler.cc
+++ b/test/cctest/test-cpu-profiler.cc
@@ -914,3 +914,52 @@ 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<v8::Function> function = v8::Local<v8::Function>::Cast(
+ env->Global()->Get(v8::String::New("start")));
+
+ v8::CpuProfiler* cpu_profiler = env->GetIsolate()->GetCpuProfiler();
+ v8::Local<v8::String> profile_name = v8::String::New("my_profile");
+
+ cpu_profiler->StartCpuProfiling(profile_name);
+ int32_t duration_ms = 100;
+ v8::Handle<v8::Value> 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<i::CpuProfile*>(
+ const_cast<v8::CpuProfile*>(profile))->Print();
+
+ const v8::CpuProfileNode* root = profile->GetTopDownRoot();
+ GetChild(root, "start");
+ ScopedVector<v8::Handle<v8::String> > 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);
+
+ cpu_profiler->DeleteAllCpuProfiles();
+}
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
