[ignition] Fix hole check for dynamic local variables

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 "src/ast/compile-time-value.h"
 #include "src/ast/scopes.h"
 #include "src/code-stubs.h"
 #include "src/compilation-info.h"
@@ skipping 1842 matching lines @@
       break;
     }
     case VariableLocation::LOOKUP: {
       switch (variable->mode()) {
         case DYNAMIC_LOCAL: {
           Variable* local_variable = variable->local_if_not_shadowed();
           int depth =
               execution_context()->ContextChainDepth(local_variable->scope());
           builder()->LoadLookupContextSlot(variable->name(), typeof_mode,
                                            local_variable->index(), depth);
-          if (hole_check_mode == HoleCheckMode::kRequired) {
+          if (local_variable->binding_needs_init()) {

[rmcilroy, 2016/12/05 19:18:07]

This seems a bit hacky, I would rather fix the hole_check_mode in scope.cc
AccessNeedsHoleCheck (assuming this is where the error is introduced).

[Leszek Swirski, 2016/12/05 19:42:37]

That's what I thought originally, but non-local variables were explicitly
defined as not needing init in https://codereview.chromium.org/2232343002/
(which is why I added neis and adamk as reviewers).

I'm not sure what a good representation of the semantics is -- the actual lookup
variable itself does not need a hole check (because it's done in the runtime,
and has to be done in the runtime unconditionally because it's runtime anything
can happen). On the fast path, it's the local (non-shadowed) variable that we
care about, and for this one we *do* want the hole check to be conditional, but
it's conditional on some property of the local variable. So, the "needs hole
check" for the lookup variable is correct, it's just not the same as "needs hole
check" for the local variable.

At least, that's how I understand it, I may be way off.

[adamk, 2016/12/06 19:16:42]

I think it's possible for us to do this determination earlier, in
AccessNeedsHoleCheck, since we have all three pieces of data necessary: the
VariableProxy, the DYNAMIC_LOCAL variable, and the local_if_not_shadowed
variable.

It might be as simple as the following at the top of AccessNeedsHoleCheck():

if (var->mode() == DYNAMIC_LOCAL) return
AccessNeedsHoleCheck(var->local_if_not_shadowed(), proxy, scope);

[Leszek Swirski, 2016/12/07 16:09:52]

Fair enough, I was worried that changing it here would be a violation of the
semantics (or too tight a coupling with the execution), but if you're both fine
with it then sure. Done.

             BuildThrowIfHole(variable->name());
           }
           break;
         }
         case DYNAMIC_GLOBAL: {
           int depth = scope()->ContextChainLengthUntilOutermostSloppyEval();
           builder()->LoadLookupGlobalSlot(variable->name(), typeof_mode,
                                           feedback_index(slot), depth);
           break;
         }
@@ skipping 1349 matching lines @@
 }
 
 Runtime::FunctionId BytecodeGenerator::StoreKeyedToSuperRuntimeId() {
   return is_strict(language_mode()) ? Runtime::kStoreKeyedToSuper_Strict
                                     : Runtime::kStoreKeyedToSuper_Sloppy;
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8