Make non-Module generators only context allocate parameters.

In particular, local variables should be allocated on stack (in bytecode register), and stored/loaded to the generator object on generator suspend/resume.

The CL is based on @adamk's change to scoping/parsers (https://chromium-review.googlesource.com/c/498538/), I only made the debugger cope with this change.

I should note that the CL changes the scope type of suspended generators from ScopeType.Closure to ScopeType.Local. In the future we might want to introduce ScopeType.SuspendedGenerator to make the distinction explicit.

Some of the changes in the tests have been made because the debugger functions do not return scopes of closed generators anymore. Generators should be allowed to throw away their internal state when they finish.

BUG=v8:6368

Review-Url: https://codereview.chromium.org/2898163002
Cr-Commit-Position: refs/heads/master@{#45515}

src/ast/scopes.cc

@@ -348,6 +348,7 @@ void Scope::SetDefaults() {
 
   inner_scope_calls_eval_ = false;
   force_context_allocation_ = false;
+  force_context_allocation_for_parameters_ = false;
 
   is_declaration_scope_ = false;
 
@@ -2193,7 +2194,8 @@ void DeclarationScope::AllocateParameterLocals() {
 
 void DeclarationScope::AllocateParameter(Variable* var, int index) {
   if (MustAllocate(var)) {
-    if (MustAllocateInContext(var)) {
+    if (has_forced_context_allocation_for_parameters() ||
+        MustAllocateInContext(var)) {
       DCHECK(var->IsUnallocated() || var->IsContextSlot());
       if (var->IsUnallocated()) {
         AllocateHeapSlot(var);

src/ast/scopes.h

@@ -323,6 +323,13 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   bool has_forced_context_allocation() const {
     return force_context_allocation_;
   }
+  void ForceContextAllocationForParameters() {
+    DCHECK(!already_resolved_);
+    force_context_allocation_for_parameters_ = true;
+  }
+  bool has_forced_context_allocation_for_parameters() const {
+    return force_context_allocation_for_parameters_;
+  }
 
   // ---------------------------------------------------------------------------
   // Predicates.
@@ -574,6 +581,7 @@ class V8_EXPORT_PRIVATE Scope : public NON_EXPORTED_BASE(ZoneObject) {
   // True if one of the inner scopes or the scope itself calls eval.
   bool inner_scope_calls_eval_ : 1;
   bool force_context_allocation_ : 1;
+  bool force_context_allocation_for_parameters_ : 1;
 
   // True if it holds 'var' declarations.
   bool is_declaration_scope_ : 1;

src/debug/debug-scopes.h

@@ -93,7 +93,8 @@ class ScopeIterator {
   };
 
   Isolate* isolate_;
-  FrameInspector* const frame_inspector_;
+  FrameInspector* const frame_inspector_ = nullptr;
+  Handle<JSGeneratorObject> generator_;
   Handle<Context> context_;
   List<ExtendedScopeInfo> nested_scope_chain_;
   Handle<StringSet> non_locals_;
@@ -103,9 +104,14 @@ class ScopeIterator {
     return frame_inspector_->GetArgumentsFrame();
   }
 
-  inline Handle<JSFunction> GetFunction() {
-    return frame_inspector_->GetFunction();
-  }
+  Handle<Context> GetContext();
+  Handle<JSFunction> GetFunction();
+  int GetSourcePosition();
+
+  void MaterializeStackLocals(Handle<JSObject> local_scope,
+                              Handle<ScopeInfo> scope_info);
+
+  void TryParseAndRetrieveScopes(ScopeIterator::Option option);
 
   void RetrieveScopeChain(DeclarationScope* scope);
 
@@ -133,7 +139,7 @@ class ScopeIterator {
                              Handle<Object> new_value);
 
   // Helper functions.
-  bool SetParameterValue(Handle<ScopeInfo> scope_info, JavaScriptFrame* frame,
+  bool SetParameterValue(Handle<ScopeInfo> scope_info,
                          Handle<String> parameter_name,
                          Handle<Object> new_value);
   bool SetStackVariableValue(Handle<ScopeInfo> scope_info,

src/parsing/parser.cc

@@ -706,6 +706,7 @@ FunctionLiteral* Parser::DoParseProgram(ParseInfo* info) {
       var->AllocateTo(VariableLocation::PARAMETER, 0);
 
       PrepareGeneratorVariables();
+      scope->ForceContextAllocation();
       Expression* initial_yield =
           BuildInitialYield(kNoSourcePosition, kGeneratorFunction);
       body->Add(
@@ -2531,11 +2532,9 @@ void Parser::DeclareArrowFunctionFormalParameters(
 }
 
 void Parser::PrepareGeneratorVariables() {
-  // For generators, allocating variables in contexts is currently a win because
-  // it minimizes the work needed to suspend and resume an activation.  The
-  // code produced for generators relies on this forced context allocation (it
-  // does not restore the frame's parameters upon resume).
-  function_state_->scope()->ForceContextAllocation();
+  // The code produced for generators relies on forced context allocation of
+  // parameters (it does not restore the frame's parameters upon resume).
+  function_state_->scope()->ForceContextAllocationForParameters();
 
   // Calling a generator returns a generator object.  That object is stored
   // in a temporary variable, a definition that is used by "yield"

src/runtime/runtime-debug.cc

@@ -921,6 +921,11 @@ RUNTIME_FUNCTION(Runtime_GetGeneratorScopeCount) {
   // Check arguments.
   CONVERT_ARG_HANDLE_CHECKED(JSGeneratorObject, gen, 0);
 
+  // Only inspect suspended generator scopes.
+  if (!gen->is_suspended()) {
+    return Smi::kZero;
+  }
+
   // Count the visible scopes.
   int n = 0;
   for (ScopeIterator it(isolate, gen); !it.Done(); it.Next()) {
@@ -942,6 +947,11 @@ RUNTIME_FUNCTION(Runtime_GetGeneratorScopeDetails) {
   CONVERT_ARG_HANDLE_CHECKED(JSGeneratorObject, gen, 0);
   CONVERT_NUMBER_CHECKED(int, index, Int32, args[1]);
 
+  // Only inspect suspended generator scopes.
+  if (!gen->is_suspended()) {
+    return isolate->heap()->undefined_value();
+  }
+
   // Find the requested scope.
   int n = 0;
   ScopeIterator it(isolate, gen);

test/debugger/debug/debug-scopes-suspended-generators.js

@@ -107,7 +107,7 @@ function *gen1() {
 }
 
 var g = gen1();
-CheckScopeChain([debug.ScopeType.Closure,
+CheckScopeChain([debug.ScopeType.Local,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
 CheckScopeContent({}, 0, g);
@@ -120,7 +120,7 @@ function *gen2(a) {
 }
 
 g = gen2(42);
-CheckScopeChain([debug.ScopeType.Closure,
+CheckScopeChain([debug.ScopeType.Local,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
 CheckScopeContent({a: 42}, 0, g);
@@ -134,7 +134,7 @@ function *gen3(a) {
 }
 
 g = gen3(0);
-CheckScopeChain([debug.ScopeType.Closure,
+CheckScopeChain([debug.ScopeType.Local,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
 CheckScopeContent({a: 0, b: undefined}, 0, g);
@@ -148,11 +148,12 @@ function *gen4(a, b) {
   var x = 2;
   yield a;
   var y = 3;
+  yield a;
   return b;
 }
 
 g = gen4(0, 1);
-CheckScopeChain([debug.ScopeType.Closure,
+CheckScopeChain([debug.ScopeType.Local,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
 CheckScopeContent({a: 0, b: 1, x: undefined, y: undefined}, 0, g);
@@ -167,12 +168,13 @@ CheckScopeContent({a: 0, b: 1, x: 2, y: 3}, 0, g);
 
 function *gen5(a) {
   eval('var b = 2');
+  yield a;
   return b;
 }
 
 g = gen5(1);
 g.next();
-CheckScopeChain([debug.ScopeType.Closure,
+CheckScopeChain([debug.ScopeType.Local,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
 CheckScopeContent({a: 1, b: 2}, 0, g);
@@ -190,13 +192,13 @@ function *gen6() {
 g = gen6();
 g.next();
 CheckScopeChain([debug.ScopeType.With,
-                 debug.ScopeType.Closure,
+                 debug.ScopeType.Local,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
 CheckScopeContent({}, 0, g);
 
 g.next();
-CheckScopeChain([debug.ScopeType.Closure,
+CheckScopeChain([debug.ScopeType.Local,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
 
@@ -216,7 +218,7 @@ g = gen7();
 g.next();
 CheckScopeChain([debug.ScopeType.With,
                  debug.ScopeType.With,
-                 debug.ScopeType.Closure,
+                 debug.ScopeType.Local,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
 CheckScopeContent({}, 0, g);
@@ -237,7 +239,7 @@ g = gen8();
 g.next();
 CheckScopeChain([debug.ScopeType.With,
                  debug.ScopeType.With,
-                 debug.ScopeType.Closure,
+                 debug.ScopeType.Local,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
 CheckScopeContent({a: 2, b: 1}, 0, g);
@@ -259,7 +261,7 @@ function *gen9() {
 g = gen9();
 g.next();
 CheckScopeChain([debug.ScopeType.Catch,
-                 debug.ScopeType.Closure,
+                 debug.ScopeType.Local,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
 CheckScopeContent({e: 42}, 0, g);
@@ -274,24 +276,23 @@ function *gen10() {
 g = gen10();
 g.next();
 CheckScopeChain([debug.ScopeType.Block,
-                 debug.ScopeType.Block,
-                 debug.ScopeType.Closure,
+                 debug.ScopeType.Local,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
 CheckScopeContent({i: 0}, 0, g);
 
 g.next();
 CheckScopeContent({i: 1}, 0, g);
-CheckScopeContent({i: 0}, 1, g);  // Additional block scope with i = 0;
 
 // Nested generators.
 
 var gen12;
 function *gen11() {
+  var b = 2;
   gen12 = function*() {
     var a = 1;
     yield 1;
-    return 2;
+    return b;
   }();
 
   var a = 0;
@@ -301,12 +302,12 @@ function *gen11() {
 gen11().next();
 g = gen12;
 
-CheckScopeChain([debug.ScopeType.Closure,
+CheckScopeChain([debug.ScopeType.Local,
                  debug.ScopeType.Closure,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
 CheckScopeContent({a: 1}, 0, g);
-CheckScopeContent({a: 0}, 1, g);
+CheckScopeContent({b: 2}, 1, g);
 
 // Set a variable in an empty scope.
 
@@ -355,7 +356,7 @@ var g = gen15();
 assertEquals(1, g.next().value);
 
 CheckScopeChain([debug.ScopeType.With,
-                 debug.ScopeType.Closure,
+                 debug.ScopeType.Local,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
 CheckScopeContent({a: 1, b: 2}, 0, g);
@@ -376,7 +377,7 @@ CheckScopeContent({a: 1, b: 2}, 0, g);
 CheckScopeContent({c: 1, d: 4, e: 42}, 1, g);
 assertEquals(5, g.next().value);  // Initialized after set.
 
-CheckScopeChain([debug.ScopeType.Closure,
+CheckScopeChain([debug.ScopeType.Local,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
 
@@ -404,15 +405,15 @@ g.next();
 
 CheckScopeChain([debug.ScopeType.Block,
                  debug.ScopeType.With,
-                 debug.ScopeType.Closure,
+                 debug.ScopeType.Local,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
-CheckScopeContent({d: 4}, 0, g);
+CheckScopeContent({d: 4, e: undefined}, 0, g);
 CheckScopeContent({a: 1, b: 2}, 1, g);
 CheckScopeContent({c: 3}, 2, g);
 
 Debug.generatorScope(g, 0).setVariableValue("d", 1);
-CheckScopeContent({d: 1}, 0, g);
+CheckScopeContent({d: 1, e: undefined}, 0, g);
 
 assertEquals(1, g.next().value);
 
@@ -434,7 +435,7 @@ g = gen17();
 g.next();
 
 CheckScopeChain([debug.ScopeType.Catch,
-                 debug.ScopeType.Closure,
+                 debug.ScopeType.Local,
                  debug.ScopeType.Script,
                  debug.ScopeType.Global], g);
 CheckScopeContent({e: 42}, 0, g);

test/debugger/debug/es8/async-function-debug-evaluate.js

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Flags: --noalways-opt
+
 var Debug = debug.Debug;
 var breakPointCount = 0;
 
@@ -12,10 +14,9 @@ function listener(event, exec_state, event_data, data) {
     if (breakPointCount === 1) {
       assertEquals(
           "inner", exec_state.frame(0).evaluate("inner").value());
-      assertThrows(() => exec_state.frame(0).evaluate("letInner").value(),
-                   ReferenceError);
-      assertThrows(() => exec_state.frame(0).evaluate("constInner").value(),
-                   ReferenceError);
+      // Variables in TDZ have 'undefined' as their values.
+      assertEquals(undefined, exec_state.frame(0).evaluate("letInner").value());
+      assertEquals(undefined, exec_state.frame(0).evaluate("constInner").value());
 
       assertEquals("outer", exec_state.frame(0).evaluate("outer").value());
       assertEquals(
@@ -29,16 +30,15 @@ function listener(event, exec_state, event_data, data) {
       assertEquals(
           "let outer", exec_state.frame(1).evaluate("letOuter").value());
 
-      assertThrows(() => exec_state.frame(0).evaluate("withVar").value(),
-                   ReferenceError);
+      // Variables in TDZ have 'undefined' as their values.
+      assertEquals(undefined, exec_state.frame(0).evaluate("withVar").value());
 
     } else if (breakPointCount === 2) {
       assertEquals(
           "inner", exec_state.frame(0).evaluate("inner").value());
-      assertThrows(() => exec_state.frame(0).evaluate("letInner").value(),
-                   ReferenceError);
-      assertThrows(() => exec_state.frame(0).evaluate("constInner").value(),
-                   ReferenceError);
+      // Variables in TDZ have 'undefined' as their values.
+      assertEquals(undefined, exec_state.frame(0).evaluate("letInner").value());
+      assertEquals(undefined, exec_state.frame(0).evaluate("constInner").value());
 
       assertEquals(57, exec_state.frame(0).evaluate("x").value());
       assertEquals(100, exec_state.frame(0).evaluate("y").value());
@@ -70,8 +70,8 @@ function listener(event, exec_state, event_data, data) {
           "Error",
           exec_state.frame(0).evaluate("error.constructor.name").value());
       assertEquals("floof", exec_state.frame(0).evaluate("bun").value());
-      assertThrows(() => exec_state.frame(0).evaluate("cow").value(),
-                   ReferenceError);
+      // Variables in TDZ have 'undefined' as their values.
+      assertEquals(undefined, exec_state.frame(0).evaluate("cow").value())
 
       assertEquals("outer", exec_state.frame(0).evaluate("outer").value());
       assertEquals(

test/debugger/debug/es8/async-function-debug-scopes.js

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Flags: --noalways-opt
+
 var Debug = debug.Debug;
 var global_marker = 7;
 
@@ -400,7 +402,7 @@ await test(
                        debug.ScopeType.Closure,
                        debug.ScopeType.Script,
                        debug.ScopeType.Global], exec_state);
-      CheckScopeContent({a:1, x: 2}, 1, exec_state);
+      CheckScopeContent({a:1}, 1, exec_state);
     },
     result => result());
 
@@ -418,7 +420,7 @@ await test(
                        debug.ScopeType.Script,
                        debug.ScopeType.Global], exec_state);
       CheckScopeContent({e: 'Exception'}, 0, exec_state);
-      CheckScopeContent({a:1, x: 2}, 2, exec_state);
+      CheckScopeContent({a:1}, 2, exec_state);
     },
     result => result());
 

test/debugger/debug/regress-3225.js

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Flags: --noalways-opt
 
 Debug = debug.Debug
 

test/inspector/debugger/suspended-generator-scopes-expected.txt

@@ -9,11 +9,11 @@ Running test: testScopesPaused
                 configurable : true
                 enumerable : true
                 isOwn : true
-                name : a
+                name : b
                 value : {
-                    description : 420
+                    description : 42
                     type : number
-                    value : 420
+                    value : 42
                 }
                 writable : true
             }
@@ -21,11 +21,11 @@ Running test: testScopesPaused
                 configurable : true
                 enumerable : true
                 isOwn : true
-                name : b
+                name : a
                 value : {
-                    description : 42
+                    description : 420
                     type : number
-                    value : 42
+                    value : 420
                 }
                 writable : true
             }
@@ -42,11 +42,9 @@ Running test: testScopesNonPaused
                 configurable : true
                 enumerable : true
                 isOwn : true
-                name : a
+                name : b
                 value : {
-                    description : 430
-                    type : number
-                    value : 430
+                    type : undefined
                 }
                 writable : true
             }
@@ -54,9 +52,11 @@ Running test: testScopesNonPaused
                 configurable : true
                 enumerable : true
                 isOwn : true
-                name : b
+                name : a
                 value : {
-                    type : undefined
+                    description : 430
+                    type : number
+                    value : 430
                 }
                 writable : true
             }

test/inspector/runtime/internal-properties-expected.txt

@@ -339,16 +339,6 @@ expression: gen1.next();gen1
                     }
                 }
             }
-            [4] : {
-                name : [[Scopes]]
-                value : {
-                    className : Array
-                    description : Scopes[2]
-                    objectId : <objectId>
-                    subtype : internal#scopeList
-                    type : object
-                }
-            }
         ]
     }
 }