Some more debugger-related refactorings.

src/debug.cc

Index: src/debug.cc
diff --git a/src/debug.cc b/src/debug.cc
index 752dd25da3d9a6c6ec848257e2b4b7454bff91c7..c2b6e5bc159a2c1c508d30bd4160dac0be28bbc8 100644
--- a/src/debug.cc
+++ b/src/debug.cc
@@ -80,6 +80,22 @@ BreakLocationIterator::~BreakLocationIterator() {
 }
 
 
+// Check whether a code stub with the specified major key is a possible break
+// point location when looking for source break locations.
+static bool IsSourceBreakStub(Code* code) {
+  CodeStub::Major major_key = CodeStub::GetMajorKey(code);
+  return major_key == CodeStub::CallFunction;
+}
+
+
+// Check whether a code stub with the specified major key is a possible break
+// location.
+static bool IsBreakStub(Code* code) {
+  CodeStub::Major major_key = CodeStub::GetMajorKey(code);
+  return major_key == CodeStub::CallFunction;
+}
+
+
 void BreakLocationIterator::Next() {
   DisallowHeapAllocation no_gc;
   ASSERT(!RinfoDone());
@@ -129,15 +145,14 @@ void BreakLocationIterator::Next() {
         if (IsDebuggerStatement()) {
           break_point_++;
           return;
-        }
-        if (type_ == ALL_BREAK_LOCATIONS) {
-          if (Debug::IsBreakStub(code)) {
+        } else if (type_ == ALL_BREAK_LOCATIONS) {
+          if (IsBreakStub(code)) {
             break_point_++;
             return;
           }
         } else {
           ASSERT(type_ == SOURCE_BREAK_LOCATIONS);
-          if (Debug::IsSourceBreakStub(code)) {
+          if (IsSourceBreakStub(code)) {
             break_point_++;
             return;
           }
@@ -422,6 +437,53 @@ bool BreakLocationIterator::IsDebugBreak() {
 }
 
 
+// Find the builtin to use for invoking the debug break
+static Handle<Code> DebugBreakForIC(Handle<Code> code, RelocInfo::Mode mode) {
+  Isolate* isolate = code->GetIsolate();
+
+  // Find the builtin debug break function matching the calling convention
+  // used by the call site.
+  if (code->is_inline_cache_stub()) {
+    switch (code->kind()) {
+      case Code::CALL_IC:
+        return isolate->builtins()->CallICStub_DebugBreak();
+
+      case Code::LOAD_IC:
+        return isolate->builtins()->LoadIC_DebugBreak();
+
+      case Code::STORE_IC:
+        return isolate->builtins()->StoreIC_DebugBreak();
+
+      case Code::KEYED_LOAD_IC:
+        return isolate->builtins()->KeyedLoadIC_DebugBreak();
+
+      case Code::KEYED_STORE_IC:
+        return isolate->builtins()->KeyedStoreIC_DebugBreak();
+
+      case Code::COMPARE_NIL_IC:
+        return isolate->builtins()->CompareNilIC_DebugBreak();
+
+      default:
+        UNREACHABLE();
+    }
+  }
+  if (RelocInfo::IsConstructCall(mode)) {
+    if (code->has_function_cache()) {
+      return isolate->builtins()->CallConstructStub_Recording_DebugBreak();
+    } else {
+      return isolate->builtins()->CallConstructStub_DebugBreak();
+    }
+  }
+  if (code->kind() == Code::STUB) {
+    ASSERT(code->major_key() == CodeStub::CallFunction);
+    return isolate->builtins()->CallFunctionStub_DebugBreak();
+  }
+
+  UNREACHABLE();
+  return Handle<Code>::null();
+}
+
+
 void BreakLocationIterator::SetDebugBreakAtIC() {
   // Patch the original code with the current address as the current address
   // might have changed by the inline caching since the code was copied.
@@ -434,7 +496,7 @@ void BreakLocationIterator::SetDebugBreakAtIC() {
 
     // Patch the code to invoke the builtin debug break function matching the
     // calling convention used by the call site.
-    Handle<Code> dbgbrk_code(Debug::FindDebugBreak(target_code, mode));
+    Handle<Code> dbgbrk_code = DebugBreakForIC(target_code, mode);
     rinfo()->set_target_address(dbgbrk_code->entry());
   }
 }
@@ -504,7 +566,6 @@ void Debug::ThreadInit() {
   thread_local_.step_out_fp_ = 0;
   // TODO(isolates): frames_are_dropped_?
   thread_local_.debugger_entry_ = NULL;
-  thread_local_.has_pending_interrupt_ = false;
   thread_local_.restarter_frame_function_pointer_ = NULL;
   thread_local_.promise_on_stack_ = NULL;
 }
@@ -530,6 +591,30 @@ int Debug::ArchiveSpacePerThread() {
 }
 
 
+ScriptCache::ScriptCache(Isolate* isolate) : HashMap(HashMap::PointersMatch),
+                                             isolate_(isolate),
+                                             collected_scripts_(10) {
+  Heap* heap = isolate_->heap();
+  HandleScope scope(isolate_);
+
+  // Perform two GCs to get rid of all unreferenced scripts. The first GC gets
+  // rid of all the cached script wrappers and the second gets rid of the
+  // scripts which are no longer referenced.  The second also sweeps precisely,
+  // which saves us doing yet another GC to make the heap iterable.
+  heap->CollectAllGarbage(Heap::kNoGCFlags, "Debug::CreateScriptCache");

[ulan, 2014/05/28 13:24:32]

The comment is out of sync: there is only one GC and the heap is not iterable
afterwards.

If incremental marking is in progress then this will not trigger old space GC.

[Yang, 2014/06/02 12:19:31]

The comment actually makes sense:
Script wrappers are weak global handles. The first GC needs to trigger their
weak callbacks. The second one then removes dead script objects. Both have to be
done here because HeapIterator only does a GC if the heap is not yet iterable.

I'm not entirely sure whether the first GC has be to kMakeHeapIterableMask or
just kSweepPreciselyMask though.

+
+  // Scan heap for Script objects.
+  HeapIterator iterator(heap);
+  DisallowHeapAllocation no_allocation;
+
+  for (HeapObject* obj = iterator.next(); obj != NULL; obj = iterator.next()) {
+    if (obj->IsScript() && Script::cast(obj)->HasValidSource()) {
+      Add(Handle<Script>(Script::cast(obj)));
+    }
+  }
+}
+
+
 void ScriptCache::Add(Handle<Script> script) {
   GlobalHandles* global_handles = isolate_->global_handles();
   // Create an entry in the hash map for the script.
@@ -785,7 +870,10 @@ void Debug::Unload() {
   if (!is_loaded()) return;
 
   // Clear the script cache.
-  DestroyScriptCache();
+  if (script_cache_ != NULL) {
+    delete script_cache_;
+    script_cache_ = NULL;
+  }
 
   // Clear debugger context global handle.
   GlobalHandles::Destroy(Handle<Object>::cast(debug_context_).location());
@@ -843,7 +931,8 @@ void Debug::Break(Arguments args, JavaScriptFrame* frame) {
 
   // If step out is active skip everything until the frame where we need to step
   // out to is reached, unless real breakpoint is hit.
-  if (StepOutActive() && frame->fp() != step_out_fp() &&
+  if (StepOutActive() &&
+      frame->fp() != thread_local_.step_out_fp_ &&
       break_points_hit->IsUndefined() ) {
       // Step count should always be 0 for StepOut.
       ASSERT(thread_local_.step_count_ == 0);
@@ -1192,7 +1281,7 @@ bool Debug::IsBreakOnException(ExceptionBreakType type) {
 }
 
 
-Debug::PromiseOnStack::PromiseOnStack(Isolate* isolate,
+PromiseOnStack::PromiseOnStack(Isolate* isolate,
                                       PromiseOnStack* prev,
                                       Handle<JSFunction> getter)
     : isolate_(isolate), prev_(prev) {
@@ -1203,7 +1292,7 @@ Debug::PromiseOnStack::PromiseOnStack(Isolate* isolate,
 }
 
 
-Debug::PromiseOnStack::~PromiseOnStack() {
+PromiseOnStack::~PromiseOnStack() {
   isolate_->global_handles()->Destroy(Handle<Object>::cast(getter_).location());
 }
 
@@ -1530,67 +1619,7 @@ bool Debug::IsDebugBreak(Address addr) {
 }
 
 
-// Check whether a code stub with the specified major key is a possible break
-// point location when looking for source break locations.
-bool Debug::IsSourceBreakStub(Code* code) {
-  CodeStub::Major major_key = CodeStub::GetMajorKey(code);
-  return major_key == CodeStub::CallFunction;
-}
-
-
-// Check whether a code stub with the specified major key is a possible break
-// location.
-bool Debug::IsBreakStub(Code* code) {
-  CodeStub::Major major_key = CodeStub::GetMajorKey(code);
-  return major_key == CodeStub::CallFunction;
-}
-
-
-// Find the builtin to use for invoking the debug break
-Handle<Code> Debug::FindDebugBreak(Handle<Code> code, RelocInfo::Mode mode) {
-  Isolate* isolate = code->GetIsolate();
-
-  // Find the builtin debug break function matching the calling convention
-  // used by the call site.
-  if (code->is_inline_cache_stub()) {
-    switch (code->kind()) {
-      case Code::CALL_IC:
-        return isolate->builtins()->CallICStub_DebugBreak();
-
-      case Code::LOAD_IC:
-        return isolate->builtins()->LoadIC_DebugBreak();
-
-      case Code::STORE_IC:
-        return isolate->builtins()->StoreIC_DebugBreak();
-
-      case Code::KEYED_LOAD_IC:
-        return isolate->builtins()->KeyedLoadIC_DebugBreak();
 
-      case Code::KEYED_STORE_IC:
-        return isolate->builtins()->KeyedStoreIC_DebugBreak();
-
-      case Code::COMPARE_NIL_IC:
-        return isolate->builtins()->CompareNilIC_DebugBreak();
-
-      default:
-        UNREACHABLE();
-    }
-  }
-  if (RelocInfo::IsConstructCall(mode)) {
-    if (code->has_function_cache()) {
-      return isolate->builtins()->CallConstructStub_Recording_DebugBreak();
-    } else {
-      return isolate->builtins()->CallConstructStub_DebugBreak();
-    }
-  }
-  if (code->kind() == Code::STUB) {
-    ASSERT(code->major_key() == CodeStub::CallFunction);
-    return isolate->builtins()->CallFunctionStub_DebugBreak();
-  }
-
-  UNREACHABLE();
-  return Handle<Code>::null();
-}
 
 
 // Simple function for returning the source positions for active break points.
@@ -1655,7 +1684,7 @@ void Debug::HandleStepIn(Handle<JSFunction> function,
 
   // Flood the function with one-shot break points if it is called from where
   // step into was requested.
-  if (fp == step_in_fp()) {
+  if (fp == thread_local_.step_into_fp_) {
     if (function->shared()->bound()) {
       // Handle Function.prototype.bind
       Debug::FloodBoundFunctionWithOneShot(function);
@@ -1912,7 +1941,7 @@ class ActiveFunctionsRedirector : public ThreadVisitor {
 };
 
 
-void Debug::EnsureFunctionHasDebugBreakSlots(Handle<JSFunction> function) {
+static void EnsureFunctionHasDebugBreakSlots(Handle<JSFunction> function) {
   if (function->code()->kind() == Code::FUNCTION &&
       function->code()->has_debug_break_slots()) {
     // Nothing to do. Function code already had debug break slots.
@@ -1931,7 +1960,7 @@ void Debug::EnsureFunctionHasDebugBreakSlots(Handle<JSFunction> function) {
 }
 
 
-void Debug::RecompileAndRelocateSuspendedGenerators(
+static void RecompileAndRelocateSuspendedGenerators(
     const List<Handle<JSGeneratorObject> > &generators) {
   for (int i = 0; i < generators.length(); i++) {
     Handle<JSFunction> fun(generators[i]->function());
@@ -2444,60 +2473,10 @@ void Debug::ClearMirrorCache() {
 }
 
 
-void Debug::CreateScriptCache() {
-  Heap* heap = isolate_->heap();
-  HandleScope scope(isolate_);
-
-  // Perform two GCs to get rid of all unreferenced scripts. The first GC gets
-  // rid of all the cached script wrappers and the second gets rid of the
-  // scripts which are no longer referenced.  The second also sweeps precisely,
-  // which saves us doing yet another GC to make the heap iterable.
-  heap->CollectAllGarbage(Heap::kNoGCFlags, "Debug::CreateScriptCache");
-
-  ASSERT(script_cache_ == NULL);
-  script_cache_ = new ScriptCache(isolate_);
-
-  // Scan heap for Script objects.
-  int count = 0;
-  HeapIterator iterator(heap);
-
-  for (HeapObject* obj = iterator.next(); obj != NULL; obj = iterator.next()) {
-    if (obj->IsScript() && Script::cast(obj)->HasValidSource()) {
-      script_cache_->Add(Handle<Script>(Script::cast(obj)));
-      count++;
-    }
-  }
-}
-
-
-void Debug::DestroyScriptCache() {
-  // Get rid of the script cache if it was created.
-  if (script_cache_ != NULL) {
-    delete script_cache_;
-    script_cache_ = NULL;
-  }
-}
-
-
-void Debug::AddScriptToScriptCache(Handle<Script> script) {
-  if (script_cache_ != NULL) {
-    script_cache_->Add(script);
-  }
-}
-
-
 Handle<FixedArray> Debug::GetLoadedScripts() {
   // Create and fill the script cache when the loaded scripts is requested for
   // the first time.
-  if (script_cache_ == NULL) {
-    CreateScriptCache();
-  }
-
-  // If the script cache is not active just return an empty array.
-  ASSERT(script_cache_ != NULL);
-  if (script_cache_ == NULL) {
-    isolate_->factory()->NewFixedArray(0);
-  }
+  if (script_cache_ == NULL) script_cache_ = new ScriptCache(isolate_);
 
   // Perform GC to get unreferenced scripts evicted from the cache before
   // returning the content.
@@ -2602,22 +2581,19 @@ MaybeHandle<Object> Debug::MakeScriptCollectedEvent(int id) {
 
 
 void Debug::OnException(Handle<Object> exception, bool uncaught) {
-  HandleScope scope(isolate_);
-
-  // Bail out based on state or if there is no listener for this event
-  if (is_entered()) return;
-  if (!EventActive()) return;
+  if (is_entered() || ignore_events()) return;
 
+  HandleScope scope(isolate_);
   Handle<Object> promise = GetPromiseForUncaughtException();
   uncaught |= !promise->IsUndefined();
 
   // Bail out if exception breaks are not active
   if (uncaught) {
     // Uncaught exceptions are reported by either flags.
-    if (!(break_on_uncaught_exception() || break_on_exception())) return;
+    if (!(break_on_uncaught_exception_ || break_on_exception_)) return;
   } else {
     // Caught exceptions are reported is activated.
-    if (!break_on_exception()) return;
+    if (!break_on_exception_) return;
   }
 
   // Enter the debugger.
@@ -2643,14 +2619,12 @@ void Debug::OnException(Handle<Object> exception, bool uncaught) {
 
 void Debug::OnDebugBreak(Handle<Object> break_points_hit,
                             bool auto_continue) {
-  HandleScope scope(isolate_);
-
   // Debugger has already been entered by caller.
   ASSERT(isolate_->context() == *debug_context());
-
   // Bail out if there is no listener for this event
-  if (!EventActive()) return;
+  if (ignore_events()) return;
 
+  HandleScope scope(isolate_);
   // Create the event data object.
   Handle<Object> event_data;
   // Bail out and don't call debugger if exception.
@@ -2664,12 +2638,9 @@ void Debug::OnDebugBreak(Handle<Object> break_points_hit,
 
 
 void Debug::OnBeforeCompile(Handle<Script> script) {
-  HandleScope scope(isolate_);
-
-  // Bail out based on state or if there is no listener for this event
-  if (is_entered()) return;
-  if (!EventActive()) return;
+  if (is_entered() || ignore_events()) return;
 
+  HandleScope scope(isolate_);
   // Enter the debugger.
   EnterDebugger debugger(isolate_);
   if (debugger.FailedToEnter()) return;
@@ -2688,15 +2659,14 @@ void Debug::OnBeforeCompile(Handle<Script> script) {
 
 // Handle debugger actions when a new script is compiled.
 void Debug::OnAfterCompile(Handle<Script> script,
-                              AfterCompileFlags after_compile_flags) {
-  HandleScope scope(isolate_);
-
+                           AfterCompileFlags after_compile_flags) {
   // Add the newly compiled script to the script cache.
-  AddScriptToScriptCache(script);
+  if (script_cache_ != NULL) script_cache_->Add(script);
 
   // No more to do if not debugging.
-  if (!EventActive()) return;
+  if (is_entered() || ignore_events()) return;
 
+  HandleScope scope(isolate_);
   // Store whether in debugger before entering debugger.
   bool in_debugger = is_entered();
 
@@ -2746,12 +2716,9 @@ void Debug::OnAfterCompile(Handle<Script> script,
 
 
 void Debug::OnScriptCollected(int id) {
-  HandleScope scope(isolate_);
-
-  // No more to do if not debugging.
-  if (is_entered()) return;
-  if (!EventActive()) return;
+  if (is_entered() || ignore_events()) return;
 
+  HandleScope scope(isolate_);
   // Enter the debugger.
   EnterDebugger debugger(isolate_);
   if (debugger.FailedToEnter()) return;
@@ -2773,9 +2740,6 @@ void Debug::ProcessDebugEvent(v8::DebugEvent event,
                               bool auto_continue) {
   HandleScope scope(isolate_);
 
-  // Clear any pending debug break if this is a real break.
-  if (!auto_continue) thread_local_.has_pending_interrupt_ = false;
-
   // Create the execution state.
   Handle<Object> exec_state;
   // Bail out and don't call debugger if exception.
@@ -3090,7 +3054,7 @@ MaybeHandle<Object> Debug::Call(Handle<JSFunction> fun, Handle<Object> data) {
 }
 
 
-void Debug::DebugBreakHelper() {
+void Debug::HandleDebugBreak() {
   // Ignore debug break during bootstrapping.
   if (isolate_->bootstrapper()->IsActive()) return;
   // Just continue if breaks are disabled.
@@ -3156,9 +3120,9 @@ EnterDebugger::EnterDebugger(Isolate* isolate)
   // Create the new break info. If there is no JavaScript frames there is no
   // break frame id.
   JavaScriptFrameIterator it(isolate_);
-  has_js_frames_ = !it.done();
-  debug->thread_local_.break_frame_id_ = has_js_frames_ ? it.frame()->id()
-                                                        : StackFrame::NO_ID;
+  bool has_js_frames = !it.done();
+  debug->thread_local_.break_frame_id_ = has_js_frames ? it.frame()->id()
+                                                       : StackFrame::NO_ID;
   debug->SetNextBreakId();
 
   debug->UpdateState();
@@ -3182,9 +3146,7 @@ EnterDebugger::~EnterDebugger() {
     // 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 (!isolate_->has_pending_exception()) {
-      debug->ClearMirrorCache();
-    }
+    if (!isolate_->has_pending_exception()) debug->ClearMirrorCache();
 
     // If there are commands in the queue when leaving the debugger request
     // that these commands are processed.
@@ -3364,10 +3326,6 @@ CommandMessage::CommandMessage(const Vector<uint16_t>& text,
 }
 
 
-CommandMessage::~CommandMessage() {
-}
-
-
 void CommandMessage::Dispose() {
   text_.Dispose();
   delete client_data_;
@@ -3388,10 +3346,7 @@ CommandMessageQueue::CommandMessageQueue(int size) : start_(0), end_(0),
 
 
 CommandMessageQueue::~CommandMessageQueue() {
-  while (!IsEmpty()) {
-    CommandMessage m = Get();
-    m.Dispose();
-  }
+  while (!IsEmpty()) Get().Dispose();
   DeleteArray(messages_);
 }