Enhance SafeStackFrameIterator to avoid triggering assertions in debug mode.

When running profiling in debug mode, several assertions in frame
iterators that are undoubtedly useful when iterator is started from a
VM thread in a known "good" state, may fail when running over a stack
of a suspended VM thread. This patch makes SafeStackFrameIterator
to proactively check addresses and bail out from iteration early,
before an assertion will be triggered.

BUG=crbug/55565

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

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

diff --git a/src/arm/frames-arm.cc b/src/arm/frames-arm.cc
index 4743439..b0c0990 100644 (file)
--- a/src/arm/frames-arm.cc
+++ b/src/arm/frames-arm.cc
@@ -37,17 +37,8 @@ namespace v8 {
 namespace internal {
 
 
-StackFrame::Type ExitFrame::GetStateForFramePointer(Address fp, State* state) {
-  if (fp == 0) return NONE;
-  // Compute frame type and stack pointer.
-  Address sp = fp + ExitFrameConstants::kSPOffset;
-
-  // Fill in the state.
-  state->sp = sp;
-  state->fp = fp;
-  state->pc_address = reinterpret_cast<Address*>(sp - 1 * kPointerSize);
-  ASSERT(*state->pc_address != NULL);
-  return EXIT;
+Address ExitFrame::ComputeStackPointer(Address fp) {
+  return fp + ExitFrameConstants::kSPOffset;
 }
 
 

diff --git a/src/frames.cc b/src/frames.cc
index 76a441b..3cdb015 100644 (file)
--- a/src/frames.cc
+++ b/src/frames.cc
@@ -143,8 +143,8 @@ void StackFrameIterator::Reset() {
     state.pc_address =
         reinterpret_cast<Address*>(StandardFrame::ComputePCAddress(fp_));
     type = StackFrame::ComputeType(&state);
-    if (SingletonFor(type) == NULL) return;
   }
+  if (SingletonFor(type) == NULL) return;
   frame_ = SingletonFor(type, &state);
 }
 
@@ -203,13 +203,24 @@ 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(
     Address fp, Address sp, Address low_bound, Address high_bound) :
-    maintainer_(), low_bound_(low_bound), high_bound_(high_bound),
-    is_valid_top_(
-        IsWithinBounds(low_bound, high_bound,
-                       Top::c_entry_fp(Top::GetCurrentThread())) &&
-        Top::handler(Top::GetCurrentThread()) != NULL),
+    maintainer_(),
+    stack_validator_(low_bound, high_bound),
+    is_valid_top_(IsValidTop(low_bound, high_bound)),
     is_valid_fp_(IsWithinBounds(low_bound, high_bound, fp)),
     is_working_iterator_(is_valid_top_ || is_valid_fp_),
     iteration_done_(!is_working_iterator_),
@@ -217,6 +228,14 @@ SafeStackFrameIterator::SafeStackFrameIterator(
 }
 
 
+bool SafeStackFrameIterator::IsValidTop(Address low_bound, Address high_bound) {
+  Address fp = Top::c_entry_fp(Top::GetCurrentThread());
+  ExitFrameValidator validator(low_bound, high_bound);
+  if (!validator.IsValidFP(fp)) return false;
+  return Top::handler(Top::GetCurrentThread()) != NULL;
+}
+
+
 void SafeStackFrameIterator::Advance() {
   ASSERT(is_working_iterator_);
   ASSERT(!done());
@@ -258,9 +277,8 @@ bool SafeStackFrameIterator::IsValidCaller(StackFrame* frame) {
     // sure that caller FP address is valid.
     Address caller_fp = Memory::Address_at(
         frame->fp() + EntryFrameConstants::kCallerFPOffset);
-    if (!IsValidStackAddress(caller_fp)) {
-      return false;
-    }
+    ExitFrameValidator validator(stack_validator_);
+    if (!validator.IsValidFP(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
@@ -415,6 +433,22 @@ Address ExitFrame::GetCallerStackPointer() const {
 }
 
 
+StackFrame::Type ExitFrame::GetStateForFramePointer(Address fp, State* state) {
+  if (fp == 0) return NONE;
+  Address sp = ComputeStackPointer(fp);
+  FillState(fp, sp, state);
+  ASSERT(*state->pc_address != NULL);
+  return EXIT;
+}
+
+
+void ExitFrame::FillState(Address fp, Address sp, State* state) {
+  state->sp = sp;
+  state->fp = fp;
+  state->pc_address = reinterpret_cast<Address*>(sp - 1 * kPointerSize);
+}
+
+
 Address StandardFrame::GetExpressionAddress(int n) const {
   const int offset = StandardFrameConstants::kExpressionsOffset;
   return fp() + offset - n * kPointerSize;

diff --git a/src/frames.h b/src/frames.h
index 2011190..9b0a078 100644 (file)
--- a/src/frames.h
+++ b/src/frames.h
@@ -202,6 +202,7 @@ class StackFrame BASE_EMBEDDED {
 
  protected:
   struct State {
+    State() : sp(NULL), fp(NULL), pc_address(NULL) { }
     Address sp;
     Address fp;
     Address* pc_address;
@@ -318,6 +319,8 @@ class ExitFrame: public StackFrame {
   // pointer. Used when constructing the first stack frame seen by an
   // iterator and the frames following entry frames.
   static Type GetStateForFramePointer(Address fp, State* state);
+  static Address ComputeStackPointer(Address fp);
+  static void FillState(Address fp, Address sp, State* state);
 
  protected:
   explicit ExitFrame(StackFrameIterator* iterator) : StackFrame(iterator) { }
@@ -443,6 +446,7 @@ class JavaScriptFrame: public StandardFrame {
   inline Object* function_slot_object() const;
 
   friend class StackFrameIterator;
+  friend class StackTracer;
 };
 
 
@@ -654,12 +658,36 @@ class SafeStackFrameIterator BASE_EMBEDDED {
   }
 
  private:
+  class StackAddressValidator {
+   public:
+    StackAddressValidator(Address low_bound, Address high_bound)
+        : low_bound_(low_bound), high_bound_(high_bound) { }
+    bool IsValid(Address addr) const {
+      return IsWithinBounds(low_bound_, high_bound_, addr);
+    }
+   private:
+    Address low_bound_;
+    Address high_bound_;
+  };
+
+  class ExitFrameValidator {
+   public:
+    explicit ExitFrameValidator(const StackAddressValidator& validator)
+        : validator_(validator) { }
+    ExitFrameValidator(Address low_bound, Address high_bound)
+        : validator_(low_bound, high_bound) { }
+    bool IsValidFP(Address fp);
+   private:
+    StackAddressValidator validator_;
+  };
+
   bool IsValidStackAddress(Address addr) const {
-    return IsWithinBounds(low_bound_, high_bound_, addr);
+    return stack_validator_.IsValid(addr);
   }
   bool CanIterateHandles(StackFrame* frame, StackHandler* handler);
   bool IsValidFrame(StackFrame* frame) const;
   bool IsValidCaller(StackFrame* frame);
+  static bool IsValidTop(Address low_bound, Address high_bound);
 
   // This is a nasty hack to make sure the active count is incremented
   // before the constructor for the embedded iterator is invoked. This
@@ -674,8 +702,7 @@ class SafeStackFrameIterator BASE_EMBEDDED {
 
   ActiveCountMaintainer maintainer_;
   static int active_count_;
-  Address low_bound_;
-  Address high_bound_;
+  StackAddressValidator stack_validator_;
   const bool is_valid_top_;
   const bool is_valid_fp_;
   const bool is_working_iterator_;

diff --git a/src/ia32/frames-ia32.cc b/src/ia32/frames-ia32.cc
index 9baf763..dd44f0e 100644 (file)
--- a/src/ia32/frames-ia32.cc
+++ b/src/ia32/frames-ia32.cc
@@ -35,16 +35,8 @@ namespace v8 {
 namespace internal {
 
 
-StackFrame::Type ExitFrame::GetStateForFramePointer(Address fp, State* state) {
-  if (fp == 0) return NONE;
-  // Compute the stack pointer.
-  Address sp = Memory::Address_at(fp + ExitFrameConstants::kSPOffset);
-  // Fill in the state.
-  state->fp = fp;
-  state->sp = sp;
-  state->pc_address = reinterpret_cast<Address*>(sp - 1 * kPointerSize);
-  ASSERT(*state->pc_address != NULL);
-  return EXIT;
+Address ExitFrame::ComputeStackPointer(Address fp) {
+  return Memory::Address_at(fp + ExitFrameConstants::kSPOffset);
 }
 
 

diff --git a/src/log.cc b/src/log.cc
index 0bca5eb..a9d89a2 100644 (file)
--- a/src/log.cc
+++ b/src/log.cc
@@ -171,7 +171,9 @@ void StackTracer::Trace(TickSample* sample) {
   SafeStackTraceFrameIterator it(sample->fp, sample->sp,
                                  sample->sp, js_entry_sp);
   while (!it.done() && i < TickSample::kMaxFramesCount) {
-    sample->stack[i++] = reinterpret_cast<Address>(it.frame()->function());
+    sample->stack[i++] =
+        reinterpret_cast<Address>(it.frame()->function_slot_object()) -
+            kHeapObjectTag;
     it.Advance();
   }
   sample->frames_count = i;

diff --git a/src/mips/frames-mips.cc b/src/mips/frames-mips.cc
index 0fce3cd..d630562 100644 (file)
--- a/src/mips/frames-mips.cc
+++ b/src/mips/frames-mips.cc
@@ -52,9 +52,7 @@ StackFrame::Type StackFrame::ComputeType(State* state) {
 }
 
 
-StackFrame::Type ExitFrame::GetStateForFramePointer(Address fp, State* state) {
-  if (fp == 0) return NONE;
-  // Compute frame type and stack pointer.
+Address ExitFrame::ComputeStackPointer(Address fp) {
   Address sp = fp + ExitFrameConstants::kSPDisplacement;
   const int offset = ExitFrameConstants::kCodeOffset;
   Object* code = Memory::Object_at(fp + offset);
@@ -62,11 +60,7 @@ StackFrame::Type ExitFrame::GetStateForFramePointer(Address fp, State* state) {
   if (is_debug_exit) {
     sp -= kNumJSCallerSaved * kPointerSize;
   }
-  // Fill in the state.
-  state->sp = sp;
-  state->fp = fp;
-  state->pc_address = reinterpret_cast<Address*>(sp - 1 * kPointerSize);
-  return EXIT;
+  return sp;
 }
 
 

diff --git a/src/x64/frames-x64.cc b/src/x64/frames-x64.cc
index fd26535..9c96047 100644 (file)
--- a/src/x64/frames-x64.cc
+++ b/src/x64/frames-x64.cc
@@ -35,18 +35,8 @@ namespace v8 {
 namespace internal {
 
 
-
-
-StackFrame::Type ExitFrame::GetStateForFramePointer(Address fp, State* state) {
-  if (fp == 0) return NONE;
-  // Compute the stack pointer.
-  Address sp = Memory::Address_at(fp + ExitFrameConstants::kSPOffset);
-  // Fill in the state.
-  state->fp = fp;
-  state->sp = sp;
-  state->pc_address = reinterpret_cast<Address*>(sp - 1 * kPointerSize);
-  ASSERT(*state->pc_address != NULL);
-  return EXIT;
+Address ExitFrame::ComputeStackPointer(Address fp) {
+  return Memory::Address_at(fp + ExitFrameConstants::kSPOffset);
 }
 
 

diff --git a/test/cctest/test-log-stack-tracer.cc b/test/cctest/test-log-stack-tracer.cc
index c921176..b0cf93d 100644 (file)
--- a/test/cctest/test-log-stack-tracer.cc
+++ b/test/cctest/test-log-stack-tracer.cc
@@ -206,21 +206,8 @@ static Handle<JSFunction> CompileFunction(const char* source) {
 }
 
 
-static Local<Value> GetGlobalProperty(const char* name) {
-  return env->Global()->Get(String::New(name));
-}
-
-
-static Handle<JSFunction> GetGlobalJSFunction(const char* name) {
-  Handle<JSFunction> result(JSFunction::cast(
-      *v8::Utils::OpenHandle(*GetGlobalProperty(name))));
-  return result;
-}
-
-
-static void CheckObjectIsJSFunction(const char* func_name,
-                                    Address addr) {
-  i::Object* obj = reinterpret_cast<i::Object*>(addr);
+static void CheckJSFunctionAtAddress(const char* func_name, Address addr) {
+  i::Object* obj = i::HeapObject::FromAddress(addr);
   CHECK(obj->IsJSFunction());
   CHECK(JSFunction::cast(obj)->shared()->name()->IsString());
   i::SmartPointer<char> found_name =
@@ -304,7 +291,6 @@ static void CreateTraceCallerFunction(const char* func_name,
 #endif
 
   SetGlobalProperty(func_name, v8::ToApi<Value>(func));
-  CHECK_EQ(*func, *GetGlobalJSFunction(func_name));
 }
 
 
@@ -332,13 +318,13 @@ TEST(CFromJSStackTrace) {
   // script [JS]
   //   JSTrace() [JS]
   //     JSFuncDoTrace() [JS] [captures EBP value and encodes it as Smi]
-  //       trace(EBP encoded as Smi) [native (extension)]
+  //       trace(EBP) [native (extension)]
   //         DoTrace(EBP) [native]
   //           StackTracer::Trace
   CHECK_GT(sample.frames_count, 1);
   // Stack tracing will start from the first JS function, i.e. "JSFuncDoTrace"
-  CheckObjectIsJSFunction("JSFuncDoTrace", sample.stack[0]);
-  CheckObjectIsJSFunction("JSTrace", sample.stack[1]);
+  CheckJSFunctionAtAddress("JSFuncDoTrace", sample.stack[0]);
+  CheckJSFunctionAtAddress("JSTrace", sample.stack[1]);
 }
 
 
@@ -370,19 +356,18 @@ TEST(PureJSStackTrace) {
   // script [JS]
   //   OuterJSTrace() [JS]
   //     JSTrace() [JS]
-  //       JSFuncDoTrace() [JS] [captures EBP value and encodes it as Smi]
-  //         js_trace(EBP encoded as Smi) [native (extension)]
+  //       JSFuncDoTrace() [JS]
+  //         js_trace(EBP) [native (extension)]
   //           DoTraceHideCEntryFPAddress(EBP) [native]
   //             StackTracer::Trace
   //
   // The last JS function called. It is only visible through
   // sample.function, as its return address is above captured EBP value.
-  CHECK_EQ(GetGlobalJSFunction("JSFuncDoTrace")->address(),
-           sample.function);
+  CheckJSFunctionAtAddress("JSFuncDoTrace", sample.function);
   CHECK_GT(sample.frames_count, 1);
   // Stack sampling will start from the caller of JSFuncDoTrace, i.e. "JSTrace"
-  CheckObjectIsJSFunction("JSTrace", sample.stack[0]);
-  CheckObjectIsJSFunction("OuterJSTrace", sample.stack[1]);
+  CheckJSFunctionAtAddress("JSTrace", sample.stack[0]);
+  CheckJSFunctionAtAddress("OuterJSTrace", sample.stack[1]);
 }