Add support for calculating a scopes maximum nested context chain.

src/interpreter/bytecode-generator.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/interpreter/bytecode-generator.h"
 
 #include <stack>
 
 #include "src/compiler.h"
 #include "src/full-codegen/full-codegen.h"
@@ skipping 134 matching lines @@
 
 BytecodeGenerator::~BytecodeGenerator() {}
 
 
 Handle<BytecodeArray> BytecodeGenerator::MakeBytecode(CompilationInfo* info) {
   set_info(info);
   set_scope(info->scope());
 
   builder()->set_parameter_count(info->num_parameters_including_this());
   builder()->set_locals_count(scope()->num_stack_slots());
-  // TODO(rmcilroy): Set correct context count.
-  builder()->set_context_count(info->num_heap_slots() > 0 ? 1 : 0);
+  builder()->set_context_count(scope()->MaxNestedContextChainLength());

[adamk, 2015/10/15 07:38:44]

Is this exercised by any tests (I'm not familiar with the testing plan for the
interpreter)?

[rmcilroy, 2015/10/15 09:23:07]

This is exercised by test-bytecode-generator.cc::DeclareGlobals and
test-interpreter.cc::DeclareGlobals, but only for the inner function context
(just below) since we don't yet have support for any other nested contexts (this
change is required before we can add that support).

One thing I was trying to figure out - is there any way to trigger a normal
(non-with) block to require a nested context without calling eval (which we
don't support yet either)? If there is an easy way to do this which is supported
by the current interpreter functionality I could add a test for that to this CL.

[adamk, 2015/10/15 09:34:32]

Capturing a lexical variable from a block will force it to be heap allocated:

function f() {
  "use strict";
  let x = 1;
  {
    let y = 2;
    return function g() { return [x, y]; }
  }
}

in this case, g's context chain will include the block containing "y".

[rmcilroy, 2015/10/15 14:06:16]

Great, thanks for the pointer. It turns out this needs a bit more support than
the interpreter supported, but I've added the test to my followup CL at
https://codereview.chromium.org/1403943004/ (both in test-interpreter.cc and
test-bytecode-generator.cc).

 
   // Build function context only if there are context allocated variables.
-  if (info->num_heap_slots() > 0) {
+  if (scope()->NeedsContext()) {

[adamk, 2015/10/15 07:38:45]

I'd leave this as num_heap_slots() > 0 (see other comment).

[rmcilroy, 2015/10/15 09:23:07]

Commented on the other comment.

     // Push a new inner context scope for the function.
     VisitNewLocalFunctionContext();
     ContextScope top_context(this, true);
     MakeBytecodeBody();
   } else {
     MakeBytecodeBody();
   }
 
   set_scope(nullptr);
   set_info(nullptr);
@@ skipping 1032 matching lines @@
 }
 
 
 int BytecodeGenerator::feedback_index(FeedbackVectorSlot slot) const {
   return info()->feedback_vector()->GetIndex(slot);
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8