[Interpreter] Pops the context to the correct level on return.

We need to pop the context to correct level on return as well. This was incorrectly
removed in this cl: https://codereview.chromium.org/1768123002/. For example
when we have a try-catch-finally block and catch does a return, the return
does not happen immediately. It should execute finally block before it
returns. Return statement should pop the context to the correct level as
expected by finally block.

BUG=594369,v8:4280
LOG=N

Review URL: https://codereview.chromium.org/1796893002

Cr-Commit-Position: refs/heads/master@{#34822}

src/interpreter/bytecode-generator.cc

@@ -72,6 +72,7 @@ class BytecodeGenerator::ContextScope BASE_EMBEDDED {
 
   Scope* scope() const { return scope_; }
   Register reg() const { return register_; }
+  bool ShouldPopContext() { return should_pop_context_; }
 
  private:
   const BytecodeArrayBuilder* builder() const { return generator_->builder(); }
@@ -212,9 +213,9 @@ class BytecodeGenerator::ControlScopeForTopLevel final
  protected:
   bool Execute(Command command, Statement* statement) override {
     switch (command) {
-      case CMD_BREAK:
+      case CMD_BREAK:  // We should never see break/continue in top-level.
       case CMD_CONTINUE:
-        break;
+        UNREACHABLE();
       case CMD_RETURN:
         generator()->builder()->Return();
         return true;
@@ -363,21 +364,20 @@ void BytecodeGenerator::ControlScope::PerformCommand(Command command,
                                                      Statement* statement) {
   ControlScope* current = this;
   ContextScope* context = generator()->execution_context();
+  // Pop context to the expected depth but do not pop the outermost context.
+  if (context != current->context() && context->ShouldPopContext()) {
+    generator()->builder()->PopContext(current->context()->reg());
+  }
   do {
-    if (current->context() != context) {
-      // Pop context to the expected depth for break and continue. For return
-      // and throw it is not required to pop. Debugger expects that the
-      // context is not popped on return. So do not pop on return.
-      // TODO(rmcilroy): Only emit a single context pop.
-      if (command == CMD_BREAK || command == CMD_CONTINUE) {
-        generator()->builder()->PopContext(current->context()->reg());
-      }
-      context = current->context();
-    }
     if (current->Execute(command, statement)) {
       return;
     }
     current = current->outer();
+    if (current->context() != context) {
+      // Pop context to the expected depth.
+      // TODO(rmcilroy): Only emit a single context pop.
+      generator()->builder()->PopContext(current->context()->reg());
+    }
   } while (current != nullptr);
   UNREACHABLE();
 }

test/cctest/interpreter/bytecode_expectations/BasicLoops.golden

@@ -793,13 +793,13 @@ snippet: "
 "
 frame size: 7
 parameter count: 1
-bytecode array length: 118
+bytecode array length: 120
 bytecodes: [
   B(StackCheck),
   B(LdaZero),
   B(Star), R(1),
   B(Ldar), R(1),
-  B(JumpIfToBooleanFalse), U8(110),
+  B(JumpIfToBooleanFalse), U8(112),
   B(StackCheck),
   B(LdaConstant), U8(0),
   B(Star), R(4),
@@ -824,9 +824,10 @@ bytecodes: [
   B(LdaConstant), U8(3),
   B(Star), R(4),
   B(CallRuntime), U16(Runtime::kThrowReferenceError), R(4), U8(1),
-  B(JumpIfToBooleanFalse), U8(6),
+  B(JumpIfToBooleanFalse), U8(8),
   B(PopContext), R(3),
-  B(Jump), U8(-67),
+  B(PopContext), R(3),
+  B(Jump), U8(-69),
   B(LdaContextSlot), R(context), U8(4),
   B(JumpIfNotHole), U8(11),
   B(LdaConstant), U8(3),
@@ -844,7 +845,7 @@ bytecodes: [
   B(Ldar), R(5),
   B(StaContextSlot), R(context), U8(4),
   B(PopContext), R(3),
-  B(Jump), U8(-110),
+  B(Jump), U8(-112),
   B(LdaUndefined),
   B(Return),
 ]

test/cctest/interpreter/bytecode_expectations/ContextVariables.golden

@@ -106,7 +106,7 @@ snippet: "
 "
 frame size: 4
 parameter count: 1
-bytecode array length: 45
+bytecode array length: 47
 bytecodes: [
   B(CallRuntime), U16(Runtime::kNewFunctionContext), R(closure), U8(1),
   B(PushContext), R(0),
@@ -126,6 +126,7 @@ bytecodes: [
   B(LdaSmi8), U8(2),
   B(StaContextSlot), R(context), U8(4),
   B(CreateClosure), U8(1), U8(0),
+  B(PopContext), R(0),
   B(Return),
 ]
 constant pool: [

test/cctest/interpreter/bytecode_expectations/TryCatch.golden

@@ -13,7 +13,7 @@ snippet: "
 "
 frame size: 5
 parameter count: 1
-bytecode array length: 38
+bytecode array length: 40
 bytecodes: [
   B(StackCheck),
   B(Mov), R(context), R(1),
@@ -30,6 +30,7 @@ bytecodes: [
   B(Ldar), R(1),
   B(PushContext), R(0),
   B(LdaSmi8), U8(2),
+  B(PopContext), R(0),
   B(Return),
   B(LdaUndefined),
   B(Return),

test/cctest/interpreter/bytecode_expectations/WithStatement.golden

@@ -13,7 +13,7 @@ snippet: "
 "
 frame size: 4
 parameter count: 1
-bytecode array length: 24
+bytecode array length: 26
 bytecodes: [
   B(StackCheck),
   B(CreateObjectLiteral), U8(0), U8(0), U8(5),
@@ -25,6 +25,7 @@ bytecodes: [
   B(CallRuntime), U16(Runtime::kPushWithContext), R(2), U8(2),
   B(PushContext), R(0),
   B(LdaLookupSlot), U8(1),
+  B(PopContext), R(0),
   B(Return),
 ]
 constant pool: [

test/cctest/interpreter/test-interpreter.cc

@@ -2075,6 +2075,15 @@ TEST(InterpreterTryFinally) {
                      "  try { a = 3; throw 23; } finally { a = 4; }"
                      "} catch(e) { a = a + e; } return a;",
                      factory->NewStringFromStaticChars("R27")),
+      std::make_pair("var func_name;"
+                     "function tcf2(a) {"
+                     "  try { throw new Error('boom');} "
+                     "  catch(e) {return 153; } "
+                     "  finally {func_name = tcf2.name;}"
+                     "}"
+                     "tcf2();"
+                     "return func_name;",
+                     factory->NewStringFromStaticChars("Rtcf2")),
   };
 
   const char* try_wrapper =

test/mjsunit/ignition/debug-scope-on-return.js

@@ -0,0 +1,31 @@
+// Copyright 2016 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.
+
+// Flags: --expose-debug-as debug
+
+// Check that the we are still in function context when we break on return.
+
+var Debug = debug.Debug;
+
+function listener(event, exec_state, event_data, data) {
+  if (event == Debug.DebugEvent.Break) {
+    // Access scope details to check the context is correct.
+    var scope_count = exec_state.frame().scopeCount();
+    // Do steps until we reach the global scope again.
+    exec_state.prepareStep(Debug.StepAction.StepIn);
+  }
+}
+
+Debug.setListener(listener);
+
+function f() {
+  debugger;
+
+  L: with ({x:12}) {
+    break L;
+  }
+
+  return;
+}
+f();