Don't record that eval calls in inner scopes in script scopes

Don't record that eval calls in inner scopes in script scopes

When we parse the top-level lazily, we don't get to see eval calls in
inner scopes anyway. By never recording them, we make sure that the
script scope ends up looking the same, no matter how we parsed it.

BUG=v8:5215
R=marja@chromium.org,adamk@chromium.org

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

[jochen (gone - plz use gerrit), 2016-09-06 15:47:12]

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

[commit-bot: I haz the power, 2016-09-06 15:47:22]

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-06 16:33:45]

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

[commit-bot: I haz the power, 2016-09-06 16:33:45]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-09-06 16:37:54]

(still working on the explanation why this is correct)

[adamk, 2016-09-06 17:10:00]

This seems like it should be fine, since all global variables (both global
object properties and global lexicals) already have to be available dynamically.

Can you write a test-parsing test that covers this, though? Or is there some
other behavioral test that actually fails if we have two "different-looking"
script scopes?

[jochen (gone - plz use gerrit), 2016-09-06 17:47:38]

On 2016/09/06 at 17:10:00, adamk wrote:
> This seems like it should be fine, since all global variables (both global
object properties and global lexicals) already have to be available dynamically.
> 
> Can you write a test-parsing test that covers this, though? Or is there some
other behavioral test that actually fails if we have two "different-looking"
script scopes?

in https://codereview.chromium.org/2271993002 I chain scope infos together and
add DCHECKs that compare the context chain to the scope info chain, and it fails
because of this on mjsunit/es6/block-const-assign 

I think I got figured out meanwhile what happens there.

In the first stress run, we use CompileTopLevel to compile the script which
generates a scope info without seeing eval
Then we lazily compile Test, so the scope info installed on the
SharedFunctionInfo for test points to the scope info from above

Then in the second stress run, we mark the top level script for optimization
because of the always on, so we compile the script with GetOptimizedCode which
parses eagerly, and therefore creates a scope info for the top level that saw
eval on the inner scopes.

Now if we later try to optimize Test, the context chain is build from the scope
info created during eager parsing, but the scope info chain that comes from the
shared function info is the one that points at the scope info created in the
first stress run.

The difference in scope infos that saw eval and ones that didn't is that
variables are marked as kMaybeAssigned

[adamk, 2016-09-06 17:49:14]

On 2016/09/06 17:47:38, jochen wrote:
> On 2016/09/06 at 17:10:00, adamk wrote:
> > This seems like it should be fine, since all global variables (both global
> object properties and global lexicals) already have to be available
dynamically.
> > 
> > Can you write a test-parsing test that covers this, though? Or is there some
> other behavioral test that actually fails if we have two "different-looking"
> script scopes?
> 
> in https://codereview.chromium.org/2271993002 I chain scope infos together and
> add DCHECKs that compare the context chain to the scope info chain, and it
fails
> because of this on mjsunit/es6/block-const-assign 

Ok, if this is covered in a followup the logic lgtm.

> I think I got figured out meanwhile what happens there.
> 
> In the first stress run, we use CompileTopLevel to compile the script which
> generates a scope info without seeing eval
> Then we lazily compile Test, so the scope info installed on the
> SharedFunctionInfo for test points to the scope info from above
> 
> Then in the second stress run, we mark the top level script for optimization
> because of the always on, so we compile the script with GetOptimizedCode which
> parses eagerly, and therefore creates a scope info for the top level that saw
> eval on the inner scopes.
> 
> Now if we later try to optimize Test, the context chain is build from the
scope
> info created during eager parsing, but the scope info chain that comes from
the
> shared function info is the one that points at the scope info created in the
> first stress run.
> 
> The difference in scope infos that saw eval and ones that didn't is that
> variables are marked as kMaybeAssigned

Hmm, I don't know much about kMaybeAssigned, but I'd think that all global vars
need to be marked as such.

[jochen (gone - plz use gerrit), 2016-09-06 17:54:00]

On 2016/09/06 at 17:49:14, adamk wrote:
> On 2016/09/06 17:47:38, jochen wrote:
> > On 2016/09/06 at 17:10:00, adamk wrote:
> > > This seems like it should be fine, since all global variables (both global
> > object properties and global lexicals) already have to be available
dynamically.
> > > 
> > > Can you write a test-parsing test that covers this, though? Or is there
some
> > other behavioral test that actually fails if we have two "different-looking"
> > script scopes?
> > 
> > in https://codereview.chromium.org/2271993002 I chain scope infos together
and
> > add DCHECKs that compare the context chain to the scope info chain, and it
fails
> > because of this on mjsunit/es6/block-const-assign 
> 
> Ok, if this is covered in a followup the logic lgtm.
> 
> > I think I got figured out meanwhile what happens there.
> > 
> > In the first stress run, we use CompileTopLevel to compile the script which
> > generates a scope info without seeing eval
> > Then we lazily compile Test, so the scope info installed on the
> > SharedFunctionInfo for test points to the scope info from above
> > 
> > Then in the second stress run, we mark the top level script for optimization
> > because of the always on, so we compile the script with GetOptimizedCode
which
> > parses eagerly, and therefore creates a scope info for the top level that
saw
> > eval on the inner scopes.
> > 
> > Now if we later try to optimize Test, the context chain is build from the
scope
> > info created during eager parsing, but the scope info chain that comes from
the
> > shared function info is the one that points at the scope info created in the
> > first stress run.
> > 
> > The difference in scope infos that saw eval and ones that didn't is that
> > variables are marked as kMaybeAssigned
> 
> Hmm, I don't know much about kMaybeAssigned, but I'd think that all global
vars need to be marked as such.

That I don't know. Maybe Marja can shed some light on this? :)

[marja, 2016-09-07 08:26:16]

Idk why kMaybeAssigned :(

But this makes some sense, since how the global variables are allocated doesn't
depend on whether an inner scope calls eval or not. So I think this code is
correct (but you should add a comment) - lgtm.

[jochen (gone - plz use gerrit), 2016-09-07 08:27:48]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2016-09-07 08:27:59]

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-07 08:29:57]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-07 08:30:28]

Description was changed from

==========
Don't record that eval calls in inner scopes in script scopes

When we parse the top-level lazily, we don't get to see eval calls in
inner scopes anyway. By never recording them, we make sure that the
script scope ends up looking the same, no matter how we parsed it.

BUG=v8:5215
R=marja@chromium.org,adamk@chromium.org
==========

to

==========
Don't record that eval calls in inner scopes in script scopes

When we parse the top-level lazily, we don't get to see eval calls in
inner scopes anyway. By never recording them, we make sure that the
script scope ends up looking the same, no matter how we parsed it.

BUG=v8:5215
R=marja@chromium.org,adamk@chromium.org

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

[commit-bot: I haz the power, 2016-09-07 08:30:29]

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

[marja, 2016-09-07 08:41:50]

Why no comment added?

[jochen (gone - plz use gerrit), 2016-09-07 08:50:36]

On 2016/09/07 at 08:41:50, marja wrote:
> Why no comment added?

eh, my mistake

[jochen (gone - plz use gerrit), 2016-09-07 08:54:07]

https://codereview.chromium.org/2318953003