[fullcodegen] Remove deprecated support for lookup variables, eval and with.

[fullcodegen] Remove deprecated support for lookup variables, eval and with.

This code is no longer used by full-codegen since all functions which use with statments,
call eval or have lookup variable access now go through Ignition first.

BUG=v8:5657
Committed: https://crrev.com/4c14bbf97b4c7e48542836335b969c3034bfcc17
Cr-Commit-Position: refs/heads/master@{#41276}

[rmcilroy, 2016-11-21 15:19:19]

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

[commit-bot: I haz the power, 2016-11-21 15:19:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 15:33:05]

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

[commit-bot: I haz the power, 2016-11-21 15:33:05]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[rmcilroy, 2016-11-21 17:15:20]

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

[commit-bot: I haz the power, 2016-11-21 17:15:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 17:37:38]

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

[commit-bot: I haz the power, 2016-11-21 17:37:39]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[rmcilroy, 2016-11-21 22:59:42]

Patchset #3 (id:40001) has been deleted

[rmcilroy, 2016-11-22 14:06:31]

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

[commit-bot: I haz the power, 2016-11-22 14:06:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 14:25:50]

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

[commit-bot: I haz the power, 2016-11-22 14:25:51]

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

[rmcilroy, 2016-11-22 21:05:50]

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

[commit-bot: I haz the power, 2016-11-22 21:05:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 22:34:32]

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

[commit-bot: I haz the power, 2016-11-22 22:34:33]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-11-23 09:01:24]

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

[rmcilroy, 2016-11-23 09:01:24]

Michi, PTAL, thanks

[Michael Starzinger, 2016-11-23 10:15:48]

LGTM. Nice, just nits.

https://codereview.chromium.org/2514393002/diff/80001/src/ast/ast-numbering.cc
File src/ast/ast-numbering.cc (right):

https://codereview.chromium.org/2514393002/diff/80001/src/ast/ast-numbering.c...
src/ast/ast-numbering.cc:536:
DisableFullCodegenAndCrankshaft(kFunctionCallsEval);
Just out of curiosity, are there cases where this is not covered by the
{scope->calls_eval()} predicate on the overall function literal? Because this
could at first glance indicate a bug in scope analysis.

https://codereview.chromium.org/2514393002/diff/80001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/2514393002/diff/80001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.h:330: // Builders for dynamic variable loads and
stores.
nit: Implementation of these two methods is gone, lets also remove the
declaration.

https://codereview.chromium.org/2514393002/diff/80001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.h:387: Node* TryLoadDynamicVariable(Variable*
variable, Handle<String> name,
nit: Implementation is gone, lets drop the declaration.

https://codereview.chromium.org/2514393002/diff/80001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/2514393002/diff/80001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1672: if (expr->is_possibly_eval())
UNREACHABLE();
nit: Why not just [D]CHECK(!expr->is_possibly_eval()) here?

[rmcilroy, 2016-11-23 13:48:48]

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

[rmcilroy, 2016-11-23 13:48:51]

https://codereview.chromium.org/2514393002/diff/80001/src/ast/ast-numbering.cc
File src/ast/ast-numbering.cc (right):

https://codereview.chromium.org/2514393002/diff/80001/src/ast/ast-numbering.c...
src/ast/ast-numbering.cc:536:
DisableFullCodegenAndCrankshaft(kFunctionCallsEval);
On 2016/11/23 10:15:48, Michael Starzinger wrote:
> Just out of curiosity, are there cases where this is not covered by the
> {scope->calls_eval()} predicate on the overall function literal? Because this
> could at first glance indicate a bug in scope analysis.

Yeah this didn't work without it. If I remember correctly, it was when an eval
was within a let block scope. The scope->inner_scope_calls_eval() would be set,
but not just scope->calls_eval(). We can't use inner_scope_calls_eval, since
that would also go through function scopes, but I'm assuming the
scope->calls_eval() is only meant to register if that particular scope calls
eval (not inner block or with scopes).

https://codereview.chromium.org/2514393002/diff/80001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/2514393002/diff/80001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.h:330: // Builders for dynamic variable loads and
stores.
On 2016/11/23 10:15:48, Michael Starzinger wrote:
> nit: Implementation of these two methods is gone, lets also remove the
> declaration.

Opps, done.

https://codereview.chromium.org/2514393002/diff/80001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.h:387: Node* TryLoadDynamicVariable(Variable*
variable, Handle<String> name,
On 2016/11/23 10:15:48, Michael Starzinger wrote:
> nit: Implementation is gone, lets drop the declaration.

Done.

