Support for precise stepping in functions compiled before debugging was started (step 2)

This change will ensure that full code with debug break slots is compiled and activated for all functions which already have activation frames.

This additional handling is only for functions which have activations on the stack, and that activation is of the full code compiled without debug break slots. In that case the full code is recompiled with debug break slots. It is ensured that the full code is compiled generating the exact same instructions - except for the additional debug break slots - as before. The return address on the stack is then patched to continue execution in the new code.

Also fixed SortedListBSearch to actually use the passed comparision function.

R=svenpanne@chromium.org, kmillikin@chromium.org

BUG=
TEST=

Review URL: http://codereview.chromium.org//8050010

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

src/compiler.cc

@@ -58,7 +58,6 @@ CompilationInfo::CompilationInfo(Handle<Script> script)
       script_(script),
       extension_(NULL),
       pre_parse_data_(NULL),
-      supports_deoptimization_(false),
       osr_ast_id_(AstNode::kNoNumber) {
   Initialize(NONOPT);
 }
@@ -73,7 +72,6 @@ CompilationInfo::CompilationInfo(Handle<SharedFunctionInfo> shared_info)
       script_(Handle<Script>(Script::cast(shared_info->script()))),
       extension_(NULL),
       pre_parse_data_(NULL),
-      supports_deoptimization_(false),
       osr_ast_id_(AstNode::kNoNumber) {
   Initialize(BASE);
 }
@@ -89,7 +87,6 @@ CompilationInfo::CompilationInfo(Handle<JSFunction> closure)
       script_(Handle<Script>(Script::cast(shared_info_->script()))),
       extension_(NULL),
       pre_parse_data_(NULL),
-      supports_deoptimization_(false),
       osr_ast_id_(AstNode::kNoNumber) {
   Initialize(BASE);
 }
