Issue 2399 part 2: In debugger allow modifying local variable values

Review URL: https://codereview.chromium.org/11412310

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

src/frames.cc

@@ -692,6 +692,11 @@ void OptimizedFrame::Iterate(ObjectVisitor* v) const {
 }
 
 
+void JavaScriptFrame::SetParameterValue(int index, Object* value) const {
+  Memory::Object_at(GetParameterSlot(index)) = value;
+}
+
+
 bool JavaScriptFrame::IsConstructor() const {
   Address fp = caller_fp();
   if (has_adapted_arguments()) {

src/frames.h

@@ -500,6 +500,9 @@ class JavaScriptFrame: public StandardFrame {
     return GetNumberOfIncomingArguments();
   }
 
+  // Debugger access.
+  void SetParameterValue(int index, Object* value) const;
+
   // Check if this frame is a constructor frame invoked through 'new'.
   bool IsConstructor() const;
 

src/runtime.cc

@@ -10798,6 +10798,92 @@ static Handle<JSObject> MaterializeLocalScope(
 }
 
 
+// Set the context local variable value.
+static bool SetContextLocalValue(Isolate* isolate,
+                                 Handle<ScopeInfo> scope_info,
+                                 Handle<Context> context,
+                                 Handle<String> variable_name,
+                                 Handle<Object> new_value) {
+  for (int i = 0; i < scope_info->ContextLocalCount(); i++) {
+    Handle<String> next_name(scope_info->ContextLocalName(i));
+    if (variable_name->Equals(*next_name)) {
+      VariableMode mode;
+      InitializationFlag init_flag;
+      int context_index =
+          scope_info->ContextSlotIndex(*next_name, &mode, &init_flag);
+      context->set(context_index, *new_value);
+      return true;
+    }
+  }
+
+  return false;
+}
+
+
+static bool SetLocalVariableValue(Isolate* isolate,
+                                  JavaScriptFrame* frame,
+                                  int inlined_jsframe_index,
+                                  Handle<String> variable_name,
+                                  Handle<Object> new_value) {
+  if (inlined_jsframe_index != 0 || frame->is_optimized()) {
+    // Optimized frames are not supported.
+    return false;
+  }
+
+  Handle<JSFunction> function(JSFunction::cast(frame->function()));
+  Handle<SharedFunctionInfo> shared(function->shared());
+  Handle<ScopeInfo> scope_info(shared->scope_info());
+
+  // Parameters.
+  for (int i = 0; i < scope_info->ParameterCount(); ++i) {
+    if (scope_info->ParameterName(i)->Equals(*variable_name)) {
+      frame->SetParameterValue(i, *new_value);
+      return true;
+    }
+  }
+
+  // Stack locals.
+  for (int i = 0; i < scope_info->StackLocalCount(); ++i) {
+    if (scope_info->StackLocalName(i)->Equals(*variable_name)) {
+      frame->SetExpression(i, *new_value);
+      return true;
+    }
+  }
+
+  if (scope_info->HasContext()) {
+    // Context locals.
+    Handle<Context> frame_context(Context::cast(frame->context()));
+    Handle<Context> function_context(frame_context->declaration_context());
+    if (SetContextLocalValue(
+        isolate, scope_info, function_context, variable_name, new_value)) {
+      return true;
+    }
+
+    // Function context extension. These are variables introduced by eval.
+    if (function_context->closure() == *function) {
+      if (function_context->has_extension() &&
+          !function_context->IsNativeContext()) {
+        Handle<JSObject> ext(JSObject::cast(function_context->extension()));
+
+        if (ext->HasProperty(*variable_name)) {
+          // We don't expect this to do anything except replacing
+          // property value.
+          SetProperty(isolate,
+                      ext,
+                      variable_name,
+                      new_value,
+                      NONE,
+                      kNonStrictMode);
+          return true;
+        }
+      }
+    }
+  }
+
+  return false;
+}
+
+
 // Create a plain JSObject which materializes the closure content for the
 // context.
 static Handle<JSObject> MaterializeClosure(Isolate* isolate,
@@ -10858,19 +10944,9 @@ static bool SetClosureVariableValue(Isolate* isolate,
   Handle<ScopeInfo> scope_info(shared->scope_info());
 
   // Context locals to the context extension.
-  for (int i = 0; i < scope_info->ContextLocalCount(); i++) {
-    Handle<String> next_name(scope_info->ContextLocalName(i));
-    if (variable_name->Equals(*next_name)) {
-      VariableMode mode;
-      InitializationFlag init_flag;
-      int context_index =
-          scope_info->ContextSlotIndex(*next_name, &mode, &init_flag);
-      if (context_index < 0) {
-        return false;
-      }
-      context->set(context_index, *new_value);
-      return true;
-    }
+  if (SetContextLocalValue(
+          isolate, scope_info, context, variable_name, new_value)) {
+    return true;
   }
 
   // Properties from the function context extension. This will
@@ -10915,6 +10991,20 @@ static Handle<JSObject> MaterializeCatchScope(Isolate* isolate,
 }
 
 
+static bool SetCatchVariableValue(Isolate* isolate,
+                                  Handle<Context> context,
+                                  Handle<String> variable_name,
+                                  Handle<Object> new_value) {
+  ASSERT(context->IsCatchContext());
+  Handle<String> name(String::cast(context->extension()));
+  if (!name->Equals(*variable_name)) {
+    return false;
+  }
+  context->set(Context::THROWN_OBJECT_INDEX, *new_value);
+  return true;
+}
+
+
 // Create a plain JSObject which materializes the block scope for the specified
 // block context.
 static Handle<JSObject> MaterializeBlockScope(
@@ -11180,13 +11270,13 @@ class ScopeIterator {
       case ScopeIterator::ScopeTypeGlobal:
         break;
       case ScopeIterator::ScopeTypeLocal:
-        // TODO(2399): implement.
-        break;
+        return SetLocalVariableValue(isolate_, frame_, inlined_jsframe_index_,
+            variable_name, new_value);
       case ScopeIterator::ScopeTypeWith:
         break;
       case ScopeIterator::ScopeTypeCatch:
-        // TODO(2399): implement.
-        break;
+        return SetCatchVariableValue(isolate_, CurrentContext(),
+            variable_name, new_value);
       case ScopeIterator::ScopeTypeClosure:
         return SetClosureVariableValue(isolate_, CurrentContext(),
             variable_name, new_value);

test/mjsunit/debug-set-variable-value.js

@@ -34,8 +34,10 @@ var Debug = debug.Debug;
 // A variable 'variable_name' must be initialized before debugger statement
 // and returned after the statement. The test will alter variable value when
 // on debugger statement and check that returned value reflects the change.
-function RunPauseTest(scope_number, variable_name, expected_new_value, fun) {
-  var old_value = fun();
+function RunPauseTest(scope_number, expected_old_result, variable_name,
+    new_value, expected_new_result, fun) {
+  var actual_old_result = fun();
+  assertEquals(expected_old_result, actual_old_result);
 
   var listener_delegate;
   var listener_called = false;
@@ -43,7 +45,7 @@ function RunPauseTest(scope_number, variable_name, expected_new_value, fun) {
 
   function listener_delegate(exec_state) {
     var scope = exec_state.frame(0).scope(scope_number);
-    scope.setVariableValue(variable_name, expected_new_value);
+    scope.setVariableValue(variable_name, new_value);
   }
 
   function listener(event, exec_state, event_data, data) {
@@ -62,102 +64,201 @@ function RunPauseTest(scope_number, variable_name, expected_new_value, fun) {
 
   var actual_new_value;
   try {
-    actual_new_value = fun();
+    actual_new_result = fun();
   } finally {
     Debug.setListener(null);
   }
 
   if (exception != null) {
-   assertUnreachable("Exception: " + exception);
+   assertUnreachable("Exception in listener\n" + exception.stack);
   }
   assertTrue(listener_called);
 
-  assertTrue(old_value != actual_new_value);
-  assertTrue(expected_new_value == actual_new_value);
+  assertEquals(expected_new_result, actual_new_result);
 }
 
 // Accepts a closure 'fun' that returns a variable from it's outer scope.
 // The test changes the value of variable via the handle to function and checks
 // that the return value changed accordingly.
-function RunClosureTest(scope_number, variable_name, expected_new_value, fun) {
-  var old_value = fun();
+function RunClosureTest(scope_number, expected_old_result, variable_name,
+    new_value, expected_new_result, fun) {
+  var actual_old_result = fun();
+  assertEquals(expected_old_result, actual_old_result);
 
   var fun_mirror = Debug.MakeMirror(fun);
 
   var scope = fun_mirror.scope(scope_number);
-  scope.setVariableValue(variable_name, expected_new_value);
+  scope.setVariableValue(variable_name, new_value);
 
-  var actual_new_value = fun();
+  var actual_new_result = fun();
 
-  assertTrue(old_value != actual_new_value);
-  assertTrue(expected_new_value == actual_new_value);
+  assertEquals(expected_new_result, actual_new_result);
 }
 
-// Test changing variable value when in pause
-RunPauseTest(1, 'v1', 5, (function Factory() {
-  var v1 = 'cat';
+
+function ClosureTestCase(scope_index, old_result, variable_name, new_value,
+    new_result, success_expected, factory) {
+  this.scope_index_ = scope_index;
+  this.old_result_ = old_result;
+  this.variable_name_ = variable_name;
+  this.new_value_ = new_value;
+  this.new_result_ = new_result;
+  this.success_expected_ = success_expected;
+  this.factory_ = factory;
+}
+
+ClosureTestCase.prototype.run_pause_test = function() {
+  var th = this;
+  var fun = this.factory_(true);
+  this.run_and_catch_(function() {
+    RunPauseTest(th.scope_index_ + 1, th.old_result_, th.variable_name_,
+        th.new_value_, th.new_result_, fun);
+  });
+}
+
+ClosureTestCase.prototype.run_closure_test = function() {
+  var th = this;
+  var fun = this.factory_(false);
+  this.run_and_catch_(function() {
+    RunClosureTest(th.scope_index_, th.old_result_, th.variable_name_,
+        th.new_value_, th.new_result_, fun);
+  });
+}
+
+ClosureTestCase.prototype.run_and_catch_ = function(runnable) {
+  if (this.success_expected_) {
+    runnable();
+  } else {
+    assertThrows(runnable);
+  }
+}
+
+
+// Test scopes visible from closures.
+
+var closure_test_cases = [
+  new ClosureTestCase(0, 'cat', 'v1', 5, 5, true,
+      function Factory(debug_stop) {
+    var v1 = 'cat';
+    return function() {
+      if (debug_stop) debugger;
+      return v1;
+    }
+  }),
+
+  new ClosureTestCase(0, 4, 't', 7, 9, true, function Factory(debug_stop) {
+    var t = 2;
+    var r = eval("t");
+    return function() {
+      if (debug_stop) debugger;
+      return r + t;
+    }
+  }),
+
+  new ClosureTestCase(0, 6, 't', 10, 13, true, function Factory(debug_stop) {
+    var t = 2;
+    var r = eval("t = 3");
+    return function() {
+      if (debug_stop) debugger;
+      return r + t;
+    }
+  }),
+
+  new ClosureTestCase(0, 17, 's', 'Bird', 'Bird', true,
+      function Factory(debug_stop) {
+    eval("var s = 17");
+    return function() {
+      if (debug_stop) debugger;
+      return s;
+    }
+  }),
+
+  new ClosureTestCase(2, 'capybara', 'foo', 77, 77, true,
+      function Factory(debug_stop) {
+    var foo = "capybara";
+    return (function() {
+      var bar = "fish";
+      try {
+        throw {name: "test exception"};
+      } catch (e) {
+        return function() {
+          if (debug_stop) debugger;
+          bar = "beast";
+          return foo;
+        }
+      }
+    })();
+  }),
+
+  new ClosureTestCase(0, 'AlphaBeta', 'eee', 5, '5Beta', true,
+      function Factory(debug_stop) {
+    var foo = "Beta";
+    return (function() {
+      var bar = "fish";
+      try {
+        throw "Alpha";
+      } catch (eee) {
+        return function() {
+          if (debug_stop) debugger;
+          return eee + foo;
+        }
+      }
+    })();
+  })
+];
+
+for (var i = 0; i < closure_test_cases.length; i++) {
+  closure_test_cases[i].run_pause_test();
+}
+
+for (var i = 0; i < closure_test_cases.length; i++) {
+  closure_test_cases[i].run_closure_test();
+}
+
+
+// Test local scope.
+
+RunPauseTest(0, 'HelloYou', 'u', 'We', 'HelloWe', (function Factory() {
   return function() {
+    var u = "You";
+    var v = "Hello";
     debugger;
-    return v1;
+    return v + u;
   }
 })());
 
-RunPauseTest(1, 'v2', 11, (function Factory(v2) {
-  return function() {
+RunPauseTest(0, 'Helloworld', 'p', 'GoodBye', 'HelloGoodBye',
+    (function Factory() {
+  function H(p) {
+    var v = "Hello";
     debugger;
-    return v2;
+    return v + p;
+  }
+  return function() {
+    return H("world");
   }
-})('dog'));
-
-RunPauseTest(3, 'foo', 77, (function Factory() {
-  var foo = "capybara";
-  return (function() {
-    var bar = "fish";
-    try {
-      throw {name: "test exception"};
-    } catch (e) {
-      return function() {
-        debugger;
-        bar = "beast";
-        return foo;
-      }
-    }
-  })();
 })());
 
-
-
-// Test changing variable value in closure by handle
-RunClosureTest(0, 'v1', 5, (function Factory() {
-  var v1 = 'cat';
+RunPauseTest(0, 'mouse', 'v1', 'dog', 'dog', (function Factory() {
   return function() {
+    var v1 = 'cat';
+    eval("v1 = 'mouse'");
+    debugger;
     return v1;
   }
 })());
 
-RunClosureTest(0, 'v2', 11, (function Factory(v2) {
+RunPauseTest(0, 'mouse', 'v1', 'dog', 'dog', (function Factory() {
   return function() {
-    return v2;
+    eval("var v1 = 'mouse'");
+    debugger;
+    return v1;
   }
-})('dog'));
-
-RunClosureTest(2, 'foo', 77, (function Factory() {
-  var foo = "capybara";
-  return (function() {
-    var bar = "fish";
-    try {
-      throw {name: "test exception"};
-    } catch (e) {
-      return function() {
-        bar = "beast";
-        return foo;
-      }
-    }
-  })();
 })());
 
 
 // Test value description protocol JSON
+
 assertEquals(true, Debug.TestApi.CommandProcessorResolveValue({value: true}));
 
 assertSame(null, Debug.TestApi.CommandProcessorResolveValue({type: "null"}));
@@ -173,4 +274,3 @@ assertSame(Number, Debug.TestApi.CommandProcessorResolveValue(
     {handle: Debug.MakeMirror(Number).handle()}));
 assertSame(RunClosureTest, Debug.TestApi.CommandProcessorResolveValue(
     {handle: Debug.MakeMirror(RunClosureTest).handle()}));
-