Extract StackFrameIteratorBase

This change introduces StackFrameIteratorBase which owns singleton frame instances and encapsulates some basic iterator functionality. It has two actual implementations: StackFrameIterator and SafeStackFrameIterator.

All logic specific to frame iteration at a random point (basically checks that fp and sp extracted from stack frames are within current stack boundaries) used only by CPU profiler is now concentrated in SafeStackFrameIterator.

Generic stack iteration used in all other places is put into StackFrameIterator. Also this iterator unlike SafeStackFrameIterator iterates through stack handlers.

StackAddressValidator and ExitFrameValidator classes were removed in favor of inline checks and simple methods.

BUG=None
R=loislo@chromium.org, svenpanne@chromium.org

Review URL: https://codereview.chromium.org/17819003

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

src/frames-inl.h

@@ -116,7 +116,7 @@ inline Object** StackHandler::code_address() const {
 }
 
 
-inline StackFrame::StackFrame(StackFrameIterator* iterator)
+inline StackFrame::StackFrame(StackFrameIteratorBase* iterator)
     : iterator_(iterator), isolate_(iterator_->isolate()) {
 }
 
@@ -136,22 +136,23 @@ inline Code* StackFrame::GetContainingCode(Isolate* isolate, Address pc) {
 }
 
 
-inline EntryFrame::EntryFrame(StackFrameIterator* iterator)
+inline EntryFrame::EntryFrame(StackFrameIteratorBase* iterator)
     : StackFrame(iterator) {
 }
 
 
-inline EntryConstructFrame::EntryConstructFrame(StackFrameIterator* iterator)
+inline EntryConstructFrame::EntryConstructFrame(
+    StackFrameIteratorBase* iterator)
     : EntryFrame(iterator) {
 }
 
 
-inline ExitFrame::ExitFrame(StackFrameIterator* iterator)
+inline ExitFrame::ExitFrame(StackFrameIteratorBase* iterator)
     : StackFrame(iterator) {
 }
 
 
-inline StandardFrame::StandardFrame(StackFrameIterator* iterator)
+inline StandardFrame::StandardFrame(StackFrameIteratorBase* iterator)
     : StackFrame(iterator) {
 }
 
@@ -201,7 +202,7 @@ inline bool StandardFrame::IsConstructFrame(Address fp) {
 }
 
 
-inline JavaScriptFrame::JavaScriptFrame(StackFrameIterator* iterator)
+inline JavaScriptFrame::JavaScriptFrame(StackFrameIteratorBase* iterator)
     : StandardFrame(iterator) {
 }
 
@@ -269,32 +270,32 @@ inline Object* JavaScriptFrame::function() const {
 }
 
 
-inline StubFrame::StubFrame(StackFrameIterator* iterator)
+inline StubFrame::StubFrame(StackFrameIteratorBase* iterator)
     : StandardFrame(iterator) {
 }
 
 
-inline OptimizedFrame::OptimizedFrame(StackFrameIterator* iterator)
+inline OptimizedFrame::OptimizedFrame(StackFrameIteratorBase* iterator)
     : JavaScriptFrame(iterator) {
 }
 
 
 inline ArgumentsAdaptorFrame::ArgumentsAdaptorFrame(
-    StackFrameIterator* iterator) : JavaScriptFrame(iterator) {
+    StackFrameIteratorBase* iterator) : JavaScriptFrame(iterator) {
 }
 
 
-inline InternalFrame::InternalFrame(StackFrameIterator* iterator)
+inline InternalFrame::InternalFrame(StackFrameIteratorBase* iterator)
     : StandardFrame(iterator) {
 }
 
 
 inline StubFailureTrampolineFrame::StubFailureTrampolineFrame(
-    StackFrameIterator* iterator) : StandardFrame(iterator) {
+    StackFrameIteratorBase* iterator) : StandardFrame(iterator) {
 }
 
 
-inline ConstructFrame::ConstructFrame(StackFrameIterator* iterator)
+inline ConstructFrame::ConstructFrame(StackFrameIteratorBase* iterator)
     : InternalFrame(iterator) {
 }
 