https://codereview.chromium.org/2514393002/diff/80001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/2514393002/diff/80001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1672: if (expr->is_possibly_eval())
UNREACHABLE();
On 2016/11/23 10:15:48, Michael Starzinger wrote:
> nit: Why not just [D]CHECK(!expr->is_possibly_eval()) here?

Done.

[commit-bot: I haz the power, 2016-11-23 13:48:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Michael Starzinger, 2016-11-23 14:11:05]

https://codereview.chromium.org/2514393002/diff/80001/src/ast/ast-numbering.cc
File src/ast/ast-numbering.cc (right):

https://codereview.chromium.org/2514393002/diff/80001/src/ast/ast-numbering.c...
src/ast/ast-numbering.cc:536:
DisableFullCodegenAndCrankshaft(kFunctionCallsEval);
On 2016/11/23 13:48:50, rmcilroy wrote:
> On 2016/11/23 10:15:48, Michael Starzinger wrote:
> > Just out of curiosity, are there cases where this is not covered by the
> > {scope->calls_eval()} predicate on the overall function literal? Because
this
> > could at first glance indicate a bug in scope analysis.
> 
> Yeah this didn't work without it. If I remember correctly, it was when an eval
> was within a let block scope. The scope->inner_scope_calls_eval() would be
set,
> but not just scope->calls_eval(). We can't use inner_scope_calls_eval, since
> that would also go through function scopes, but I'm assuming the
> scope->calls_eval() is only meant to register if that particular scope calls
> eval (not inner block or with scopes).

I briefly talked with Toon about this and he agrees with your assessment and
this is WAI for the scope analysis. I would vote however to remove the check for
{scope->calls_eval()} below in line 593, because it is covered by this new check
and is somewhat confusing IMHO. WDYT?

[commit-bot: I haz the power, 2016-11-23 14:15:48]

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

[commit-bot: I haz the power, 2016-11-23 14:15:49]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-11-24 13:58:37]

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

[rmcilroy, 2016-11-24 13:58:46]

On 2016/11/23 14:11:05, Michael Starzinger wrote:
> https://codereview.chromium.org/2514393002/diff/80001/src/ast/ast-numbering.cc
> File src/ast/ast-numbering.cc (right):
> 
>
https://codereview.chromium.org/2514393002/diff/80001/src/ast/ast-numbering.c...
> src/ast/ast-numbering.cc:536:
> DisableFullCodegenAndCrankshaft(kFunctionCallsEval);
> On 2016/11/23 13:48:50, rmcilroy wrote:
> > On 2016/11/23 10:15:48, Michael Starzinger wrote:
> > > Just out of curiosity, are there cases where this is not covered by the
> > > {scope->calls_eval()} predicate on the overall function literal? Because
> this
> > > could at first glance indicate a bug in scope analysis.
> > 
> > Yeah this didn't work without it. If I remember correctly, it was when an
eval
> > was within a let block scope. The scope->inner_scope_calls_eval() would be
> set,
> > but not just scope->calls_eval(). We can't use inner_scope_calls_eval, since
> > that would also go through function scopes, but I'm assuming the
> > scope->calls_eval() is only meant to register if that particular scope calls
> > eval (not inner block or with scopes).
> 
> I briefly talked with Toon about this and he agrees with your assessment and
> this is WAI for the scope analysis. I would vote however to remove the check
for
> {scope->calls_eval()} below in line 593, because it is covered by this new
check
> and is somewhat confusing IMHO. WDYT?

Sounds good, done.

[commit-bot: I haz the power, 2016-11-24 13:58:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 14:29:50]

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

[commit-bot: I haz the power, 2016-11-24 14:29:51]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-11-24 18:13:45]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2016-11-24 18:13:45]

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/2514393002/#ps140001
(title: "Address comment")

[commit-bot: I haz the power, 2016-11-24 18:13:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 18:15:32]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480011225171210,
"parent_rev": "21cd59056e4ba29c199a745305ec55da9c68d011", "commit_rev":
"658536952ec05508b359fb58016a64e06f943953"}

[commit-bot: I haz the power, 2016-11-24 18:15:38]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-11-24 18:16:14]

Description was changed from

==========
[fullcodegen] Remove deprecated support for lookup variables, eval and with.

This code is no longer used by full-codegen since all functions which use with
statments,
call eval or have lookup variable access now go through Ignition first.

BUG=v8:5657
==========

to

==========
[fullcodegen] Remove deprecated support for lookup variables, eval and with.

This code is no longer used by full-codegen since all functions which use with
statments,
call eval or have lookup variable access now go through Ignition first.

BUG=v8:5657

Committed: https://crrev.com/4c14bbf97b4c7e48542836335b969c3034bfcc17
Cr-Commit-Position: refs/heads/master@{#41276}
==========

[commit-bot: I haz the power, 2016-11-24 18:16:15]

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