[Interpreter] Add support for new local function context creation.

[Interpreter] Add support for new local function context creation.

Adds support for creation of new local function contexts (or script context for
top-level code). As part of this, also adds support for context push/pop
operations using a ContextScope object in BytecodeGenerator. Adds the following
bytecodes:
 - PushContext
 - PopContext

Support for inner contexts and loading from / storing to context allocated
variables will come in a future CL.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/c0185b7d984419eb11dd290476c62deb977bfa9f
Cr-Commit-Position: refs/heads/master@{#31238}

[rmcilroy, 2015-10-06 11:57:24]

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

[commit-bot: I haz the power, 2015-10-06 11:57:32]

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

[rmcilroy, 2015-10-06 11:57:56]

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

[rmcilroy, 2015-10-06 11:57:57]

Orion, Michi, could you please take a look, thanks.

[commit-bot: I haz the power, 2015-10-06 11:58:32]

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

[commit-bot: I haz the power, 2015-10-06 11:58:32]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/8694)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[rmcilroy, 2015-10-07 20:53:19]

Patchset #2 (id:20001) has been deleted

[oth, 2015-10-08 09:30:35]

lgtm

[oth, 2015-10-08 09:30:38]

lgtm

[rmcilroy, 2015-10-08 09:43:28]

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

[commit-bot: I haz the power, 2015-10-08 09:43:37]

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

[rmcilroy, 2015-10-08 09:45:32]

rmcilroy@chromium.org changed reviewers:
+ bmeurer@chromium.org

[rmcilroy, 2015-10-08 09:45:32]

+Benedikt: As discussed offline, I've needed to modify the ia32/x87 ports to
move the context parameter into a register (swapped for dispatch table), since
Turbofan doesn't currently support tail calls with mutated stack argument
parameters.

cc: Danno for context since he's working on tail call support.

[commit-bot: I haz the power, 2015-10-08 10:04:40]

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

[commit-bot: I haz the power, 2015-10-08 10:04:41]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2015-10-09 10:49:47]

Ping?

[rmcilroy, 2015-10-13 09:13:59]

Michi / Benedikt, could you take a look please? My follow-on CLs depend on this
CL. Thanks.

[Michael Starzinger, 2015-10-13 09:38:43]

https://codereview.chromium.org/1379793004/diff/60001/src/compiler/interprete...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1379793004/diff/60001/src/compiler/interprete...
src/compiler/interpreter-assembler.cc:341: args[3] = LoadObjectField(function,
JSFunction::kContextOffset);
Hmm, I think loading the function context here is bogus, this is done during the
call dispatch (i.e. in this case by Builtins::Generate_CallFunction). There is
the case where the call fails (e.g. calling a non-Callable) which can throw an
exception, the exception would then come out of the function context instead out
of the current context. I think this is dangerous. I think using GetContext()
here is the right thing. Is anything failing for you when you do that?

https://codereview.chromium.org/1379793004/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1379793004/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:63: 
nit: Two empty newlines.

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

https://codereview.chromium.org/1379793004/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:22: // change the current {scope} and
{context} during visitation.
nit: Comment looks outdated. Doesn't really track {scope} and also tracks
{context_register} instead of {context} IIUC.

https://codereview.chromium.org/1379793004/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:154:
builder()->set_context_count(info->num_heap_slots() > 0 ? 1 : 0);
How hard will it be to determine in advance how deep the longest context chain
is? AFAICT we don't have that information, or do we?

[rmcilroy, 2015-10-13 10:35:22]

https://codereview.chromium.org/1379793004/diff/60001/src/compiler/interprete...
File src/compiler/interpreter-assembler.cc (right):

https://codereview.chromium.org/1379793004/diff/60001/src/compiler/interprete...
src/compiler/interpreter-assembler.cc:341: args[3] = LoadObjectField(function,
JSFunction::kContextOffset);
On 2015/10/13 09:38:43, Michael Starzinger wrote:
> Hmm, I think loading the function context here is bogus, this is done during
the
> call dispatch (i.e. in this case by Builtins::Generate_CallFunction). There is
> the case where the call fails (e.g. calling a non-Callable) which can throw an
> exception, the exception would then come out of the function context instead
out
> of the current context. I think this is dangerous. I think using GetContext()
> here is the right thing. Is anything failing for you when you do that?

Yes, your right, this should be GetContext(). I had problems previously and
thought it was because I was passing the wrong context to the function since TF
loads the function's context before calling it correctly, but I didn't realize
that the builtin did this for us. Changed back to GetContext, thanks for the
spot!

https://codereview.chromium.org/1379793004/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1379793004/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:63: 
On 2015/10/13 09:38:43, Michael Starzinger wrote:
> nit: Two empty newlines.

Done.

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

https://codereview.chromium.org/1379793004/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:22: // change the current {scope} and
{context} during visitation.
On 2015/10/13 09:38:43, Michael Starzinger wrote:
> nit: Comment looks outdated. Doesn't really track {scope} and also tracks
> {context_register} instead of {context} IIUC.

Done.

https://codereview.chromium.org/1379793004/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:154:
builder()->set_context_count(info->num_heap_slots() > 0 ? 1 : 0);
On 2015/10/13 09:38:43, Michael Starzinger wrote:
> How hard will it be to determine in advance how deep the longest context chain
> is? AFAICT we don't have that information, or do we?

Yeah, we don't have that information yet. I'll need to add this support to the
parser before we can do anything with nested context chains. I'm hoping it
shouldn't be to tricky though since the parser knows when it needs to push and
pop contexts.

[Michael Starzinger, 2015-10-13 11:12:13]

LGTM.

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

https://codereview.chromium.org/1379793004/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:154:
builder()->set_context_count(info->num_heap_slots() > 0 ? 1 : 0);
On 2015/10/13 10:35:22, rmcilroy wrote:
> On 2015/10/13 09:38:43, Michael Starzinger wrote:
> > How hard will it be to determine in advance how deep the longest context
chain
> > is? AFAICT we don't have that information, or do we?
> 
> Yeah, we don't have that information yet. I'll need to add this support to the
> parser before we can do anything with nested context chains. I'm hoping it
> shouldn't be to tricky though since the parser knows when it needs to push and
> pop contexts.

Acknowledged. Not sure how hard/easy this will be, more thinking/discussion
required. But let's defer that to another CL to unblock this one.

[rmcilroy, 2015-10-13 12:23:31]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-10-13 12:23:32]

The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org
Link to the patchset: https://codereview.chromium.org/1379793004/#ps80001
(title: "Review comments")

[commit-bot: I haz the power, 2015-10-13 12:23:40]

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

[commit-bot: I haz the power, 2015-10-13 12:34:02]

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

[commit-bot: I haz the power, 2015-10-13 12:34:03]

Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/10611)

[rmcilroy, 2015-10-13 12:47:18]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-10-13 12:47:18]

The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1379793004/#ps100001
(title: "Fix interpreter-assembler-unittest")

[commit-bot: I haz the power, 2015-10-13 12:47:33]

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

[commit-bot: I haz the power, 2015-10-13 13:09:53]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-10-13 13:10:12]

Patchset 5 (id:??) landed as
https://crrev.com/c0185b7d984419eb11dd290476c62deb977bfa9f
Cr-Commit-Position: refs/heads/master@{#31238}