Stack allocate lexical locals + hoist stack slots

src/runtime/runtime-debug.cc

Index: src/runtime/runtime-debug.cc
diff --git a/src/runtime/runtime-debug.cc b/src/runtime/runtime-debug.cc
index 736d33dadce30cdfcc9571eb4b50dc13d6ad3c73..763e7bd928d3d7609c3f75e4ba9906815b53de34 100644
--- a/src/runtime/runtime-debug.cc
+++ b/src/runtime/runtime-debug.cc
@@ -687,17 +687,16 @@ static bool ParameterIsShadowedByContextLocal(Handle<ScopeInfo> info,
 // frame.
 MUST_USE_RESULT
 static MaybeHandle<JSObject> MaterializeStackLocalsWithFrameInspector(
-    Isolate* isolate, Handle<JSObject> target, Handle<JSFunction> function,
+    Isolate* isolate, Handle<JSObject> target, Handle<ScopeInfo> scope_info,
     FrameInspector* frame_inspector) {
-  Handle<SharedFunctionInfo> shared(function->shared());
-  Handle<ScopeInfo> scope_info(shared->scope_info());
-
   // First fill all parameters.
   for (int i = 0; i < scope_info->ParameterCount(); ++i) {
     // Do not materialize the parameter if it is shadowed by a context local.
     Handle<String> name(scope_info->ParameterName(i));
     if (ParameterIsShadowedByContextLocal(scope_info, name)) continue;
 
+    DCHECK(frame_inspector != nullptr);
+
     HandleScope scope(isolate);
     Handle<Object> value(i < frame_inspector->GetParametersCount()
                              ? frame_inspector->GetParameter(i)
@@ -714,8 +713,12 @@ static MaybeHandle<JSObject> MaterializeStackLocalsWithFrameInspector(
   for (int i = 0; i < scope_info->StackLocalCount(); ++i) {
     if (scope_info->LocalIsSynthetic(i)) continue;
     Handle<String> name(scope_info->StackLocalName(i));
-    Handle<Object> value(frame_inspector->GetExpression(i), isolate);
-    if (value->IsTheHole()) continue;
+    Handle<Object> value(
+        frame_inspector->GetExpression(scope_info->StackLocalIndex(i)),
+        isolate);
+    if (value->IsTheHole()) {
+      value = isolate->factory()->undefined_value();

[rossberg, 2015/04/17 15:16:27]

Is this safe? Isn't the implication that you can now change an uninitialised
binding to initialised by assigning to it in the debugger? If so, I'm not sure
it's good to allow that, and it may potentially break some assumptions
elsewhere.

I'm not sure, I'd be careful with this change. If at all, can we do it in a
separate CL?

[Dmitry Lomov (no reviews), 2015/04/17 15:21:54]

If we do not do that, shadowing will not work correctly. 
One way to prevent 'uninitialized->initialized' is to disallow assignments to
holes later when I copy the the values back.

[rossberg, 2015/04/17 15:26:07]

I see. Yes, preventing the copying back sounds good to me.

[Dmitry Lomov (no reviews), 2015/04/20 11:53:20]

We actually already have the check (line 770)

+    }
 
     RETURN_ON_EXCEPTION(isolate, Runtime::SetObjectProperty(
                                      isolate, target, name, value, SLOPPY),
@@ -725,12 +728,21 @@ static MaybeHandle<JSObject> MaterializeStackLocalsWithFrameInspector(
   return target;
 }
 
+MUST_USE_RESULT
+static MaybeHandle<JSObject> MaterializeStackLocalsWithFrameInspector(
+    Isolate* isolate, Handle<JSObject> target, Handle<JSFunction> function,
+    FrameInspector* frame_inspector) {
+  Handle<SharedFunctionInfo> shared(function->shared());
+  Handle<ScopeInfo> scope_info(shared->scope_info());
+
+  return MaterializeStackLocalsWithFrameInspector(isolate, target, scope_info,
+                                                  frame_inspector);
+}
+
 
-static void UpdateStackLocalsFromMaterializedObject(Isolate* isolate,
-                                                    Handle<JSObject> target,
-                                                    Handle<JSFunction> function,
-                                                    JavaScriptFrame* frame,
-                                                    int inlined_jsframe_index) {
+static void UpdateStackLocalsFromMaterializedObject(
+    Isolate* isolate, Handle<JSObject> target, Handle<ScopeInfo> scope_info,
+    JavaScriptFrame* frame, int inlined_jsframe_index) {
   if (inlined_jsframe_index != 0 || frame->is_optimized()) {
     // Optimized frames are not supported.
     // TODO(yangguo): make sure all code deoptimized when debugger is active
@@ -738,9 +750,6 @@ static void UpdateStackLocalsFromMaterializedObject(Isolate* isolate,
     return;
   }
 
-  Handle<SharedFunctionInfo> shared(function->shared());
-  Handle<ScopeInfo> scope_info(shared->scope_info());
-
   // Parameters.
   for (int i = 0; i < scope_info->ParameterCount(); ++i) {
     // Shadowed parameters were not materialized.
@@ -757,12 +766,13 @@ static void UpdateStackLocalsFromMaterializedObject(Isolate* isolate,
   // Stack locals.
   for (int i = 0; i < scope_info->StackLocalCount(); ++i) {
     if (scope_info->LocalIsSynthetic(i)) continue;
-    if (frame->GetExpression(i)->IsTheHole()) continue;
+    int index = scope_info->StackLocalIndex(i);
+    if (frame->GetExpression(index)->IsTheHole()) continue;
     HandleScope scope(isolate);
     Handle<Object> value = Object::GetPropertyOrElement(
                                target, handle(scope_info->StackLocalName(i),
                                               isolate)).ToHandleChecked();
-    frame->SetExpression(i, *value);
+    frame->SetExpression(index, *value);
   }
 }
 
@@ -904,7 +914,7 @@ static bool SetLocalVariableValue(Isolate* isolate, JavaScriptFrame* frame,
   for (int i = 0; i < scope_info->StackLocalCount(); ++i) {
     HandleScope scope(isolate);
     if (String::Equals(handle(scope_info->StackLocalName(i)), variable_name)) {
-      frame->SetExpression(i, *new_value);
+      frame->SetExpression(scope_info->StackLocalIndex(i), *new_value);
       return true;
     }
   }
@@ -941,6 +951,30 @@ static bool SetLocalVariableValue(Isolate* isolate, JavaScriptFrame* frame,
 }
 
 
+static bool SetBlockVariableValue(Isolate* isolate,
+                                  Handle<Context> block_context,
+                                  Handle<ScopeInfo> scope_info,
+                                  JavaScriptFrame* frame,
+                                  Handle<String> variable_name,
+                                  Handle<Object> new_value) {
+  if (frame != nullptr) {
+    for (int i = 0; i < scope_info->StackLocalCount(); ++i) {
+      HandleScope scope(isolate);
+      if (String::Equals(handle(scope_info->StackLocalName(i)),
+                         variable_name)) {
+        frame->SetExpression(scope_info->StackLocalIndex(i), *new_value);
+        return true;
+      }
+    }
+  }
+  if (!block_context.is_null()) {
+    return SetContextLocalValue(block_context->GetIsolate(), scope_info,
+                                block_context, variable_name, new_value);
+  }
+  return false;
+}
+
+
 // Create a plain JSObject which materializes the closure content for the
 // context.
 MUST_USE_RESULT static MaybeHandle<JSObject> MaterializeClosure(
@@ -1021,17 +1055,6 @@ static bool SetClosureVariableValue(Isolate* isolate, Handle<Context> context,
 }
 
 
-static bool SetBlockContextVariableValue(Handle<Context> block_context,
-                                         Handle<String> variable_name,
-                                         Handle<Object> new_value) {
-  DCHECK(block_context->IsBlockContext());
-  Handle<ScopeInfo> scope_info(ScopeInfo::cast(block_context->extension()));
-
-  return SetContextLocalValue(block_context->GetIsolate(), scope_info,
-                              block_context, variable_name, new_value);
-}
-
-
 static bool SetScriptVariableValue(Handle<Context> context,
                                    Handle<String> variable_name,
                                    Handle<Object> new_value) {
@@ -1083,19 +1106,27 @@ static bool SetCatchVariableValue(Isolate* isolate, Handle<Context> context,
 // Create a plain JSObject which materializes the block scope for the specified
 // block context.
 MUST_USE_RESULT static MaybeHandle<JSObject> MaterializeBlockScope(
-    Isolate* isolate, Handle<Context> context) {
-  DCHECK(context->IsBlockContext());
-  Handle<ScopeInfo> scope_info(ScopeInfo::cast(context->extension()));
-
-  // Allocate and initialize a JSObject with all the arguments, stack locals
-  // heap locals and extension properties of the debugged function.
+    Isolate* isolate, Handle<ScopeInfo> scope_info, Handle<Context> context,
+    JavaScriptFrame* frame, int inlined_jsframe_index) {
   Handle<JSObject> block_scope =
       isolate->factory()->NewJSObject(isolate->object_function());
 
-  // Fill all context locals.
-  if (!ScopeInfo::CopyContextLocalsToScopeObject(scope_info, context,
-                                                 block_scope)) {
-    return MaybeHandle<JSObject>();
+  if (frame != nullptr) {
+    FrameInspector frame_inspector(frame, inlined_jsframe_index, isolate);
+    RETURN_ON_EXCEPTION(isolate,
+                        MaterializeStackLocalsWithFrameInspector(
+                            isolate, block_scope, scope_info, &frame_inspector),
+                        JSObject);
+  }
+
+  if (!context.is_null()) {
+    Handle<ScopeInfo> scope_info_from_context(
+        ScopeInfo::cast(context->extension()));
+    // Fill all context locals.
+    if (!ScopeInfo::CopyContextLocalsToScopeObject(scope_info_from_context,
+                                                   context, block_scope)) {
+      return MaybeHandle<JSObject>();
+    }
   }
 
   return block_scope;
@@ -1362,8 +1393,20 @@ class ScopeIterator {
       case ScopeIterator::ScopeTypeClosure:
         // Materialize the content of the closure scope into a JSObject.
         return MaterializeClosure(isolate_, CurrentContext());
-      case ScopeIterator::ScopeTypeBlock:
-        return MaterializeBlockScope(isolate_, CurrentContext());
+      case ScopeIterator::ScopeTypeBlock: {
+        if (!nested_scope_chain_.is_empty()) {
+          // this is a block scope on the stack.

[rossberg, 2015/04/17 15:16:27]

Nit: upper case

+          Handle<ScopeInfo> scope_info = nested_scope_chain_.last();
+          Handle<Context> context = scope_info->HasContext()
+                                        ? CurrentContext()
+                                        : Handle<Context>::null();
+          return MaterializeBlockScope(isolate_, scope_info, context, frame_,
+                                       inlined_jsframe_index_);
+        } else {
+          return MaterializeBlockScope(isolate_, Handle<ScopeInfo>::null(),
+                                       CurrentContext(), nullptr, 0);
+        }
+      }
       case ScopeIterator::ScopeTypeModule:
         return MaterializeModuleScope(isolate_, CurrentContext());
     }
@@ -1371,6 +1414,16 @@ class ScopeIterator {
     return Handle<JSObject>();
   }
 
+  bool HasContext() {
+    ScopeType type = Type();
+    if (type == ScopeTypeBlock || type == ScopeTypeLocal) {
+      if (!nested_scope_chain_.is_empty()) {
+        return nested_scope_chain_.last()->HasContext();
+      }
+    }
+    return true;
+  }
+
   bool SetVariableValue(Handle<String> variable_name,
                         Handle<Object> new_value) {
     DCHECK(!failed_);
@@ -1392,8 +1445,9 @@ class ScopeIterator {
         return SetScriptVariableValue(CurrentContext(), variable_name,
                                       new_value);
       case ScopeIterator::ScopeTypeBlock:
-        return SetBlockContextVariableValue(CurrentContext(), variable_name,
-                                            new_value);
+        return SetBlockVariableValue(
+            isolate_, HasContext() ? CurrentContext() : Handle<Context>::null(),
+            CurrentScopeInfo(), frame_, variable_name, new_value);
       case ScopeIterator::ScopeTypeModule:
         // TODO(2399): should we implement it?
         break;
@@ -2189,6 +2243,179 @@ static Handle<JSObject> NewJSObjectWithNullProto(Isolate* isolate) {
 }
 
 
+namespace {
+
+// This class builds a context chain for evaluation of expressions
+// in debugger.
+// The scope chain leading up to a breakpoint where evaluation occurs
+// looks like:
+// - [a mix of with, catch and block scopes]
+//    - [function stack + context]
+//      - [outer context]
+// The builder materializes all stack variables into properties of objects;
+// the expression is then evaluated as if it is inside a series of 'with'
+// statements using those objects. To this end, the builder builds a new
+// context chain, based on a scope chain:
+//   - every With and Catch scope begets a cloned context
+//   - Block scope begets one or two contexts:
+//       - if a block has context-allocated varaibles, its context is cloned

[rossberg, 2015/04/17 15:16:27]

typo: variables

+//       - stack locals are materizalized as a With context
+//   - Local scope begets a With context for materizalized locals, chained to
+//     original function context. Original function context is the end of
+//     the chain.
+class EvaluationContextBuilder {
+ public:
+  EvaluationContextBuilder(Isolate* isolate, JavaScriptFrame* frame,
+                           int inlined_jsframe_index)
+      : isolate_(isolate),
+        frame_(frame),
+        inlined_jsframe_index_(inlined_jsframe_index) {
+    FrameInspector frame_inspector(frame, inlined_jsframe_index, isolate);
+    Handle<JSFunction> function =
+        handle(JSFunction::cast(frame_inspector.GetFunction()));
+    Handle<Context> outer_context = handle(function->context(), isolate);
+    outer_info_ = handle(function->shared());
+    Handle<Context> inner_context;
+
+    bool stop = false;
+    for (ScopeIterator it(isolate, frame, inlined_jsframe_index);
+         !it.Failed() && !it.Done() && !stop; it.Next()) {
+      ScopeIterator::ScopeType scope_type = it.Type();
+
+      if (scope_type == ScopeIterator::ScopeTypeLocal) {
+        Handle<JSObject> materialized_function =
+            NewJSObjectWithNullProto(isolate);
+
+        if (!MaterializeStackLocalsWithFrameInspector(
+                 isolate, materialized_function, function, &frame_inspector)
+                 .ToHandle(&materialized_function))
+          return;
+
+        if (!MaterializeArgumentsObject(isolate, materialized_function,
+                                        function)
+                 .ToHandle(&materialized_function))
+          return;
+
+        Handle<Context> parent_context =
+            it.HasContext() ? it.CurrentContext() : outer_context;
+        Handle<Context> with_context = isolate->factory()->NewWithContext(
+            function, parent_context, materialized_function);
+
+        ContextChainElement context_chain_element;
+        context_chain_element.original_context = it.CurrentContext();
+        context_chain_element.materialized_object = materialized_function;
+        context_chain_element.scope_info = it.CurrentScopeInfo();
+        context_chain_.Add(context_chain_element);
+
+        stop = true;
+        RecordContextsInChain(&inner_context, with_context, with_context);
+      } else if (scope_type == ScopeIterator::ScopeTypeCatch ||
+                 scope_type == ScopeIterator::ScopeTypeWith) {
+        Handle<Context> cloned_context =
+            Handle<Context>::cast(FixedArray::CopySize(
+                it.CurrentContext(), it.CurrentContext()->length()));
+
+        ContextChainElement context_chain_element;
+        context_chain_element.original_context = it.CurrentContext();
+        context_chain_element.cloned_context = cloned_context;
+        context_chain_.Add(context_chain_element);
+
+        RecordContextsInChain(&inner_context, cloned_context, cloned_context);
+      } else if (scope_type == ScopeIterator::ScopeTypeBlock) {
+        Handle<JSObject> materialized_object =
+            NewJSObjectWithNullProto(isolate);
+        if (!MaterializeStackLocalsWithFrameInspector(
+                 isolate, materialized_object, it.CurrentScopeInfo(),
+                 &frame_inspector).ToHandle(&materialized_object))
+          return;
+        if (it.HasContext()) {
+          Handle<Context> cloned_context =
+              Handle<Context>::cast(FixedArray::CopySize(
+                  it.CurrentContext(), it.CurrentContext()->length()));
+          Handle<Context> with_context = isolate->factory()->NewWithContext(
+              function, cloned_context, materialized_object);
+
+          ContextChainElement context_chain_element;
+          context_chain_element.original_context = it.CurrentContext();
+          context_chain_element.cloned_context = cloned_context;
+          context_chain_element.materialized_object = materialized_object;
+          context_chain_element.scope_info = it.CurrentScopeInfo();
+          context_chain_.Add(context_chain_element);
+
+          RecordContextsInChain(&inner_context, cloned_context, with_context);
+        } else {
+          Handle<Context> with_context = isolate->factory()->NewWithContext(
+              function, outer_context, materialized_object);
+
+          ContextChainElement context_chain_element;
+          context_chain_element.materialized_object = materialized_object;
+          context_chain_element.scope_info = it.CurrentScopeInfo();
+          context_chain_.Add(context_chain_element);
+
+          RecordContextsInChain(&inner_context, with_context, with_context);
+        }
+      } else {
+        stop = true;
+      }
+    }
+    if (innermost_context_.is_null()) {
+      innermost_context_ = outer_context;
+    }
+    DCHECK(!innermost_context_.is_null());
+  }
+
+  void UpdateVariables() {
+    for (int i = 0; i < context_chain_.length(); i++) {
+      ContextChainElement element = context_chain_[i];
+      if (!element.original_context.is_null() &&
+          !element.cloned_context.is_null()) {
+        Handle<Context> cloned_context = element.cloned_context;
+        cloned_context->CopyTo(
+            Context::MIN_CONTEXT_SLOTS, *element.original_context,
+            Context::MIN_CONTEXT_SLOTS,
+            cloned_context->length() - Context::MIN_CONTEXT_SLOTS);
+      }
+      if (!element.materialized_object.is_null()) {
+        // Write back potential changes to materialized stack locals to the
+        // stack.
+        UpdateStackLocalsFromMaterializedObject(
+            isolate_, element.materialized_object, element.scope_info, frame_,
+            inlined_jsframe_index_);
+      }
+    }
+  }
+
+  Handle<Context> innermost_context() const { return innermost_context_; }
+  Handle<SharedFunctionInfo> outer_info() const { return outer_info_; }
+
+ private:
+  struct ContextChainElement {
+    Handle<Context> original_context;
+    Handle<Context> cloned_context;
+    Handle<JSObject> materialized_object;
+    Handle<ScopeInfo> scope_info;
+  };
+
+  void RecordContextsInChain(Handle<Context>* inner_context,
+                             Handle<Context> first, Handle<Context> last) {
+    if (!inner_context->is_null()) {
+      (*inner_context)->set_previous(*last);
+    } else {
+      innermost_context_ = last;
+    }
+    *inner_context = first;
+  }
+
+  Handle<SharedFunctionInfo> outer_info_;
+  Handle<Context> innermost_context_;
+  List<ContextChainElement> context_chain_;
+  Isolate* isolate_;
+  JavaScriptFrame* frame_;
+  int inlined_jsframe_index_;
+};
+}
+
+
 // Evaluate a piece of JavaScript in the context of a stack frame for
 // debugging.  Things that need special attention are:
 // - Parameters and stack-allocated locals need to be materialized.  Altered
@@ -2225,100 +2452,22 @@ RUNTIME_FUNCTION(Runtime_DebugEvaluate) {
   isolate->set_context(*(save->context()));
 
   // Materialize stack locals and the arguments object.
-  Handle<JSObject> materialized;
-  Handle<JSFunction> function;
-  Handle<SharedFunctionInfo> outer_info;
-  Handle<Context> eval_context;
-
-  // We need to limit the lifetime of the FrameInspector because evaluation can
-  // call arbitrary code and only one FrameInspector can be active at a time.
-  {
-    FrameInspector frame_inspector(frame, inlined_jsframe_index, isolate);
-    materialized = NewJSObjectWithNullProto(isolate);
-    function = handle(JSFunction::cast(frame_inspector.GetFunction()));
-    outer_info = handle(function->shared());
-    eval_context = handle(Context::cast(frame_inspector.GetContext()));
 
-    ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
-        isolate, materialized,
-        MaterializeStackLocalsWithFrameInspector(isolate, materialized,
-                                                 function, &frame_inspector));
-
-    ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
-        isolate, materialized,
-        MaterializeArgumentsObject(isolate, materialized, function));
-  }
-
-  // At this point, the lookup chain may look like this:
-  // [inner context] -> [function stack]+[function context] -> [outer context]
-  // The function stack is not an actual context, it complements the function
-  // context. In order to have the same lookup chain when debug-evaluating,
-  // we:
-  //  - clone inner context
-  //  - materialize the stack and insert it into the context chain as a
-  //    with-context before the function context.
-  // [inner context clone] -> [with context] -> [function context] ->
-  //      [outer context]
-  // Ordering the with-context before the function context forces a dynamic
-  // lookup instead of a static lookup that could fail as the scope info is
-  // outdated and may expect variables to still be stack-allocated.
-  // Afterwards, we write changes to the with-context back to the stack, and
-  // write changes in cloned contexts back to original contexts.
-
-  DCHECK(!eval_context.is_null());
-  Handle<Context> function_context = eval_context;
-  Handle<Context> outer_context(function->context(), isolate);
-  Handle<Context> inner_context;
-  Handle<Context> innermost_context;
-
-  // We iterate to find the function's context, cloning until we hit it.
-  // If the function has no context-allocated variables, we iterate until
-  // we hit the outer context.
-  while (!function_context->IsFunctionContext() &&
-         !function_context->IsScriptContext() &&
-         !function_context.is_identical_to(outer_context)) {
-    Handle<Context> clone = Handle<Context>::cast(
-        FixedArray::CopySize(function_context, function_context->length()));
-    if (!inner_context.is_null()) {
-      inner_context->set_previous(*clone);
-    } else {
-      innermost_context = clone;
-    }
-    inner_context = clone;
-    function_context = Handle<Context>(function_context->previous(), isolate);
+  EvaluationContextBuilder context_builder(isolate, frame,
+                                           inlined_jsframe_index);
+  if (isolate->has_pending_exception()) {
+    return isolate->heap()->exception();
   }
 
-  Handle<Context> materialized_context = isolate->factory()->NewWithContext(
-      function, function_context, materialized);
-
-  if (inner_context.is_null()) {
-    // No inner context. The with-context is now inner-most.
-    innermost_context = materialized_context;
-  } else {
-    inner_context->set_previous(*materialized_context);
-  }
 
   Handle<Object> receiver(frame->receiver(), isolate);
-  MaybeHandle<Object> maybe_result =
-      DebugEvaluate(isolate, outer_info, innermost_context, context_extension,
-                    receiver, source);
+  MaybeHandle<Object> maybe_result = DebugEvaluate(
+      isolate, context_builder.outer_info(),
+      context_builder.innermost_context(), context_extension, receiver, source);
 
   Handle<Object> result;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, result, maybe_result);
-
-  // Write back potential changes to materialized stack locals to the stack.
-  UpdateStackLocalsFromMaterializedObject(isolate, materialized, function,
-                                          frame, inlined_jsframe_index);
-
-  while (!innermost_context.is_identical_to(materialized_context)) {
-    DCHECK(eval_context->map() == innermost_context->map());
-    innermost_context->CopyTo(
-        Context::MIN_CONTEXT_SLOTS, *eval_context, Context::MIN_CONTEXT_SLOTS,
-        innermost_context->length() - Context::MIN_CONTEXT_SLOTS);
-    innermost_context = handle(innermost_context->previous(), isolate);
-    eval_context = handle(eval_context->previous(), isolate);
-  }
-
+  context_builder.UpdateVariables();
   return *result;
 }