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 94ac074de3ec0b44d0a3efc4cd76710d7d718702..d23d0e2322e983aa019cfa3364937df4cd52d324 100644
--- a/src/runtime/runtime-debug.cc
+++ b/src/runtime/runtime-debug.cc
@@ -686,17 +686,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_NOT_NULL(frame_inspector);
+
     HandleScope scope(isolate);
     Handle<Object> value(i < frame_inspector->GetParametersCount()
                              ? frame_inspector->GetParameter(i)
@@ -713,8 +712,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();
+    }
 
     RETURN_ON_EXCEPTION(isolate, Runtime::SetObjectProperty(
                                      isolate, target, name, value, SLOPPY),
@@ -724,12 +727,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
@@ -737,9 +749,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.
@@ -756,12 +765,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);
   }
 }
 
@@ -903,7 +913,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;
     }
   }
@@ -940,6 +950,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(
@@ -1020,17 +1054,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) {
@@ -1082,19 +1105,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;
@@ -1361,8 +1392,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.
+          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());
     }
@@ -1370,6 +1413,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_);
@@ -1391,8 +1444,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;
@@ -2188,6 +2242,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
+//       - 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
@@ -2224,100 +2451,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;
 }