@@ -308,9 +305,9 @@ static bool MakeCrankshaftCode(CompilationInfo* info) {
 
 
 static bool GenerateCode(CompilationInfo* info) {
-  return V8::UseCrankshaft() ?
-    MakeCrankshaftCode(info) :
-    FullCodeGenerator::MakeCode(info);
+  return info->IsCompilingForDebugging() || !V8::UseCrankshaft() ?
+      FullCodeGenerator::MakeCode(info) :
+      MakeCrankshaftCode(info);
 }
 
 

src/compiler.h

@@ -120,6 +120,19 @@ class CompilationInfo BASE_EMBEDDED {
     ASSERT(IsOptimizing());
     osr_ast_id_ = osr_ast_id;
   }
+  void MarkCompilingForDebugging(Handle<Code> current_code) {
+    ASSERT(mode_ != OPTIMIZE);
+    ASSERT(current_code->kind() == Code::FUNCTION);
+    flags_ |= IsCompilingForDebugging::encode(true);
+    if (current_code->is_compiled_optimizable()) {
+      EnableDeoptimizationSupport();
+    } else {
+      mode_ = CompilationInfo::NONOPT;
+    }
+  }
+  bool IsCompilingForDebugging() {
+    return IsCompilingForDebugging::decode(flags_);
+  }
 
   bool has_global_object() const {
     return !closure().is_null() && (closure()->context()->global() != NULL);
@@ -139,10 +152,12 @@ class CompilationInfo BASE_EMBEDDED {
   void DisableOptimization();
 
   // Deoptimization support.
-  bool HasDeoptimizationSupport() const { return supports_deoptimization_; }
+  bool HasDeoptimizationSupport() const {
+    return SupportsDeoptimization::decode(flags_);
+  }
   void EnableDeoptimizationSupport() {
     ASSERT(IsOptimizable());
-    supports_deoptimization_ = true;
+    flags_ |= SupportsDeoptimization::encode(true);
   }
 
   // Determine whether or not we can adaptively optimize.
@@ -203,6 +218,11 @@ class CompilationInfo BASE_EMBEDDED {
   class IsNativesSyntaxAllowed: public BitField<bool, 5, 1> {};
   // Is this a function from our natives.
   class IsNative: public BitField<bool, 6, 1> {};
+  // Is this code being compiled with support for deoptimization..
+  class SupportsDeoptimization: public BitField<bool, 7, 1> {};
+  // If compiling for debugging produce just full code matching the
+  // initial mode setting.
+  class IsCompilingForDebugging: public BitField<bool, 8, 1> {};
 
 
   unsigned flags_;
@@ -231,7 +251,6 @@ class CompilationInfo BASE_EMBEDDED {
 
   // Compilation mode flag and whether deoptimization is allowed.
   Mode mode_;
-  bool supports_deoptimization_;
   int osr_ast_id_;
 
   DISALLOW_COPY_AND_ASSIGN(CompilationInfo);

src/debug.cc

@@ -1727,32 +1727,75 @@ void Debug::ClearStepNext() {
 }
 
 
+// Helper function to compile full code for debugging. This code will
+// have debug break slots and deoptimization
+// information. Deoptimization information is required in case that an
+// optimized version of this function is still activated on the
+// stack. It will also make sure that the full code is compiled with
+// the same flags as the previous version - that is flags which can
+// change the code generated. The current method of mapping from
+// already compiled full code without debug break slots to full code
+// with debug break slots depends on the generated code is otherwise
+// exactly the same.
+static bool CompileFullCodeForDebugging(Handle<SharedFunctionInfo> shared,
+                                        Handle<Code> current_code) {
+  ASSERT(!current_code->has_debug_break_slots());
+
+  CompilationInfo info(shared);
+  info.MarkCompilingForDebugging(current_code);
+  ASSERT(!info.shared_info()->is_compiled());
+  ASSERT(!info.isolate()->has_pending_exception());
+
+  // Use compile lazy which will end up compiling the full code in the
+  // configuration configured above.
+  bool result = Compiler::CompileLazy(&info);
+  ASSERT(result != Isolate::Current()->has_pending_exception());
+  info.isolate()->clear_pending_exception();
+#if DEBUG
+  if (result) {
+    Handle<Code> new_code(shared->code());
+    ASSERT(new_code->has_debug_break_slots());
+    ASSERT(current_code->is_compiled_optimizable() ==
+           new_code->is_compiled_optimizable());
+    ASSERT(current_code->instruction_size() <= new_code->instruction_size());
+  }
+#endif
+  return result;
+}
+
+
 void Debug::PrepareForBreakPoints() {
   // If preparing for the first break point make sure to deoptimize all
   // functions as debugging does not work with optimized code.
   if (!has_break_points_) {
     Deoptimizer::DeoptimizeAll();
 
-    // We are going to iterate heap to find all functions without
-    // debug break slots.
-    isolate_->heap()->CollectAllGarbage(Heap::kMakeHeapIterableMask);
+    Handle<Code> lazy_compile =
+        Handle<Code>(isolate_->builtins()->builtin(Builtins::kLazyCompile));
 
+    // Keep the list of activated functions in a handlified list as it
+    // is used both in GC and non-GC code.
+    List<Handle<JSFunction> > active_functions(100);
+
+    {
+      // Ensure no GC in this scope as we are comparing raw pointer
+      // values and performing a heap iteration.
       AssertNoAllocation no_allocation;
-    Builtins* builtins = isolate_->builtins();
-    Code* lazy_compile = builtins->builtin(Builtins::kLazyCompile);
 
       // Find all non-optimized code functions with activation frames on
       // the stack.
-    List<JSFunction*> active_functions(100);
       for (JavaScriptFrameIterator it(isolate_); !it.done(); it.Advance()) {
         JavaScriptFrame* frame = it.frame();
         if (frame->function()->IsJSFunction()) {
           JSFunction* function = JSFunction::cast(frame->function());
-        if (function->code()->kind() == Code::FUNCTION)
-          active_functions.Add(function);
+          if (function->code()->kind() == Code::FUNCTION &&
+              !function->code()->has_debug_break_slots())
+            active_functions.Add(Handle<JSFunction>(function));
         }
       }
-    active_functions.Sort();
+      // Sort the functions on the object pointer value to prepare for
+      // the binary search below.
+      active_functions.Sort(HandleObjectPointerCompare<JSFunction>);
 
       // Scan the heap for all non-optimized functions which has no
       // debug break slots.
@@ -1766,15 +1809,121 @@ void Debug::PrepareForBreakPoints() {
               function->code()->kind() == Code::FUNCTION &&
               !function->code()->has_debug_break_slots()) {
             bool has_activation =
-              SortedListBSearch<JSFunction*>(active_functions, function) != -1;
+                SortedListBSearch<Handle<JSFunction> >(
+                    active_functions,
+                    Handle<JSFunction>(function),
+                    HandleObjectPointerCompare<JSFunction>) != -1;
             if (!has_activation) {
-            function->set_code(lazy_compile);
-            function->shared()->set_code(lazy_compile);
+              function->set_code(*lazy_compile);
+              function->shared()->set_code(*lazy_compile);
             }
           }
         }
       }
     }
+
+    // Now the non-GC scope is left, and the sorting of the functions
+    // in active_function is not ensured any more. The code below does
+    // not rely on it.
+
+    // Now recompile all functions with activation frames and and
+    // patch the return address to run in the new compiled code.
+    for (int i = 0; i < active_functions.length(); i++) {
+      Handle<JSFunction> function = active_functions[i];
+      Handle<SharedFunctionInfo> shared(function->shared());
+      // If recompilation is not possible just skip it.
+      if (shared->is_toplevel() ||
+          !shared->allows_lazy_compilation() ||
+          shared->code()->kind() == Code::BUILTIN) {
+        continue;
+      }
+
+      // Make sure that the shared full code is compiled with debug
+      // break slots.
+      Handle<Code> current_code(function->code());
+      if (shared->code()->has_debug_break_slots()) {
+        // if the code is already recompiled to have break slots skip
+        // recompilation.
+        ASSERT(!function->code()->has_debug_break_slots());
+      } else {
+        // Try to compile the full code with debug break slots. If it
+        // fails just keep the current code.
+        ASSERT(shared->code() == *current_code);
+        ZoneScope zone_scope(isolate_, DELETE_ON_EXIT);
+        shared->set_code(*lazy_compile);
+        bool prev_force_debugger_active =
+            isolate_->debugger()->force_debugger_active();
+        isolate_->debugger()->set_force_debugger_active(true);
+        CompileFullCodeForDebugging(shared, current_code);
+        isolate_->debugger()->set_force_debugger_active(
+            prev_force_debugger_active);
+        if (!shared->is_compiled()) {
+          shared->set_code(*current_code);
+          continue;
+        }
+      }
+      Handle<Code> new_code(shared->code());
+
+      // Find the function and patch return address.
+      for (JavaScriptFrameIterator it(isolate_); !it.done(); it.Advance()) {
+        JavaScriptFrame* frame = it.frame();
+        // If the current frame is for this function in its
+        // non-optimized form rewrite the return address to continue
+        // in the newly compiled full code with debug break slots.
+        if (frame->function()->IsJSFunction() &&
+            frame->function() == *function &&
+            frame->LookupCode()->kind() == Code::FUNCTION) {
+          int delta = frame->pc() - current_code->instruction_start();
+          int debug_break_slot_count = 0;
+          int mask = RelocInfo::ModeMask(RelocInfo::DEBUG_BREAK_SLOT);
+          for (RelocIterator it(*new_code, mask); !it.done(); it.next()) {
+            // Check if the pc in the new code with debug break
+            // slots is before this slot.
+            RelocInfo* info = it.rinfo();
+            int debug_break_slot_bytes =
+                debug_break_slot_count * Assembler::kDebugBreakSlotLength;
+            int new_delta =
+                info->pc() -
+                new_code->instruction_start() -
+                debug_break_slot_bytes;
+            if (new_delta > delta) {
+              break;
+            }
+
+            // Passed a debug break slot in the full code with debug
+            // break slots.
+            debug_break_slot_count++;
+          }
+          int debug_break_slot_bytes =
+              debug_break_slot_count * Assembler::kDebugBreakSlotLength;
+          if (FLAG_trace_deopt) {
+            PrintF("Replacing code %08" V8PRIxPTR " - %08" V8PRIxPTR " (%d) "
+                   "with %08" V8PRIxPTR " - %08" V8PRIxPTR " (%d) "
+                   "for debugging, "
+                   "changing pc from %08" V8PRIxPTR " to %08" V8PRIxPTR "\n",
+                   reinterpret_cast<intptr_t>(
+                       current_code->instruction_start()),
+                   reinterpret_cast<intptr_t>(
+                       current_code->instruction_start()) +
+                       current_code->instruction_size(),
+                   current_code->instruction_size(),
+                   reinterpret_cast<intptr_t>(new_code->instruction_start()),
+                   reinterpret_cast<intptr_t>(new_code->instruction_start()) +
+                       new_code->instruction_size(),
+                   new_code->instruction_size(),
+                   reinterpret_cast<intptr_t>(frame->pc()),
+                   reinterpret_cast<intptr_t>(new_code->instruction_start()) +
+                       delta + debug_break_slot_bytes);
+          }
+
+          // Patch the return address to return into the code with
+          // debug break slots.
+          frame->set_pc(
+              new_code->instruction_start() + delta + debug_break_slot_bytes);
+        }
+      }
+    }
+  }
 }
 
 
@@ -2841,7 +2990,9 @@ void Debugger::EnqueueDebugCommand(v8::Debug::ClientData* client_data) {
 bool Debugger::IsDebuggerActive() {
   ScopedLock with(debugger_access_);
 
-  return message_handler_ != NULL || !event_listener_.is_null();
+  return message_handler_ != NULL ||
+      !event_listener_.is_null() ||
+      force_debugger_active_;
 }
 
 

src/debug.h

@@ -809,11 +809,15 @@ class Debugger {
   }
 
   void set_compiling_natives(bool compiling_natives) {
-    Debugger::compiling_natives_ = compiling_natives;
+    compiling_natives_ = compiling_natives;
   }
   bool compiling_natives() const { return compiling_natives_; }
   void set_loading_debugger(bool v) { is_loading_debugger_ = v; }
   bool is_loading_debugger() const { return is_loading_debugger_; }
+  void set_force_debugger_active(bool force_debugger_active) {
+    force_debugger_active_ = force_debugger_active;
+  }
+  bool force_debugger_active() const { return force_debugger_active_; }
 
   bool IsDebuggerActive();
 
@@ -839,6 +843,7 @@ class Debugger {
   bool compiling_natives_;  // Are we compiling natives?
   bool is_loading_debugger_;  // Are we loading the debugger?
   bool never_unload_debugger_;  // Can we unload the debugger?
+  bool force_debugger_active_;  // Activate debugger without event listeners.
   v8::Debug::MessageHandler2 message_handler_;
   bool debugger_unload_pending_;  // Was message handler cleared?
   v8::Debug::HostDispatchHandler host_dispatch_handler_;

src/full-codegen.cc

@@ -289,11 +289,12 @@ bool FullCodeGenerator::MakeCode(CompilationInfo* info) {
 #ifdef ENABLE_DEBUGGER_SUPPORT
   code->set_has_debug_break_slots(
       info->isolate()->debugger()->IsDebuggerActive());
+  code->set_compiled_optimizable(info->IsOptimizable());
 #endif  // ENABLE_DEBUGGER_SUPPORT
   code->set_allow_osr_at_loop_nesting_level(0);
   code->set_stack_check_table_offset(table_offset);
   CodeGenerator::PrintCode(code, info);
-  info->SetCode(code);  // may be an empty handle.
+  info->SetCode(code);  // May be an empty handle.
 #ifdef ENABLE_GDB_JIT_INTERFACE
   if (FLAG_gdbjit && !code.is_null()) {
     GDBJITLineInfo* lineinfo =

src/list-inl.h

@@ -216,11 +216,11 @@ int SortedListBSearch(
     int mid = (low + high) / 2;
     T mid_elem = list[mid];
 
-    if (mid_elem > elem) {
+    if (cmp(&mid_elem, &elem) > 0) {
       high = mid - 1;
       continue;
     }
-    if (mid_elem < elem) {
+    if (cmp(&mid_elem, &elem) < 0) {
       low = mid + 1;
       continue;
     }

src/objects-inl.h

@@ -2990,6 +2990,21 @@ void Code::set_has_debug_break_slots(bool value) {
 }
 
 
+bool Code::is_compiled_optimizable() {
+  ASSERT(kind() == FUNCTION);
+  byte flags = READ_BYTE_FIELD(this, kFullCodeFlags);
+  return FullCodeFlagsIsCompiledOptimizable::decode(flags);
+}
+
+
+void Code::set_compiled_optimizable(bool value) {
+  ASSERT(kind() == FUNCTION);
+  byte flags = READ_BYTE_FIELD(this, kFullCodeFlags);
+  flags = FullCodeFlagsIsCompiledOptimizable::update(flags, value);
+  WRITE_BYTE_FIELD(this, kFullCodeFlags, flags);
+}
+
+
 int Code::allow_osr_at_loop_nesting_level() {
   ASSERT(kind() == FUNCTION);
   return READ_BYTE_FIELD(this, kAllowOSRAtLoopNestingLevelOffset);

src/objects.h

@@ -3676,6 +3676,11 @@ class Code: public HeapObject {
   inline bool has_debug_break_slots();
   inline void set_has_debug_break_slots(bool value);
 
+  // [compiled_with_optimizing]: For FUNCTION kind, tells if it has
+  // been compiled with IsOptimizing set to true.
+  inline bool is_compiled_optimizable();
+  inline void set_compiled_optimizable(bool value);
+
   // [allow_osr_at_loop_nesting_level]: For FUNCTION kind, tells for
   // how long the function has been marked for OSR and therefore which
   // level of loop nesting we are willing to do on-stack replacement
@@ -3871,6 +3876,7 @@ class Code: public HeapObject {
   class FullCodeFlagsHasDeoptimizationSupportField:
       public BitField<bool, 0, 1> {};  // NOLINT
   class FullCodeFlagsHasDebugBreakSlotsField: public BitField<bool, 1, 1> {};
+  class FullCodeFlagsIsCompiledOptimizable: public BitField<bool, 2, 1> {};
 
   static const int kBinaryOpReturnTypeOffset = kBinaryOpTypeOffset + 1;
 

src/utils.h

@@ -143,6 +143,16 @@ static int PointerValueCompare(const T* a, const T* b) {
 }
 
 
+// Compare function to compare the object pointer value of two
+// handlified objects. The handles are passed as pointers to the
+// handles.
+template<typename T> class Handle;  // Forward declaration.
+template <typename T>
+static int HandleObjectPointerCompare(const Handle<T>* a, const Handle<T>* b) {
+  return Compare<T*>(*(*a), *(*b));
+}
+
+
 // Returns the smallest power of two which is >= x. If you pass in a
 // number that is already a power of two, it is returned as is.
 // Implementation is from "Hacker's Delight" by Henry S. Warren, Jr.,

test/mjsunit/debug-step-3.js

@@ -0,0 +1,93 @@
+// Copyright 2011 the V8 project authors. All rights reserved.
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+//       notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+//       copyright notice, this list of conditions and the following
+//       disclaimer in the documentation and/or other materials provided
+//       with the distribution.
+//     * Neither the name of Google Inc. nor the names of its
+//       contributors may be used to endorse or promote products derived
+//       from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+// Flags: --expose-debug-as debug
+
+// This test tests that full code compiled without debug break slots
+// is recompiled with debug break slots when debugging is started.
+
+// Get the Debug object exposed from the debug context global object.
+Debug = debug.Debug
+
+var bp;
+var done = false;
+var step_count = 0;
+var set_bp = false
+
+// Debug event listener which steps until the global variable done is true.
+function listener(event, exec_state, event_data, data) {
+  if (event == Debug.DebugEvent.Break) {
+    if (!done) exec_state.prepareStep(Debug.StepAction.StepNext);
+    step_count++;
+  }
+};
+
+// Set the global variables state to prpare the stepping test.
+function prepare_step_test() {
+  done = false;
+  step_count = 0;
+}
+
+// Test function to step through.
+function f() {
+  var a = 0;
+  if (set_bp) { bp = Debug.setBreakPoint(f, 3); }
+  var i = 1;
+  var j = 2;
+  done = true;
+};
+
+prepare_step_test();
+f();
+
+// Add the debug event listener.
+Debug.setListener(listener);
+
+// Make f set a breakpoint with an activation on the stack.
+prepare_step_test();
+set_bp = true;
+f();
+assertEquals(4, step_count);
+Debug.clearBreakPoint(bp);
+
+// Set a breakpoint on the first var statement (line 1).
+set_bp = false;
+bp = Debug.setBreakPoint(f, 3);
+
+// Step through the function ensuring that the var statements are hit as well.
+prepare_step_test();
+f();
+assertEquals(4, step_count);
+
+// Clear the breakpoint and check that no stepping happens.
+Debug.clearBreakPoint(bp);
+prepare_step_test();
+f();
+assertEquals(0, step_count);
+
+// Get rid of the debug event listener.
+Debug.setListener(null);