From 22d6605f2fe42f7e02e99e27e34a2bdfa97fc5b0 Mon Sep 17 00:00:00 2001 From: "sgjesse@chromium.org" Date: Fri, 6 Mar 2009 11:03:14 +0000 Subject: [PATCH] All preemption requests are now ignored while in the debugger. This ensures that no change of V8 thread happenes while in the debugger. The only thing that happens is that a flag is set to indicate that preemption happened. When the debugger is left preemption is requested if it occourred while in the debugger. Moved the debugger related global variables from Top to thread local in Debug. Review URL: http://codereview.chromium.org/39124 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@1436 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/debug.cc | 32 ++++++++++++-- src/debug.h | 91 +++++++++++++++++++++++++++++++++++---- src/execution.cc | 7 ++- src/runtime.cc | 6 +-- src/top.cc | 45 ------------------- src/top.h | 24 ++--------- test/cctest/test-debug.cc | 17 ++++---- 7 files changed, 132 insertions(+), 90 deletions(-) diff --git a/src/debug.cc b/src/debug.cc index 620739cc30..b704eed12c 100644 --- a/src/debug.cc +++ b/src/debug.cc @@ -400,12 +400,17 @@ DebugInfoListNode* Debug::debug_info_list_ = NULL; // Threading support. void Debug::ThreadInit() { + thread_local_.break_count_ = 0; + thread_local_.break_id_ = 0; + thread_local_.break_frame_id_ = StackFrame::NO_ID; thread_local_.last_step_action_ = StepNone; thread_local_.last_statement_position_ = RelocInfo::kNoPosition; thread_local_.step_count_ = 0; thread_local_.last_fp_ = 0; thread_local_.step_into_fp_ = 0; thread_local_.after_break_target_ = 0; + thread_local_.debugger_entry_ = NULL; + thread_local_.preemption_pending_ = false; } @@ -611,6 +616,13 @@ void Debug::Unload() { } +// Set the flag indicating that preemption happened during debugging. +void Debug::PreemptionWhileInDebugger() { + ASSERT(InDebugger()); + Debug::set_preemption_pending(true); +} + + void Debug::Iterate(ObjectVisitor* v) { v->VisitPointer(bit_cast(&(debug_break_return_entry_))); v->VisitPointer(bit_cast(&(debug_break_return_))); @@ -743,7 +755,7 @@ bool Debug::CheckBreakPoint(Handle break_point_object) { *Factory::LookupAsciiSymbol("IsBreakPointTriggered")))); // Get the break id as an object. - Handle break_id = Factory::NewNumberFromInt(Top::break_id()); + Handle break_id = Factory::NewNumberFromInt(Debug::break_id()); // Call HandleBreakPointx. bool caught_exception = false; @@ -873,7 +885,7 @@ void Debug::FloodWithOneShot(Handle shared) { void Debug::FloodHandlerWithOneShot() { // Iterate through the JavaScript stack looking for handlers. - StackFrame::Id id = Top::break_frame_id(); + StackFrame::Id id = break_frame_id(); if (id == StackFrame::NO_ID) { // If there is no JavaScript stack don't do anything. return; @@ -913,7 +925,7 @@ void Debug::PrepareStep(StepAction step_action, int step_count) { // any. The debug frame will only be present if execution was stopped due to // hitting a break point. In other situations (e.g. unhandled exception) the // debug frame is not present. - StackFrame::Id id = Top::break_frame_id(); + StackFrame::Id id = break_frame_id(); if (id == StackFrame::NO_ID) { // If there is no JavaScript stack don't do anything. return; @@ -1118,6 +1130,18 @@ Handle Debug::GetSourceBreakLocations( } +void Debug::NewBreak(StackFrame::Id break_frame_id) { + thread_local_.break_frame_id_ = break_frame_id; + thread_local_.break_id_ = ++thread_local_.break_count_; +} + + +void Debug::SetBreak(StackFrame::Id break_frame_id, int break_id) { + thread_local_.break_frame_id_ = break_frame_id; + thread_local_.break_id_ = break_id; +} + + // Handle stepping into a function. void Debug::HandleStepIn(Handle function, Address fp, @@ -1381,7 +1405,7 @@ Handle Debugger::MakeJSObject(Vector constructor_name, Handle Debugger::MakeExecutionState(bool* caught_exception) { // Create the execution state object. - Handle break_id = Factory::NewNumberFromInt(Top::break_id()); + Handle break_id = Factory::NewNumberFromInt(Debug::break_id()); const int argc = 1; Object** argv[argc] = { break_id.location() }; return MakeJSObject(CStrVector("MakeExecutionState"), diff --git a/src/debug.h b/src/debug.h index f7760d0eff..41193b41a1 100644 --- a/src/debug.h +++ b/src/debug.h @@ -36,10 +36,16 @@ #include "factory.h" #include "platform.h" #include "string-stream.h" +#include "v8threads.h" namespace v8 { namespace internal { + +// Forward declarations. +class EnterDebugger; + + // Step actions. NOTE: These values are in macros.py as well. enum StepAction { StepNone = -1, // Stepping not prepared. @@ -166,7 +172,8 @@ class Debug { static bool Load(); static void Unload(); static bool IsLoaded() { return !debug_context_.is_null(); } - static bool InDebugger() { return Top::is_break(); } + static bool InDebugger() { return thread_local_.debugger_entry_ != NULL; } + static void PreemptionWhileInDebugger(); static void Iterate(ObjectVisitor* v); static Object* Break(Arguments args); @@ -211,6 +218,16 @@ class Debug { // Fast check to see if any break points are active. inline static bool has_break_points() { return has_break_points_; } + static void NewBreak(StackFrame::Id break_frame_id); + static void SetBreak(StackFrame::Id break_frame_id, int break_id); + static StackFrame::Id break_frame_id() { + return thread_local_.break_frame_id_; + } + static int break_id() { return thread_local_.break_id_; } + + + + static bool StepInActive() { return thread_local_.step_into_fp_ != 0; } static void HandleStepIn(Handle function, Address fp, @@ -218,6 +235,20 @@ class Debug { static Address step_in_fp() { return thread_local_.step_into_fp_; } static Address* step_in_fp_addr() { return &thread_local_.step_into_fp_; } + static EnterDebugger* debugger_entry() { + return thread_local_.debugger_entry_; + } + static void set_debugger_entry(EnterDebugger* entry) { + thread_local_.debugger_entry_ = entry; + } + + static bool preemption_pending() { + return thread_local_.preemption_pending_; + } + static void set_preemption_pending(bool preemption_pending) { + thread_local_.preemption_pending_ = preemption_pending; + } + // Getter and setter for the disable break state. static bool disable_break() { return disable_break_; } static void set_disable_break(bool disable_break) { @@ -313,9 +344,18 @@ class Debug { static bool break_on_exception_; static bool break_on_uncaught_exception_; - // Per-thread: + // Per-thread data. class ThreadLocal { public: + // Counter for generating next break id. + int break_count_; + + // Current break id. + int break_id_; + + // Frame id for the frame of the current break. + StackFrame::Id break_frame_id_; + // Step action for last step performed. StepAction last_step_action_; @@ -333,6 +373,12 @@ class Debug { // Storage location for jump when exiting debug break calls. Address after_break_target_; + + // Top debugger entry. + EnterDebugger* debugger_entry_; + + // Preemption happened while debugging. + bool preemption_pending_; }; // Storage location for registers when handling debug break calls @@ -519,17 +565,34 @@ class DebugMessageThread: public Thread { // some reason could not be entered FailedToEnter will return true. class EnterDebugger BASE_EMBEDDED { public: - EnterDebugger() : has_js_frames_(!it_.done()) { + EnterDebugger() + : prev_(Debug::debugger_entry()), + has_js_frames_(!it_.done()) { + ASSERT(!Debug::preemption_pending()); + + // Link recursive debugger entry. + Debug::set_debugger_entry(this); + + // If a preemption is pending when first entering the debugger clear it as + // we don't want preemption happening while executing JavaScript in the + // debugger. When recursively entering the debugger the preemption flag + // cannot be set as this is disabled while in the debugger (see + // RuntimePreempt). + if (prev_ == NULL && StackGuard::IsPreempted()) { + StackGuard::Continue(PREEMPT); + } + ASSERT(!StackGuard::IsPreempted()); + // Store the previous break id and frame id. - break_id_ = Top::break_id(); - break_frame_id_ = Top::break_frame_id(); + break_id_ = Debug::break_id(); + break_frame_id_ = Debug::break_frame_id(); // Create the new break info. If there is no JavaScript frames there is no // break frame id. if (has_js_frames_) { - Top::new_break(it_.frame()->id()); + Debug::NewBreak(it_.frame()->id()); } else { - Top::new_break(StackFrame::NO_ID); + Debug::NewBreak(StackFrame::NO_ID); } // Make sure that debugger is loaded and enter the debugger context. @@ -543,7 +606,18 @@ class EnterDebugger BASE_EMBEDDED { ~EnterDebugger() { // Restore to the previous break state. - Top::set_break(break_frame_id_, break_id_); + Debug::SetBreak(break_frame_id_, break_id_); + + // Request preemption when leaving the last debugger entry and a preemption + // had been recorded while debugging. This is to avoid starvation in some + // debugging scenarios. + if (prev_ == NULL && Debug::preemption_pending()) { + StackGuard::Preempt(); + Debug::set_preemption_pending(false); + } + + // Leaving this debugger entry. + Debug::set_debugger_entry(prev_); } // Check whether the debugger could be entered. @@ -553,6 +627,7 @@ class EnterDebugger BASE_EMBEDDED { inline bool HasJavaScriptFrames() { return has_js_frames_; } private: + EnterDebugger* prev_; // Previous debugger entry if entered recursively. JavaScriptFrameIterator it_; const bool has_js_frames_; // Were there any JavaScript frames? StackFrame::Id break_frame_id_; // Previous break frame id. diff --git a/src/execution.cc b/src/execution.cc index f721cbd68b..419cc927e4 100644 --- a/src/execution.cc +++ b/src/execution.cc @@ -523,7 +523,12 @@ static Object* RuntimePreempt() { ContextSwitcher::PreemptionReceived(); - { + if (Debug::InDebugger()) { + // If currently in the debugger don't do any actual preemption but record + // that preemption occoured while in the debugger. + Debug::PreemptionWhileInDebugger(); + } else { + // Perform preemption. v8::Unlocker unlocker; Thread::YieldCPU(); } diff --git a/src/runtime.cc b/src/runtime.cc index 2e4d8727f6..89b6a52676 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -4967,7 +4967,7 @@ static Object* Runtime_CheckExecutionState(Arguments args) { ASSERT(args.length() >= 1); CONVERT_NUMBER_CHECKED(int, break_id, Int32, args[0]); // Check that the break id is valid. - if (Top::break_id() == 0 || break_id != Top::break_id()) { + if (Debug::break_id() == 0 || break_id != Debug::break_id()) { return Top::Throw(Heap::illegal_execution_state_symbol()); } @@ -4985,7 +4985,7 @@ static Object* Runtime_GetFrameCount(Arguments args) { // Count all frames which are relevant to debugging stack trace. int n = 0; - StackFrame::Id id = Top::break_frame_id(); + StackFrame::Id id = Debug::break_frame_id(); if (id == StackFrame::NO_ID) { // If there is no JavaScript stack frame count is 0. return Smi::FromInt(0); @@ -5030,7 +5030,7 @@ static Object* Runtime_GetFrameDetails(Arguments args) { CONVERT_NUMBER_CHECKED(int, index, Int32, args[1]); // Find the relevant frame with the requested index. - StackFrame::Id id = Top::break_frame_id(); + StackFrame::Id id = Debug::break_frame_id(); if (id == StackFrame::NO_ID) { // If there are no JavaScript stack frames return undefined. return Heap::undefined_value(); diff --git a/src/top.cc b/src/top.cc index ed4cb4257d..05a38366d6 100644 --- a/src/top.cc +++ b/src/top.cc @@ -38,9 +38,6 @@ namespace v8 { namespace internal { ThreadLocalTop Top::thread_local_; Mutex* Top::break_access_ = OS::CreateMutex(); -StackFrame::Id Top::break_frame_id_; -int Top::break_count_; -int Top::break_id_; NoAllocationStringAllocator* preallocated_message_space = NULL; @@ -222,10 +219,6 @@ void Top::Initialize() { InitializeThreadLocal(); - break_frame_id_ = StackFrame::NO_ID; - break_count_ = 0; - break_id_ = 0; - // Only preallocate on the first initialization. if (FLAG_preallocate_message_memory && (preallocated_message_space == NULL)) { // Start the thread which will set aside some memory. @@ -295,44 +288,6 @@ void Top::UnregisterTryCatchHandler(v8::TryCatch* that) { } -void Top::new_break(StackFrame::Id break_frame_id) { - ExecutionAccess access; - break_frame_id_ = break_frame_id; - break_id_ = ++break_count_; -} - - -void Top::set_break(StackFrame::Id break_frame_id, int break_id) { - ExecutionAccess access; - break_frame_id_ = break_frame_id; - break_id_ = break_id; -} - - -bool Top::check_break(int break_id) { - ExecutionAccess access; - return break_id == break_id_; -} - - -bool Top::is_break() { - ExecutionAccess access; - return break_id_ != 0; -} - - -StackFrame::Id Top::break_frame_id() { - ExecutionAccess access; - return break_frame_id_; -} - - -int Top::break_id() { - ExecutionAccess access; - return break_id_; -} - - void Top::MarkCompactPrologue(bool is_compacting) { MarkCompactPrologue(is_compacting, &thread_local_); } diff --git a/src/top.h b/src/top.h index 26151bdc97..175a7f6f03 100644 --- a/src/top.h +++ b/src/top.h @@ -182,13 +182,6 @@ class Top { // Generated code scratch locations. static void* formal_count_address() { return &thread_local_.formal_count_; } - static void new_break(StackFrame::Id break_frame_id); - static void set_break(StackFrame::Id break_frame_id, int break_id); - static bool check_break(int break_id); - static bool is_break(); - static StackFrame::Id break_frame_id(); - static int break_id(); - static void MarkCompactPrologue(bool is_compacting); static void MarkCompactEpilogue(bool is_compacting); static void MarkCompactPrologue(bool is_compacting, @@ -304,15 +297,6 @@ class Top { // Mutex for serializing access to break control structures. static Mutex* break_access_; - // ID of the frame where execution is stopped by debugger. - static StackFrame::Id break_frame_id_; - - // Counter to create unique id for each debug break. - static int break_count_; - - // Current debug break, 0 if running. - static int break_id_; - friend class SaveContext; friend class AssertNoContextChange; friend class ExecutionAccess; @@ -326,12 +310,12 @@ class Top { // versions of GCC. See V8 issue 122 for details. class SaveContext BASE_EMBEDDED { public: - SaveContext() : - context_(Top::context()), + SaveContext() + : context_(Top::context()), #if __GNUC_VERSION__ >= 40100 && __GNUC_VERSION__ < 40300 - dummy_(Top::context()), + dummy_(Top::context()), #endif - prev_(Top::save_context()) { + prev_(Top::save_context()) { Top::set_save_context(this); // If there is no JS frame under the current C frame, use the value 0. diff --git a/test/cctest/test-debug.cc b/test/cctest/test-debug.cc index f8a2dd03d9..253ab44a8f 100644 --- a/test/cctest/test-debug.cc +++ b/test/cctest/test-debug.cc @@ -511,7 +511,7 @@ static void DebugEventBreakPointHitCount(v8::DebugEvent event, v8::Handle event_data, v8::Handle data) { // When hitting a debug event listener there must be a break set. - CHECK(v8::internal::Top::is_break()); + CHECK(v8::internal::Debug::break_id() != 0); // Count the number of breaks. if (event == v8::Break) { @@ -551,7 +551,7 @@ static void DebugEventCounter(v8::DebugEvent event, v8::Handle event_data, v8::Handle data) { // When hitting a debug event listener there must be a break set. - CHECK(v8::internal::Top::is_break()); + CHECK(v8::internal::Debug::break_id() != 0); // Count the number of breaks. if (event == v8::Break) { @@ -609,7 +609,7 @@ static void DebugEventEvaluate(v8::DebugEvent event, v8::Handle event_data, v8::Handle data) { // When hitting a debug event listener there must be a break set. - CHECK(v8::internal::Top::is_break()); + CHECK(v8::internal::Debug::break_id() != 0); if (event == v8::Break) { for (int i = 0; checks[i].expr != NULL; i++) { @@ -635,7 +635,7 @@ static void DebugEventRemoveBreakPoint(v8::DebugEvent event, v8::Handle event_data, v8::Handle data) { // When hitting a debug event listener there must be a break set. - CHECK(v8::internal::Top::is_break()); + CHECK(v8::internal::Debug::break_id() != 0); if (event == v8::Break) { break_point_hit_count++; @@ -653,7 +653,7 @@ static void DebugEventStep(v8::DebugEvent event, v8::Handle event_data, v8::Handle data) { // When hitting a debug event listener there must be a break set. - CHECK(v8::internal::Top::is_break()); + CHECK(v8::internal::Debug::break_id() != 0); if (event == v8::Break) { break_point_hit_count++; @@ -679,7 +679,7 @@ static void DebugEventStepSequence(v8::DebugEvent event, v8::Handle event_data, v8::Handle data) { // When hitting a debug event listener there must be a break set. - CHECK(v8::internal::Top::is_break()); + CHECK(v8::internal::Debug::break_id() != 0); if (event == v8::Break || event == v8::Exception) { // Check that the current function is the expected. @@ -709,7 +709,7 @@ static void DebugEventBreakPointCollectGarbage( v8::Handle event_data, v8::Handle data) { // When hitting a debug event listener there must be a break set. - CHECK(v8::internal::Top::is_break()); + CHECK(v8::internal::Debug::break_id() != 0); // Perform a garbage collection when break point is hit and continue. Based // on the number of break points hit either scavenge or mark compact @@ -734,7 +734,7 @@ static void DebugEventBreak(v8::DebugEvent event, v8::Handle event_data, v8::Handle data) { // When hitting a debug event listener there must be a break set. - CHECK(v8::internal::Top::is_break()); + CHECK(v8::internal::Debug::break_id() != 0); if (event == v8::Break) { // Count the number of breaks. @@ -3695,4 +3695,3 @@ TEST(DebuggerHostDispatch) { // The host dispatch callback should be called. CHECK_EQ(1, host_dispatch_hit_count); } -