Eagerly declare eval scopes, even for sloppy scopes

Eagerly declare eval scopes, even for sloppy scopes

R=marja@chromium.org, mstarzinger@chromium.org
LOG=N
BUG=N

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

[wingo, 2015-04-15 13:59:24]

The parser part of this seems straightforward to me, but the runtime fallout was
not.  I don't actually know why it worked before.  However without the patch to
runtime-scopes.cc, DeclareGlobals would be called with a null object ("holder"
would be null).  Perhaps this was the "changes all over the tree" that Michael
mentioned here:

 
https://chromium.googlesource.com/v8/v8.git/+/79a98de9f7f8f42d3404ad513bb4b50...

So I sheepishly note that the attached patch does pass the test suite, though I
don't know how it did before.  WDYT?

[marja, 2015-04-15 14:05:39]

I have no idea whether the changes to runtime-scopes.cc are correct, somebody
who has should review.

[arv (Not doing code reviews), 2015-04-15 15:40:14]

arv@chromium.org changed reviewers:
+ dslomov@chromium.org

[arv (Not doing code reviews), 2015-04-15 15:40:15]

Maybe Dmitry knows?

[Michael Starzinger, 2015-04-15 15:57:19]

The variable binding and scope analysis has significantly changed over the past
three years, since I introduced this this hack. So it might very well be that
the "all over the code" from back then turned into "nothing at all" by now. As
for the changes to the parser, that is exactly what my TODO was referring to and
I like the parser changes. Need to dig into the DeclareLookupSlot part of the
change further to understand it.

[Michael Starzinger, 2015-04-15 16:02:09]

Hint: Unless you already did so, also run test262 (release mode suffices) to get
additional coverage.

[wingo, 2015-04-16 10:05:52]

On 2015/04/15 16:02:09, Michael Starzinger wrote:
> Hint: Unless you already did so, also run test262 (release mode suffices) to
get
> additional coverage.

Good tip.  Happily test262 is clean.

I was thinking and I have some idea about what changed.  Before, sloppy direct
eval in a script code would not cause a new eval scope to be created.  With this
patch, there is an eval scope created.  This means that Scope::LookupLocal fails
for global variables, instead causing Scope::ResolveVariables to process the
declared variable as UNBOUND_EVAL_SHADOWED, which goes down the
DeclareLookupSlot path instead of the DeclareGlobals path -- and hence the
change to runtime-scopes.cc.

[Michael Starzinger, 2015-04-16 10:49:18]

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

[Michael Starzinger, 2015-04-16 10:49:19]

LGTM from my end. I asked Toon to look double-check Runtime_DeclareLookupSlot as
well, please wait for his response as well.

[wingo, 2015-04-17 11:51:03]

On 2015/04/16 10:49:19, Michael Starzinger wrote:
> LGTM from my end. I asked Toon to look double-check Runtime_DeclareLookupSlot
as
> well, please wait for his response as well.

A gentle ping to Toon then :-)

[wingo, 2015-04-20 08:06:31]

wingo@igalia.com changed reviewers:
- dslomov@chromium.org, marja@chromium.org

[wingo, 2015-04-20 08:06:31]

wingo@igalia.com changed required reviewers:
+ verwaest@chromium.org

[wingo, 2015-04-20 08:07:11]

On 2015/04/17 11:51:03, wingo wrote:
> On 2015/04/16 10:49:19, Michael Starzinger wrote:
> > LGTM from my end. I asked Toon to look double-check
Runtime_DeclareLookupSlot
> as
> > well, please wait for his response as well.
> 
> A gentle ping to Toon then :-)

Updated to list Toon as a required reviewer and trim the rest of the reviewer
list.

[Toon Verwaest, 2015-04-20 10:59:14]

https://codereview.chromium.org/1085263003/diff/1/src/runtime/runtime-scopes.cc
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/1085263003/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:247: context_arg->extension()->IsJSGlobalObject())
{
If the holder is a JSGlobalObject, shouldn't it == context_arg->extension()
since we DONT_FOLLOW_CHAINS? Am I missing something?

[wingo, 2015-04-20 11:23:18]

https://codereview.chromium.org/1085263003/diff/1/src/runtime/runtime-scopes.cc
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/1085263003/diff/1/src/runtime/runtime-scopes....
src/runtime/runtime-scopes.cc:247: context_arg->extension()->IsJSGlobalObject())
{
On 2015/04/20 10:59:14, Toon Verwaest wrote:
> If the holder is a JSGlobalObject, shouldn't it == context_arg->extension()
> since we DONT_FOLLOW_CHAINS? Am I missing something?

Thanks for looking into this.  I think the issue I was seeing was that "holder"
was NULL and attributes were ABSENT, yet the context_arg had an extension which
was the global object.  This case was not getting exercised before, AFAICT,
because of the scoping resolution issues I mentioned in an earlier comment.

[wingo, 2015-04-21 08:56:33]

On 2015/04/20 11:23:18, wingo wrote:
>
https://codereview.chromium.org/1085263003/diff/1/src/runtime/runtime-scopes.cc
> File src/runtime/runtime-scopes.cc (right):
> 
>
https://codereview.chromium.org/1085263003/diff/1/src/runtime/runtime-scopes....
> src/runtime/runtime-scopes.cc:247:
context_arg->extension()->IsJSGlobalObject())
> {
> On 2015/04/20 10:59:14, Toon Verwaest wrote:
> > If the holder is a JSGlobalObject, shouldn't it == context_arg->extension()
> > since we DONT_FOLLOW_CHAINS? Am I missing something?
> 
> Thanks for looking into this.  I think the issue I was seeing was that
"holder"
> was NULL and attributes were ABSENT, yet the context_arg had an extension
which
> was the global object.  This case was not getting exercised before, AFAICT,
> because of the scoping resolution issues I mentioned in an earlier comment.

Just realized my message might not be clear -- the above explains why the patch
is the way it is.  It seems fine to me but I'm waiting on Toon :)

[wingo, 2015-04-23 08:07:57]

Landing following offline discussion with Michael.  I'm happy to follow up with
Toon when he gets back :)

[wingo, 2015-04-23 08:08:11]

wingo@igalia.com changed required reviewers:
- verwaest@chromium.org

[wingo, 2015-04-23 08:08:22]

The CQ bit was checked by wingo@igalia.com

[commit-bot: I haz the power, 2015-04-23 08:08:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1085263003/1

[commit-bot: I haz the power, 2015-04-23 08:09:34]

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

[commit-bot: I haz the power, 2015-04-23 08:09:35]

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

[wingo, 2015-04-23 08:46:33]

The CQ bit was checked by wingo@igalia.com

[wingo, 2015-04-23 08:46:34]

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/1085263003/#ps20001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-23 08:46:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1085263003/20001

[commit-bot: I haz the power, 2015-04-23 09:27:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-23 09:28:09]

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

[marja, 2015-04-24 05:55:30]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1082013003/ by marja@chromium.org.

The reason for reverting is: Regresses CodeLoad (crbug.com/480774)..