Change the handling of the debug break stack guard. The debug break is no...

Change the handling of the debug break stack guard. The debug break is no longer ignored when hit inside "system" JavaScript. The reason for this is twofold:

* Running "system" JavaScript with the debug break flag active leads to slow running code while waiting for the break in non "system" JavaScript (one exception to this it is to try to avoid breaks in the clear mirror cache JavaScript code called when leaving the debugger).

* If this happens while processing RegExp running in native code an infinite loop is created as the stack guard handler for RegExp does not move execution forward

Fixed a GC bug in the interrupt handling for RegExp running in native code.

Added test of debug break while in debug message handler callback and debug break while executing a RegExp.
Review URL: http://codereview.chromium.org/115262

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

src/debug.cc

@@ -443,7 +443,7 @@ void Debug::ThreadInit() {
   thread_local_.step_into_fp_ = 0;
   thread_local_.after_break_target_ = 0;
   thread_local_.debugger_entry_ = NULL;
-  thread_local_.preemption_pending_ = false;
+  thread_local_.pending_interrupts_ = 0;
 }
 
 
@@ -727,7 +727,7 @@ void Debug::Unload() {
 // Set the flag indicating that preemption happened during debugging.
 void Debug::PreemptionWhileInDebugger() {
   ASSERT(InDebugger());
-  Debug::set_preemption_pending(true);
+  Debug::set_interrupts_pending(PREEMPT);
 }
 
 
@@ -1927,6 +1927,11 @@ void Debugger::ProcessDebugEvent(v8::DebugEvent event,
                                  bool auto_continue) {
   HandleScope scope;
 
+  // Clear any pending debug break if this is a real break.
+  if (!auto_continue) {
+    Debug::clear_interrupt_pending(DEBUGBREAK);
+  }
+
   // Create the execution state.
   bool caught_exception = false;
   Handle<Object> exec_state = MakeExecutionState(&caught_exception);

src/debug.h

@@ -282,11 +282,19 @@ class Debug {
     thread_local_.debugger_entry_ = entry;
   }
 
-  static bool preemption_pending() {
-    return thread_local_.preemption_pending_;
+  // Check whether any of the specified interrupts are pending.
+  static bool is_interrupt_pending(InterruptFlag what) {
+    return (thread_local_.pending_interrupts_ & what) != 0;
   }
-  static void set_preemption_pending(bool preemption_pending) {
-    thread_local_.preemption_pending_ = preemption_pending;
+
+  // Set specified interrupts as pending.
+  static void set_interrupts_pending(InterruptFlag what) {
+    thread_local_.pending_interrupts_ |= what;
+  }
+
+  // Clear specified interrupts from pending.
+  static void clear_interrupt_pending(InterruptFlag what) {
+    thread_local_.pending_interrupts_ &= ~static_cast<int>(what);
   }
 
   // Getter and setter for the disable break state.
@@ -431,8 +439,8 @@ class Debug {
     // Top debugger entry.
     EnterDebugger* debugger_entry_;
 
-    // Preemption happened while debugging.
-    bool preemption_pending_;
+    // Pending interrupts scheduled while debugging.
+    int pending_interrupts_;
   };
 
   // Storage location for registers when handling debug break calls
@@ -679,7 +687,8 @@ class EnterDebugger BASE_EMBEDDED {
   EnterDebugger()
       : prev_(Debug::debugger_entry()),
         has_js_frames_(!it_.done()) {
-    ASSERT(prev_ == NULL ? !Debug::preemption_pending() : true);
+    ASSERT(prev_ != NULL || !Debug::is_interrupt_pending(PREEMPT));
+    ASSERT(prev_ != NULL || !Debug::is_interrupt_pending(DEBUGBREAK));
 
     // Link recursive debugger entry.
     Debug::set_debugger_entry(this);
@@ -709,28 +718,41 @@ class EnterDebugger BASE_EMBEDDED {
     // Restore to the previous break state.
     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);
-    }
-
-    // If there are commands in the queue when leaving the debugger request that
-    // these commands are processed.
-    if (prev_ == NULL && Debugger::HasCommands()) {
-      StackGuard::DebugCommand();
-    }
-
+    // Check for leaving the debugger.
     if (prev_ == NULL) {
       // Clear mirror cache when leaving the debugger. Skip this if there is a
       // pending exception as clearing the mirror cache calls back into
       // JavaScript. This can happen if the v8::Debug::Call is used in which
       // case the exception should end up in the calling code.
       if (!Top::has_pending_exception()) {
+        // Try to avoid any pending debug break breaking in the clear mirror
+        // cache JavaScript code.
+        if (StackGuard::IsDebugBreak()) {
+          Debug::set_interrupts_pending(DEBUGBREAK);
+          StackGuard::Continue(DEBUGBREAK);
+        }
         Debug::ClearMirrorCache();
       }
+
+      // Request preemption and debug break when leaving the last debugger entry
+      // if any of these where recorded while debugging.
+      if (Debug::is_interrupt_pending(PREEMPT)) {
+        // This re-scheduling of preemption is to avoid starvation in some
+        // debugging scenarios.
+        Debug::clear_interrupt_pending(PREEMPT);
+        StackGuard::Preempt();
+      }
+      if (Debug::is_interrupt_pending(DEBUGBREAK)) {
+        Debug::clear_interrupt_pending(DEBUGBREAK);
+        StackGuard::DebugBreak();
+      }
+
+      // If there are commands in the queue when leaving the debugger request
+      // that these commands are processed.
+      if (Debugger::HasCommands()) {
+        StackGuard::DebugCommand();
+      }
+
       // If leaving the debugger with the debugger no longer active unload it.
       if (!Debugger::IsDebuggerActive()) {
         Debugger::UnloadDebugger();

src/execution.cc

@@ -588,32 +588,15 @@ Object* Execution::DebugBreakHelper() {
     return Heap::undefined_value();
   }
 
-  // Don't break in system functions. If the current function is
-  // either in the builtins object of some context or is in the debug
-  // context just return with the debug break stack guard active.
-  JavaScriptFrameIterator it;
-  JavaScriptFrame* frame = it.frame();
-  Object* fun = frame->function();
-  if (fun->IsJSFunction()) {
-    GlobalObject* global = JSFunction::cast(fun)->context()->global();
-    if (global->IsJSBuiltinsObject() || Debug::IsDebugGlobal(global)) {
-      return Heap::undefined_value();
-    }
-  }
-
-  // Check for debug command break only.
+  // Collect the break state before clearing the flags.
   bool debug_command_only =
       StackGuard::IsDebugCommand() && !StackGuard::IsDebugBreak();
+  bool is_debug_break = StackGuard::IsDebugBreak();
 
   // Clear the debug request flags.
   StackGuard::Continue(DEBUGBREAK);
   StackGuard::Continue(DEBUGCOMMAND);
 
-  // If debug command only and already in debugger ignore it.
-  if (debug_command_only && Debug::InDebugger()) {
-    return Heap::undefined_value();
-  }
-
   HandleScope scope;
   // Enter the debugger. Just continue if we fail to enter the debugger.
   EnterDebugger debugger;
@@ -621,7 +604,8 @@ Object* Execution::DebugBreakHelper() {
     return Heap::undefined_value();
   }
 
-  // Notify the debug event listeners.
+  // Notify the debug event listeners. Indicate auto continue if the break was
+  // a debug command break.
   Debugger::OnDebugBreak(Factory::undefined_value(), debug_command_only);
 
   // Return to continue execution.

src/ia32/regexp-macro-assembler-ia32.cc

@@ -1195,10 +1195,11 @@ int RegExpMacroAssemblerIA32::CheckStackGuardState(Address* return_address,
   const byte* new_address = StringCharacterPosition(*subject, start_index);
 
   if (start_address != new_address) {
-    // If there is a difference, update start and end addresses in the
-    // RegExp stack frame to match the new value.
+    // If there is a difference, update the object pointer and start and end
+    // addresses in the RegExp stack frame to match the new value.
     const byte* end_address = frame_entry<const byte* >(re_frame, kInputEnd);
     int byte_length = end_address - start_address;
+    frame_entry<const String*>(re_frame, kInputString) = *subject;
     frame_entry<const byte*>(re_frame, kInputStart) = new_address;
     frame_entry<const byte*>(re_frame, kInputEnd) = new_address + byte_length;
   }

test/cctest/test-debug.cc

@@ -4776,6 +4776,117 @@ TEST(ContextData) {
 }
 
 
+// Debug message handler which issues a debug break when it hits a break event.
+static int message_handler_break_hit_count = 0;
+static void DebugBreakMessageHandler(const v8::Debug::Message& message) {
+  // Schedule a debug break for break events.
+  if (message.IsEvent() && message.GetEvent() == v8::Break) {
+    message_handler_break_hit_count++;
+    if (message_handler_break_hit_count == 1) {
+      v8::Debug::DebugBreak();
+    }
+  }
+
+  // Issue a continue command if this event will not cause the VM to start
+  // running.
+  if (!message.WillStartRunning()) {
+    SendContinueCommand();
+  }
+}
+
+
+// Test that a debug break can be scheduled while in a message handler.
+TEST(DebugBreakInMessageHandler) {
+  v8::HandleScope scope;
+  DebugLocalContext env;
+
+  v8::Debug::SetMessageHandler2(DebugBreakMessageHandler);
+
+  // Test functions.
+  const char* script = "function f() { debugger; } function g() { }";
+  CompileRun(script);
+  v8::Local<v8::Function> f =
+      v8::Local<v8::Function>::Cast(env->Global()->Get(v8::String::New("f")));
+  v8::Local<v8::Function> g =
+      v8::Local<v8::Function>::Cast(env->Global()->Get(v8::String::New("g")));
+
+  // Call f then g. The debugger statement in f will casue a break which will
+  // cause another break.
+  f->Call(env->Global(), 0, NULL);
+  CHECK_EQ(2, message_handler_break_hit_count);
+  // Calling g will not cause any additional breaks.
+  g->Call(env->Global(), 0, NULL);
+  CHECK_EQ(2, message_handler_break_hit_count);
+}
+
+
+// Debug event handler which gets the function on the top frame and schedules a
+// break a number of times.
+static void DebugEventDebugBreak(
+    v8::DebugEvent event,
+    v8::Handle<v8::Object> exec_state,
+    v8::Handle<v8::Object> event_data,
+    v8::Handle<v8::Value> data) {
+
+  if (event == v8::Break) {
+    break_point_hit_count++;
+
+    // Get the name of the top frame function.
+    if (!frame_function_name.IsEmpty()) {
+      // Get the name of the function.
+      const int argc = 1;
+      v8::Handle<v8::Value> argv[argc] = { exec_state };
+      v8::Handle<v8::Value> result = frame_function_name->Call(exec_state,
+                                                               argc, argv);
+      if (result->IsUndefined()) {
+        last_function_hit[0] = '\0';
+      } else {
+        CHECK(result->IsString());
+        v8::Handle<v8::String> function_name(result->ToString());
+        function_name->WriteAscii(last_function_hit);
+      }
+    }
+
+    // Keep forcing breaks.
+    if (break_point_hit_count < 20) {
+      v8::Debug::DebugBreak();
+    }
+  }
+}
+
+
+TEST(RegExpDebugBreak) {
+  v8::HandleScope scope;
+  DebugLocalContext env;
+
+  i::FLAG_regexp_native = true;
+
+  // Create a function for checking the function when hitting a break point.
+  frame_function_name = CompileFunction(&env,
+                                        frame_function_name_source,
+                                        "frame_function_name");
+
+  // Test RegExp which matches white spaces and comments at the begining of a
+  // source line.
+  const char* script =
+    "var sourceLineBeginningSkip = /^(?:[ \\v\\h]*(?:\\/\\*.*?\\*\\/)*)*/;\n"
+    "function f(s) { return s.match(sourceLineBeginningSkip)[0].length; }";
+
+  v8::Local<v8::Function> f = CompileFunction(script, "f");
+  const int argc = 1;
+  v8::Handle<v8::Value> argv[argc] = { v8::String::New("  /* xxx */ a=0;") };
+  v8::Local<v8::Value> result = f->Call(env->Global(), argc, argv);
+  CHECK_EQ(12, result->Int32Value());
+
+  v8::Debug::SetDebugEventListener(DebugEventDebugBreak);
+  v8::Debug::DebugBreak();
+  result = f->Call(env->Global(), argc, argv);
+
+  CHECK_EQ(20, break_point_hit_count);
+  CHECK_EQ("exec", last_function_hit);
+}
+
+
 // Common part of EvalContextData and NestedBreakEventContextData tests.
 static void ExecuteScriptForContextCheck() {
   // Create a context.