Filter out synthetic variables from with scopes

Filter out synthetic variables from with scopes

This patch ensures that variables like .new_target aren't overwritable
using with scopes. It does this by ensuring that scope analysis does
not consider with scopes (or eval scopes) for such 'synthetic variables',
similarly to how the 'this' variable was already handled.
The patch also adds a DCHECK for the dynamic parallel to this case,
replacing a previous unreachable path for a particular instance.

BUG=v8:5405
Committed: https://crrev.com/dcd61b90207adc8cc068c2b86c9f5a9d4e54999f
Cr-Commit-Position: refs/heads/master@{#39567}

[Dan Ehrenberg, 2016-09-19 18:45:27]

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

[commit-bot: I haz the power, 2016-09-19 18:45:36]

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

[Dan Ehrenberg, 2016-09-19 18:50:33]

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

[commit-bot: I haz the power, 2016-09-19 18:50:37]

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-09-19 19:03:27]

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

[commit-bot: I haz the power, 2016-09-19 19:03:28]

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

[Dan Ehrenberg, 2016-09-19 19:30:52]

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

[commit-bot: I haz the power, 2016-09-19 19:30:57]

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-09-19 19:39:27]

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

[commit-bot: I haz the power, 2016-09-19 19:39:27]

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

[Dan Ehrenberg, 2016-09-19 19:41:10]

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

[commit-bot: I haz the power, 2016-09-19 19:41:19]

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

[Dan Ehrenberg, 2016-09-19 19:57:36]

littledan@chromium.org changed reviewers:
+ adamk@chromium.org

[commit-bot: I haz the power, 2016-09-19 19:59:49]

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

[commit-bot: I haz the power, 2016-09-19 19:59:50]

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/13003)
  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...)

[adamk, 2016-09-19 20:01:26]

https://codereview.chromium.org/2353623002/diff/60001/src/ast/scopeinfo.cc
File src/ast/scopeinfo.cc (right):

https://codereview.chromium.org/2353623002/diff/60001/src/ast/scopeinfo.cc#ne...
src/ast/scopeinfo.cc:621: bool Scope::VariableIsSynthetic(const AstRawString*
name) {
Yikes, I think this should be in scopes.cc. If you want to have them in the same
file, they should be in the same class, in which case I think putting them both
in Scope makes sense (as public static methods).

https://codereview.chromium.org/2353623002/diff/60001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/2353623002/diff/60001/src/contexts.cc#newcode233
src/contexts.cc:233: DCHECK(!ScopeInfo::VariableIsSynthetic(*name));
Looks like this is failing?

[Toon Verwaest, 2016-09-19 20:01:49]

verwaest@chromium.org changed reviewers:
+ verwaest@chromium.org

[Toon Verwaest, 2016-09-19 20:01:50]

https://codereview.chromium.org/2353623002/diff/60001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2353623002/diff/60001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1451: if (Scope::VariableIsSynthetic(var->raw_name())) return
var;
I'd be much in favor of having specialized scope resolution for those variables,
done on-the-fly when dealing with those specific vars. This just slows down
scope resolution for al other vars unnecessarily.

[Dan Ehrenberg, 2016-09-20 17:22:13]

https://codereview.chromium.org/2353623002/diff/60001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2353623002/diff/60001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1451: if (Scope::VariableIsSynthetic(var->raw_name())) return
var;
On 2016/09/19 at 20:01:50, Toon Verwaest wrote:
> I'd be much in favor of having specialized scope resolution for those
variables, done on-the-fly when dealing with those specific vars. This just
slows down scope resolution for al other vars unnecessarily.

I looked into some ways to mitigate the performance regression here in a more
limited way, without a separate scope resolution mechanism, but it seems like it
would result in at least reading another flag/having a conditional per variable
at some point or other.

What if we backed out this portion of the patch, as it hurts performance without
being what we want for the long-term structure, but left in the context part as
a quick, hopefully temporary fix for the new.target case? The context code
change shouldn't hurt performance much (I don't think we should care about with
getting slightly slower). For the promise case, I'm working on switching that to
directly using temporaries at https://codereview.chromium.org/2359513002 , but
new.target is a little tougher because it is captured by arrow functions.

[Dan Ehrenberg, 2016-09-20 21:40:48]

What do you think of this minimal fix for new.target while a better solution is
developed for a more static mechanism for scoping synthetic variables? I don't
think it should have significant performance effects (with scopes are already
slow, with unscopables, etc).

[Dan Ehrenberg, 2016-09-20 21:40:53]

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

[commit-bot: I haz the power, 2016-09-20 21:41:04]

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

[adamk, 2016-09-20 22:02:52]

FWIW this minimal fix lgtm; Toon, thoughts?

[commit-bot: I haz the power, 2016-09-20 22:05:35]

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

[commit-bot: I haz the power, 2016-09-20 22:05:36]

Dry run: This issue passed the CQ dry run.

[Toon Verwaest, 2016-09-20 22:08:31]

Lgtm thanks. Surprised to see this handled there as it should never happen.

[Dan Ehrenberg, 2016-09-20 22:11:41]

The CQ bit was checked by littledan@chromium.org

[commit-bot: I haz the power, 2016-09-20 22:11:44]

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-09-20 22:14:04]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-20 22:15:08]

Description was changed from

==========
Filter out synthetic variables from with scopes

This patch ensures that variables like .new_target aren't overwritable
using with scopes. It does this by ensuring that scope analysis does
not consider with scopes (or eval scopes) for such 'synthetic variables',
similarly to how the 'this' variable was already handled.
The patch also adds a DCHECK for the dynamic parallel to this case,
replacing a previous unreachable path for a particular instance.

BUG=v8:5405
==========

to

==========
Filter out synthetic variables from with scopes

This patch ensures that variables like .new_target aren't overwritable
using with scopes. It does this by ensuring that scope analysis does
not consider with scopes (or eval scopes) for such 'synthetic variables',
similarly to how the 'this' variable was already handled.
The patch also adds a DCHECK for the dynamic parallel to this case,
replacing a previous unreachable path for a particular instance.

BUG=v8:5405

Committed: https://crrev.com/dcd61b90207adc8cc068c2b86c9f5a9d4e54999f
Cr-Commit-Position: refs/heads/master@{#39567}
==========

[commit-bot: I haz the power, 2016-09-20 22:15:10]

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