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

[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

Committed: https://crrev.com/2fefc4827e2d8b69083194d8b3cc7ab6b9ba7115
Cr-Commit-Position: refs/heads/master@{#34822}

[mythria, 2016-03-14 15:43:14]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-14 15:43:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1796893002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1796893002/1

[commit-bot: I haz the power, 2016-03-14 16:07:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-14 16:07:58]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/2876)
  v8_linux64_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)

[mythria, 2016-03-15 08:54:34]

Description was changed from

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

We need to pop the context to correct level on return as well. 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
==========

to

==========
[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
==========

[mythria, 2016-03-15 08:55:06]

Description was changed from

==========
[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
==========

to

==========
[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
==========

[mythria, 2016-03-15 09:13:42]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-15 09:13:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1796893002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1796893002/20001

[mythria, 2016-03-15 09:49:11]

mythria@chromium.org changed reviewers:
+ mstarzinger@chromium.org, rmcilroy@chromium.org

[mythria, 2016-03-15 09:49:11]

Michi,

I am sorry I messed up this code in my last cl. I tried to clear it up with this
:). PTAL.

Thanks,
Mythri

https://codereview.chromium.org/1796893002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1796893002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:368: if (context != current->context() &&
context->ShouldPopContext()) {
I am still not very sure about the check for context->ShouldPopContext(). If we
pop the outermost context before we return debugger could have problems. I will
add a test case that would cause it to fail.  I might be handling this at the
wrong place, please let me know if this is incorrect to handle it here. If we
need a check for ShouldPopContext do I also need it in the do loop where we pop
the context? I dont think it is correct to conditionally pop there because we go
to the outerscope only if the current control scope did not handle the command.

[mythria, 2016-03-15 09:49:19]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-15 09:49:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1796893002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1796893002/40001

[Michael Achenbach, 2016-03-15 10:06:57]

The CQ bit was unchecked by machenbach@chromium.org

[Michael Achenbach, 2016-03-15 10:07:03]

The CQ bit was checked by machenbach@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-15 10:07:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1796893002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1796893002/40001

[commit-bot: I haz the power, 2016-03-15 10:23:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-15 10:23:45]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-03-15 12:05:45]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-15 12:05:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1796893002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1796893002/80001

[commit-bot: I haz the power, 2016-03-15 12:28:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-15 12:28:02]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-03-16 11:10:05]

I added a new mjsunit test to check we don't pop the outermost context of the
function before returning. I also added a DCHECK that break/continue do not
bubble up to the top level context as discussed with Michi offline. 

PTAL.

Thanks,
Mythri

[rmcilroy, 2016-03-16 12:07:28]

Looks good to me, but I'll leave it to Michi to approve.

https://codereview.chromium.org/1796893002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1796893002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:367: // Do not pop if it is the outermost
context.
This comment is a bit confusing. Could you make this comment say to pop context
to expected depth but not to pop the outer function context.

[Michael Starzinger, 2016-03-16 14:16:53]

LGTM.

https://codereview.chromium.org/1796893002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1796893002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:222: case CMD_BREAK:  // We should never
see break/continue in top-level.
nit: Please leave the cases in the same order as everywhere else for
readability.

[mythria, 2016-03-16 14:33:20]

Thanks. I fixed both of them.

https://codereview.chromium.org/1796893002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1796893002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:222: case CMD_BREAK:  // We should never
see break/continue in top-level.
On 2016/03/16 14:16:53, Michael Starzinger wrote:
> nit: Please leave the cases in the same order as everywhere else for
> readability.

Done.

https://codereview.chromium.org/1796893002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:367: // Do not pop if it is the outermost
context.
On 2016/03/16 12:07:28, rmcilroy wrote:
> This comment is a bit confusing. Could you make this comment say to pop
context
> to expected depth but not to pop the outer function context.

Done.

[mythria, 2016-03-16 14:33:29]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-16 14:33:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1796893002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1796893002/100001

[commit-bot: I haz the power, 2016-03-16 14:34:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-16 14:34:53]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/3019)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[mythria, 2016-03-16 14:49:53]

The CQ bit was checked by mythria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-16 14:50:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1796893002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1796893002/120001

[commit-bot: I haz the power, 2016-03-16 15:11:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-16 15:11:39]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-03-16 15:36:10]

The CQ bit was checked by mythria@chromium.org

[mythria, 2016-03-16 15:36:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1796893002/#ps120001
(title: "Rebased the patch.")

[commit-bot: I haz the power, 2016-03-16 15:36:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1796893002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1796893002/120001

[commit-bot: I haz the power, 2016-03-16 15:51:19]

Description was changed from

==========
[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
==========

to

==========
[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
==========

[commit-bot: I haz the power, 2016-03-16 15:51:21]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-16 15:53:01]

Description was changed from

==========
[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
==========

to

==========
[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

Committed: https://crrev.com/2fefc4827e2d8b69083194d8b3cc7ab6b9ba7115
Cr-Commit-Position: refs/heads/master@{#34822}
==========

[commit-bot: I haz the power, 2016-03-16 15:53:02]

Patchset 7 (id:??) landed as
https://crrev.com/2fefc4827e2d8b69083194d8b3cc7ab6b9ba7115
Cr-Commit-Position: refs/heads/master@{#34822}

[Michael Achenbach, 2016-03-16 18:59:03]

https://codereview.chromium.org/1796893002/diff/120001/test/mjsunit/ignition/...
File test/mjsunit/ignition/debug-scope-on-return.js (right):

https://codereview.chromium.org/1796893002/diff/120001/test/mjsunit/ignition/...
test/mjsunit/ignition/debug-scope-on-return.js:2: // Use of this source code is
governed by a BSD-style license that can be
Please skip this test with ignition+msan:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20...