All preemption requests are now ignored while in the debugger. This ensures...

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

@@ -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<Object**, Code**>(&(debug_break_return_entry_)));
   v->VisitPointer(bit_cast<Object**, Code**>(&(debug_break_return_)));
@@ -743,7 +755,7 @@ bool Debug::CheckBreakPoint(Handle<Object> break_point_object) {
           *Factory::LookupAsciiSymbol("IsBreakPointTriggered"))));
 
   // Get the break id as an object.
-  Handle<Object> break_id = Factory::NewNumberFromInt(Top::break_id());
+  Handle<Object> break_id = Factory::NewNumberFromInt(Debug::break_id());
 
   // Call HandleBreakPointx.
   bool caught_exception = false;
@@ -873,7 +885,7 @@ void Debug::FloodWithOneShot(Handle<SharedFunctionInfo> 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<Object> 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<JSFunction> function,
                          Address fp,
@@ -1381,7 +1405,7 @@ Handle<Object> Debugger::MakeJSObject(Vector<const char> constructor_name,
 
 Handle<Object> Debugger::MakeExecutionState(bool* caught_exception) {
   // Create the execution state object.
-  Handle<Object> break_id = Factory::NewNumberFromInt(Top::break_id());
+  Handle<Object> break_id = Factory::NewNumberFromInt(Debug::break_id());
   const int argc = 1;
   Object** argv[argc] = { break_id.location() };
   return MakeJSObject(CStrVector("MakeExecutionState"),

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<JSFunction> 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.

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();
   }

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();

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_);
 }

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.

test/cctest/test-debug.cc

@@ -511,7 +511,7 @@ static void DebugEventBreakPointHitCount(v8::DebugEvent event,
                                          v8::Handle<v8::Object> event_data,
                                          v8::Handle<v8::Value> 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<v8::Object> event_data,
                               v8::Handle<v8::Value> 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<v8::Object> event_data,
                                v8::Handle<v8::Value> 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<v8::Object> event_data,
                                        v8::Handle<v8::Value> 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<v8::Object> event_data,
                            v8::Handle<v8::Value> 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<v8::Object> event_data,
                                    v8::Handle<v8::Value> 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<v8::Object> event_data,
     v8::Handle<v8::Value> 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<v8::Object> event_data,
                             v8::Handle<v8::Value> 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);
 }
-