From: mikhail.naganov@gmail.com Date: Thu, 16 Sep 2010 08:23:34 +0000 (+0000) Subject: Enhance SafeStackFrameIterator to avoid triggering assertions in debug mode. X-Git-Tag: upstream/4.7.83~21200 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=187d249d924f58e8610970feed8f368ad3cad259;p=platform%2Fupstream%2Fv8.git 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 --- 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(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 --- a/src/frames.cc +++ b/src/frames.cc @@ -143,8 +143,8 @@ void StackFrameIterator::Reset() { state.pc_address = reinterpret_cast(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
(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(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 --- 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 --- 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(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 --- 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
(it.frame()->function()); + sample->stack[i++] = + reinterpret_cast
(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 --- 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(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 --- 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(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 --- a/test/cctest/test-log-stack-tracer.cc +++ b/test/cctest/test-log-stack-tracer.cc @@ -206,21 +206,8 @@ static Handle CompileFunction(const char* source) { } -static Local GetGlobalProperty(const char* name) { - return env->Global()->Get(String::New(name)); -} - - -static Handle GetGlobalJSFunction(const char* name) { - Handle result(JSFunction::cast( - *v8::Utils::OpenHandle(*GetGlobalProperty(name)))); - return result; -} - - -static void CheckObjectIsJSFunction(const char* func_name, - Address addr) { - i::Object* obj = reinterpret_cast(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 found_name = @@ -304,7 +291,6 @@ static void CreateTraceCallerFunction(const char* func_name, #endif SetGlobalProperty(func_name, v8::ToApi(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]); }