@@ -325,14 +326,9 @@ inline JavaScriptFrame* JavaScriptFrameIterator::frame() const {
 
 
 inline JavaScriptFrame* SafeStackFrameIterator::frame() const {
-  ASSERT(!iteration_done_);
-  // TODO(1233797): The frame hierarchy needs to change. It's
-  // problematic that we can't use the safe-cast operator to cast to
-  // the JavaScript frame type, because we may encounter arguments
-  // adaptor frames.
-  StackFrame* frame = iterator_.frame();
-  ASSERT(frame->is_java_script());
-  return static_cast<JavaScriptFrame*>(frame);
+  ASSERT(!done());
+  ASSERT(frame_->is_java_script());
+  return static_cast<JavaScriptFrame*>(frame_);
 }
 
 

src/frames.cc

@@ -88,41 +88,29 @@ class StackHandlerIterator BASE_EMBEDDED {
 
 
 #define INITIALIZE_SINGLETON(type, field) field##_(this),
-StackFrameIterator::StackFrameIterator(Isolate* isolate)
+StackFrameIteratorBase::StackFrameIteratorBase(Isolate* isolate,
+                                               bool can_access_heap_objects)
     : isolate_(isolate),
       STACK_FRAME_TYPE_LIST(INITIALIZE_SINGLETON)
       frame_(NULL), handler_(NULL),
-      thread_(isolate_->thread_local_top()),
-      fp_(NULL), sp_(NULL), advance_(&StackFrameIterator::AdvanceWithHandler),
-      can_access_heap_objects_(true) {
-  Reset();
-}
-StackFrameIterator::StackFrameIterator(Isolate* isolate, ThreadLocalTop* t)
-    : isolate_(isolate),
-      STACK_FRAME_TYPE_LIST(INITIALIZE_SINGLETON)
-      frame_(NULL), handler_(NULL), thread_(t),
-      fp_(NULL), sp_(NULL), advance_(&StackFrameIterator::AdvanceWithHandler),
-      can_access_heap_objects_(true) {
-  Reset();
+      can_access_heap_objects_(can_access_heap_objects) {
 }
-StackFrameIterator::StackFrameIterator(Isolate* isolate,
-                                       bool use_top, Address fp, Address sp)
-    : isolate_(isolate),
-      STACK_FRAME_TYPE_LIST(INITIALIZE_SINGLETON)
-      frame_(NULL), handler_(NULL),
-      thread_(use_top ? isolate_->thread_local_top() : NULL),
-      fp_(use_top ? NULL : fp), sp_(sp),
-      advance_(&StackFrameIterator::AdvanceWithoutHandler),
-      can_access_heap_objects_(false) {
-  if (use_top || fp != NULL) {
-    Reset();
-  }
+#undef INITIALIZE_SINGLETON
+
+
+StackFrameIterator::StackFrameIterator(Isolate* isolate)
+    : StackFrameIteratorBase(isolate, true) {
+  Reset(isolate->thread_local_top());
 }
 
-#undef INITIALIZE_SINGLETON
+
+StackFrameIterator::StackFrameIterator(Isolate* isolate, ThreadLocalTop* t)
+    : StackFrameIteratorBase(isolate, true) {
+  Reset(t);
+}
 
 
-void StackFrameIterator::AdvanceWithHandler() {
+void StackFrameIterator::Advance() {
   ASSERT(!done());
   // Compute the state of the calling frame before restoring
   // callee-saved registers and unwinding handlers. This allows the
@@ -145,37 +133,17 @@ void StackFrameIterator::AdvanceWithHandler() {
 }
 
 
-void StackFrameIterator::AdvanceWithoutHandler() {
-  // A simpler version of Advance which doesn't care about handler.
-  ASSERT(!done());
+void StackFrameIterator::Reset(ThreadLocalTop* top) {
   StackFrame::State state;
-  StackFrame::Type type = frame_->GetCallerState(&state);
-  frame_ = SingletonFor(type, &state);
-}
-
-
-void StackFrameIterator::Reset() {
-  StackFrame::State state;
-  StackFrame::Type type;
-  if (thread_ != NULL) {
-    type = ExitFrame::GetStateForFramePointer(
-        Isolate::c_entry_fp(thread_), &state);
-    handler_ = StackHandler::FromAddress(
-        Isolate::handler(thread_));
-  } else {
-    ASSERT(fp_ != NULL);
-    state.fp = fp_;
-    state.sp = sp_;
-    state.pc_address = ResolveReturnAddressLocation(
-        reinterpret_cast<Address*>(StandardFrame::ComputePCAddress(fp_)));
-    type = StackFrame::ComputeType(this, &state);
-  }
+  StackFrame::Type type = ExitFrame::GetStateForFramePointer(
+      Isolate::c_entry_fp(top), &state);
+  handler_ = StackHandler::FromAddress(Isolate::handler(top));
   if (SingletonFor(type) == NULL) return;
   frame_ = SingletonFor(type, &state);
 }
 
 
-StackFrame* StackFrameIterator::SingletonFor(StackFrame::Type type,
+StackFrame* StackFrameIteratorBase::SingletonFor(StackFrame::Type type,
                                              StackFrame::State* state) {
   if (type == StackFrame::NONE) return NULL;
   StackFrame* result = SingletonFor(type);
@@ -185,7 +153,7 @@ StackFrame* StackFrameIterator::SingletonFor(StackFrame::Type type,
 }
 
 
-StackFrame* StackFrameIterator::SingletonFor(StackFrame::Type type) {
+StackFrame* StackFrameIteratorBase::SingletonFor(StackFrame::Type type) {
 #define FRAME_TYPE_CASE(type, field) \
   case StackFrame::type: result = &field##_; break;
 
@@ -257,55 +225,61 @@ bool StackTraceFrameIterator::IsValidFrame() {
 // -------------------------------------------------------------------------
 
 
-bool SafeStackFrameIterator::ExitFrameValidator::IsValidFP(Address fp) {
-  if (!validator_.IsValid(fp)) return false;
-  Address sp = ExitFrame::ComputeStackPointer(fp);
-  if (!validator_.IsValid(sp)) return false;
-  StackFrame::State state;
-  ExitFrame::FillState(fp, sp, &state);
-  if (!validator_.IsValid(reinterpret_cast<Address>(state.pc_address))) {
-    return false;
-  }
-  return *state.pc_address != NULL;
-}
-
-
 SafeStackFrameIterator::SafeStackFrameIterator(
     Isolate* isolate,
     Address fp, Address sp, Address low_bound, Address high_bound) :
-    stack_validator_(low_bound, high_bound),
-    is_valid_top_(IsValidTop(isolate, low_bound, high_bound)),
-    is_valid_fp_(IsWithinBounds(low_bound, high_bound, fp)),
-    iteration_done_(!is_valid_top_ && !is_valid_fp_),
-    iterator_(isolate, is_valid_top_, is_valid_fp_ ? fp : NULL, sp) {
+    StackFrameIteratorBase(isolate, false),
+    low_bound_(low_bound), high_bound_(high_bound) {
+  StackFrame::State state;
+  StackFrame::Type type;
+  ThreadLocalTop* top = isolate->thread_local_top();
+  if (IsValidTop(top)) {
+    type = ExitFrame::GetStateForFramePointer(Isolate::c_entry_fp(top), &state);
+  } else if (IsValidStackAddress(fp)) {
+    ASSERT(fp != NULL);
+    state.fp = fp;
+    state.sp = sp;
+    state.pc_address = ResolveReturnAddressLocation(
+        reinterpret_cast<Address*>(StandardFrame::ComputePCAddress(fp)));
+    type = StackFrame::ComputeType(this, &state);
+  } else {
+    return;
+  }
+  if (SingletonFor(type) == NULL) return;
+  frame_ = SingletonFor(type, &state);
+
   if (!done()) Advance();
 }
 
 
-bool SafeStackFrameIterator::IsValidTop(Isolate* isolate,
-                                        Address low_bound, Address high_bound) {
-  ThreadLocalTop* top = isolate->thread_local_top();
+bool SafeStackFrameIterator::IsValidTop(ThreadLocalTop* top) const {
   Address fp = Isolate::c_entry_fp(top);
-  ExitFrameValidator validator(low_bound, high_bound);
-  if (!validator.IsValidFP(fp)) return false;
+  if (!IsValidExitFrame(fp)) return false;
+  // There should be at least one JS_ENTRY stack handler.
   return Isolate::handler(top) != NULL;
 }
 
 
 void SafeStackFrameIterator::AdvanceOneFrame() {
   ASSERT(!done());
-  StackFrame* last_frame = iterator_.frame();
+  StackFrame* last_frame = frame_;
   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) ||
-      !IsValidCaller(last_frame);
-  if (iteration_done_) return;
+  // Before advancing to the next stack frame, perform pointer validity tests.
+  if (!IsValidFrame(last_frame) || !IsValidCaller(last_frame)) {
+    frame_ = NULL;
+    return;
+  }
 
-  iterator_.Advance();
-  if (iterator_.done()) return;
-  // Check that we have actually moved to the previous frame in the stack
-  StackFrame* prev_frame = iterator_.frame();
-  iteration_done_ = prev_frame->sp() < last_sp || prev_frame->fp() < last_fp;
+  // Advance to the previous frame.
+  StackFrame::State state;
+  StackFrame::Type type = frame_->GetCallerState(&state);
+  frame_ = SingletonFor(type, &state);
+  if (frame_ == NULL) return;
+
+  // Check that we have actually moved to the previous frame in the stack.
+  if (frame_->sp() < last_sp || frame_->fp() < last_fp) {
+    frame_ = NULL;
+  }
 }
 
 
@@ -322,8 +296,7 @@ bool SafeStackFrameIterator::IsValidCaller(StackFrame* frame) {
     // sure that caller FP address is valid.
     Address caller_fp = Memory::Address_at(
         frame->fp() + EntryFrameConstants::kCallerFPOffset);
-    ExitFrameValidator validator(stack_validator_);
-    if (!validator.IsValidFP(caller_fp)) return false;
+    if (!IsValidExitFrame(caller_fp)) return false;
   } else if (frame->is_arguments_adaptor()) {
     // See ArgumentsAdaptorFrame::GetCallerStackPointer. It assumes that
     // the number of arguments is stored on stack as Smi. We need to check
@@ -336,7 +309,20 @@ bool SafeStackFrameIterator::IsValidCaller(StackFrame* frame) {
   }
   frame->ComputeCallerState(&state);
   return IsValidStackAddress(state.sp) && IsValidStackAddress(state.fp) &&
-      iterator_.SingletonFor(frame->GetCallerState(&state)) != NULL;
+      SingletonFor(frame->GetCallerState(&state)) != NULL;
+}
+
+
+bool SafeStackFrameIterator::IsValidExitFrame(Address fp) const {
+  if (!IsValidStackAddress(fp)) return false;
+  Address sp = ExitFrame::ComputeStackPointer(fp);
+  if (!IsValidStackAddress(sp)) return false;
+  StackFrame::State state;
+  ExitFrame::FillState(fp, sp, &state);
+  if (!IsValidStackAddress(reinterpret_cast<Address>(state.pc_address))) {
+    return false;
+  }
+  return *state.pc_address != NULL;
 }
 
 
@@ -344,7 +330,7 @@ void SafeStackFrameIterator::Advance() {
   while (true) {
     AdvanceOneFrame();
     if (done()) return;
-    if (iterator_.frame()->is_java_script()) return;
+    if (frame_->is_java_script()) return;
   }
 }
 
@@ -408,7 +394,7 @@ void StackFrame::SetReturnAddressLocationResolver(
 }
 
 
-StackFrame::Type StackFrame::ComputeType(const StackFrameIterator* iterator,
+StackFrame::Type StackFrame::ComputeType(const StackFrameIteratorBase* iterator,
                                          State* state) {
   ASSERT(state->fp != NULL);
   if (StandardFrame::IsArgumentsAdaptorFrame(state->fp)) {