Resolve references to "this" the same way as normal variables

Resolve references to "this" the same way as normal variables

Make the parser handle references to "this" as unresolved variables, so the
same logic as for the rest of function parameters is used for the receiver.
Minor additions to the code generation handle copying the receiver to the
context, along with the rest of the function parameters.

Based on work by Adrian Perez de Castro <aperez@igalia.com>;.

BUG=
LOG=N

Committed: https://crrev.com/18619d355192e2699203d12d9ebb9caea107b693
Cr-Commit-Position: refs/heads/master@{#28236}

[wingo, 2015-04-21 16:02:06]

Patchset #1 (id:1) has been deleted

[wingo, 2015-04-21 16:09:49]

This is a WIP to resolve "this" using the normal variable resolution mechanism. 
It passes all tests except those involving turbofan.  It shouldn't introduce a
functional change.  A patch to make arrow functions not declare this will be a
small followup.

Some notes:

 * Only a few tests fail with TF enabled, and only one compiler test.  Probably
the compiler test is the easiest way to reproduce:

     tools/run-tests.py --arch-and-mode=x64.release test-run-jscalls/EvalCall

 * Just x64 for now

 * Probably we will be able to clean up ResolvePossiblyDirectEvalCall to not
pass the receiver to the eval stub, since after this patch, eval doesn't declare
a "this" binding.  In that way the "LookupThis()" method of Scope might be a
temporary hack.

Some thoughts regarding TF would be appreciated.  TIA :)

[wingo, 2015-04-21 16:10:36]

wingo@igalia.com changed reviewers:
+ rossberg@chromium.org

[wingo, 2015-04-21 16:12:09]

Another note -- a side effect of this patch will be that "this" can end up being
allocated in a context -- a function's context, or a script context.  It would
never be in the context before.

[wingo, 2015-04-21 16:15:01]

Another note: this depends on https://codereview.chromium.org/1085263003/

[Michael Starzinger, 2015-04-21 18:13:43]

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

[Michael Starzinger, 2015-04-21 18:13:44]

https://codereview.chromium.org/1097283003/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1097283003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:646: if (variable->is_this()) {
AFAICT, this is only needed for patching the receiver to the global proxy in the
method prologue. Please don't "extend" Environment::Bind this way, but rather
adapt BuildPatchReceiverToGlobalProxy accordingly.

https://codereview.chromium.org/1097283003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:669: if (variable->is_this()) {
Likewise.

https://codereview.chromium.org/1097283003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2672: (i == -1) ? info()->scope()->receiver()
: info()->scope()->parameter(i);
This is getting out of hand, please hoist handling of the receiver out into a
separate block (preferably placed at line 2666 above) and rather duplicate
creation of the StoreContext node below.

https://codereview.chromium.org/1097283003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2676: Node* parameter =
NewNode(common()->Parameter(i + 1), graph()->start());
If the receiver parameter has been patched, then using Patameter(0) he is
incorrect and will use the un-patched version.

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

Sheepish confessions follow.  Thanks for the feedback Michael :)

https://codereview.chromium.org/1097283003/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1097283003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:646: if (variable->is_this()) {
On 2015/04/21 18:13:44, Michael Starzinger wrote:
> AFAICT, this is only needed for patching the receiver to the global proxy in
the
> method prologue. Please don't "extend" Environment::Bind this way, but rather
> adapt BuildPatchReceiverToGlobalProxy accordingly.

I had tried to do that, but I couldn't get at environment->values()->at(0), as
it's private.  I realize this is an awful hack and something different should go
in :)  I guess common()->Parameter(0) is the thing to use, then?

https://codereview.chromium.org/1097283003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:669: if (variable->is_this()) {
On 2015/04/21 18:13:44, Michael Starzinger wrote:
> Likewise.

Acknowledged.

https://codereview.chromium.org/1097283003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2676: Node* parameter =
NewNode(common()->Parameter(i + 1), graph()->start());
On 2015/04/21 18:13:44, Michael Starzinger wrote:
> If the receiver parameter has been patched, then using Patameter(0) he is
> incorrect and will use the un-patched version.

I see.  Thanks, I didn't realize that.  Will update with a new patch.

[wingo, 2015-04-22 08:27:12]

Patchset #2 (id:40001) has been deleted

[wingo, 2015-04-22 08:55:50]

New patch passes all tests, thanks to the Michael's expert administration of the
cluestick :)

[Michael Starzinger, 2015-04-22 10:59:22]

Looking good, I like this a lot more now. Just one more suggestion. :)

https://codereview.chromium.org/1097283003/diff/20001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1097283003/diff/20001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:646: if (variable->is_this()) {
On 2015/04/22 08:04:00, wingo wrote:
> On 2015/04/21 18:13:44, Michael Starzinger wrote:
> > AFAICT, this is only needed for patching the receiver to the global proxy in
> the
> > method prologue. Please don't "extend" Environment::Bind this way, but
rather
> > adapt BuildPatchReceiverToGlobalProxy accordingly.
> 
> I had tried to do that, but I couldn't get at environment->values()->at(0), as
> it's private.  I realize this is an awful hack and something different should
go
> in :)  I guess common()->Parameter(0) is the thing to use, then?

Yes, I think for patching purposes Parameter(0) is always correct.

https://codereview.chromium.org/1097283003/diff/60001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1097283003/diff/60001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:475: Node* patched_receiver =
BuildPatchReceiverToGlobalProxy(original_receiver);
Instead of having two call-sites of BuildPatchReceiverToGlobalProxy, could we
instead add a "Node* function_receiver_" field to the AstGraphBuilder? That
would allow us to ...

- initially set that field to Parameter(0) unconditionally
- conditionally patch it if scope->has_this_declaration()
- use that field in AstGraphBuilder::BuildLocalFunctionContext

https://codereview.chromium.org/1097283003/diff/60001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2157: // once ResolvePossibleDirectEval
changes.
Does this mean that the "receiver" input to ResolvePossibleDirectEval will
disappear completely? That would be awesome then, and I can totally live with
this TODO then. :)

[Michael Starzinger, 2015-04-22 11:17:53]

https://codereview.chromium.org/1097283003/diff/60001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1097283003/diff/60001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:475: Node* patched_receiver =
BuildPatchReceiverToGlobalProxy(original_receiver);
On 2015/04/22 10:59:21, Michael Starzinger wrote:
> Instead of having two call-sites of BuildPatchReceiverToGlobalProxy, could we
> instead add a "Node* function_receiver_" field to the AstGraphBuilder? That
> would allow us to ...
> 
> - initially set that field to Parameter(0) unconditionally
> - conditionally patch it if scope->has_this_declaration()
> - use that field in AstGraphBuilder::BuildLocalFunctionContext

Or alternatively pass the patched_receiver as an argument to
AstGraphBuilder::BuildLocalFunctionContext, that might be even cleaner.

[wingo, 2015-04-22 13:05:23]

Fixed TF nits.

I realized that this patch actually excludes arrow functions from
has_this_declaration, so it fixes this scoping for arrow functions, and
therefore needs tests for that.  We will also be able to remove the uses_this /
inner_uses_this flags from Scope; dunno whether to do here or in a followup.

https://codereview.chromium.org/1097283003/diff/60001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1097283003/diff/60001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:475: Node* patched_receiver =
BuildPatchReceiverToGlobalProxy(original_receiver);
On 2015/04/22 11:17:53, Michael Starzinger wrote:
> On 2015/04/22 10:59:21, Michael Starzinger wrote:
> > Instead of having two call-sites of BuildPatchReceiverToGlobalProxy, could
we
> > instead add a "Node* function_receiver_" field to the AstGraphBuilder? That
> > would allow us to ...
> > 
> > - initially set that field to Parameter(0) unconditionally
> > - conditionally patch it if scope->has_this_declaration()
> > - use that field in AstGraphBuilder::BuildLocalFunctionContext
> 
> Or alternatively pass the patched_receiver as an argument to
> AstGraphBuilder::BuildLocalFunctionContext, that might be even cleaner.

A bit squirrely, but done.

https://codereview.chromium.org/1097283003/diff/60001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2157: // once ResolvePossibleDirectEval
changes.
On 2015/04/22 10:59:21, Michael Starzinger wrote:
> Does this mean that the "receiver" input to ResolvePossibleDirectEval will
> disappear completely? That would be awesome then, and I can totally live with
> this TODO then. :)

Yes I think so, and the second return value from ResolvePossibleDirectEval would
also go away, methinks :)

[Michael Starzinger, 2015-04-22 13:26:18]

mstarzinger@chromium.org changed required reviewers:
+ rossberg@chromium.org

[Michael Starzinger, 2015-04-22 13:26:19]

The TF part looks good. Didn't look at the rest, I'll leave that to Andreas.

https://codereview.chromium.org/1097283003/diff/60001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/1097283003/diff/60001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2157: // once ResolvePossibleDirectEval
changes.
On 2015/04/22 13:05:23, wingo wrote:
> On 2015/04/22 10:59:21, Michael Starzinger wrote:
> > Does this mean that the "receiver" input to ResolvePossibleDirectEval will
> > disappear completely? That would be awesome then, and I can totally live
with
> > this TODO then. :)
> 
> Yes I think so, and the second return value from ResolvePossibleDirectEval
would
> also go away, methinks :)

Woot! That would be the awesomez! :)

[rossberg, 2015-04-22 13:40:02]

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

[wingo, 2015-04-22 14:57:04]

Updated CL adds tests for "this" scoping in arrow functions.

[rossberg, 2015-04-22 15:39:32]

LGTM, but Dmitry should perhaps also have a second eyeball on the scoping
matters.

https://codereview.chromium.org/1097283003/diff/80001/src/contexts.cc
File src/contexts.cc (right):

https://codereview.chromium.org/1097283003/diff/80001/src/contexts.cc#newcode261
src/contexts.cc:261: // A with context will never bind "this".
Do we have a test for this?

[adamk, 2015-04-22 15:50:53]

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

[adamk, 2015-04-22 15:50:54]

Just a few drive-by s/CHECK/DCHECK/ nits. Overall looks good.

https://codereview.chromium.org/1097283003/diff/100001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1097283003/diff/100001/src/scopes.h#newcode350
src/scopes.h:350: CHECK_NOT_NULL(receiver_);
These should probably be DCHECKs.

https://codereview.chromium.org/1097283003/diff/100001/src/x64/full-codegen-x...
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1097283003/diff/100001/src/x64/full-codegen-x...
src/x64/full-codegen-x64.cc:216: CHECK_NOT_NULL(var);
Seems unnecessary since receiver() already has this check and the next line will
crash anyway; if you want to keep it please change to DCHECK.

https://codereview.chromium.org/1097283003/diff/100001/src/x64/full-codegen-x...
src/x64/full-codegen-x64.cc:2996: CHECK_NOT_NULL(this_var);
DCHECK

[wingo, 2015-04-23 13:24:44]

On 2015/04/22 15:39:32, rossberg wrote:
> LGTM, but Dmitry should perhaps also have a second eyeball on the scoping
> matters.
> 
> https://codereview.chromium.org/1097283003/diff/80001/src/contexts.cc
> File src/contexts.cc (right):
> 
>
https://codereview.chromium.org/1097283003/diff/80001/src/contexts.cc#newcode261
> src/contexts.cc:261: // A with context will never bind "this".
> Do we have a test for this?

There are a few:

 * tests that reference "this" through a with that has no "this" property
 * tests that reference "this" through a with that has a proxy

This patch adds some tests through "with" objects with "this" properties.  Dunno
if there are more cases to cover, though.

[rossberg, 2015-04-23 13:30:53]

On 2015/04/23 13:24:44, wingo wrote:
> On 2015/04/22 15:39:32, rossberg wrote:
>
https://codereview.chromium.org/1097283003/diff/80001/src/contexts.cc#newcode261
> > src/contexts.cc:261: // A with context will never bind "this".
> > Do we have a test for this?
> 
> There are a few:
> 
>  * tests that reference "this" through a with that has no "this" property
>  * tests that reference "this" through a with that has a proxy
> 
> This patch adds some tests through "with" objects with "this" properties. 
Dunno
> if there are more cases to cover, though.

Yes, the last one was what I was wondering about. Thanks for adding.

[rossberg, 2015-04-23 13:31:45]

https://codereview.chromium.org/1097283003/diff/100001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/arrow-functions-this.js (right):

https://codereview.chromium.org/1097283003/diff/100001/test/mjsunit/harmony/a...
test/mjsunit/harmony/arrow-functions-this.js:26: var withObject = { ['this']:
object }
Nit: you don't need the brackets

[wingo, 2015-04-23 14:16:02]

Updated patch fixes nits.  It also enabled parts of the test that were commented
out now that v8:4056 has landed.  Unfortunately that meant a rebase, so the
interdiffs are trash :/

https://codereview.chromium.org/1097283003/diff/100001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1097283003/diff/100001/src/scopes.h#newcode350
src/scopes.h:350: CHECK_NOT_NULL(receiver_);
On 2015/04/22 15:50:54, adamk wrote:
> These should probably be DCHECKs.

Done.

https://codereview.chromium.org/1097283003/diff/100001/src/x64/full-codegen-x...
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1097283003/diff/100001/src/x64/full-codegen-x...
src/x64/full-codegen-x64.cc:216: CHECK_NOT_NULL(var);
On 2015/04/22 15:50:54, adamk wrote:
> Seems unnecessary since receiver() already has this check and the next line
will
> crash anyway; if you want to keep it please change to DCHECK.

Good catch; was vestigial.  Removed.

https://codereview.chromium.org/1097283003/diff/100001/src/x64/full-codegen-x...
src/x64/full-codegen-x64.cc:2996: CHECK_NOT_NULL(this_var);
On 2015/04/22 15:50:54, adamk wrote:
> DCHECK

Done.

[wingo, 2015-04-23 14:57:12]

Updated patch adds ia32, arm, arm64 implementations.  I believe we are good to
go; PTAL for the codegen changes.

Note that the LookupThis() in the codegens can go away if we fix
ResolvePossiblyDirectEvalCall to not take a receiver.

[wingo, 2015-04-23 15:23:02]

Regarding the failing try-builds -- could it be that try builds and rebased
patches somehow interact poorly?  An arm64.release check passes locally, for
example, as do ia32 and x64 (haven't had time to finish arm).

[wingo, 2015-04-23 15:49:42]

The CQ bit was checked by wingo@igalia.com to run a CQ dry run

[wingo, 2015-04-23 15:49:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/1097283003/#ps140001
(title: "Add ia32, arm, arm64 support")

[commit-bot: I haz the power, 2015-04-23 15:49:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1097283003/140001

[wingo, 2015-04-23 15:50:27]

On 2015/04/23 15:23:02, wingo wrote:
> Regarding the failing try-builds -- could it be that try builds and rebased
> patches somehow interact poorly?  An arm64.release check passes locally, for
> example, as do ia32 and x64 (haven't had time to finish arm).

Giving a go with the CQ dry run.

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

On 2015/04/23 15:50:27, wingo wrote:
> On 2015/04/23 15:23:02, wingo wrote:
> > Regarding the failing try-builds -- could it be that try builds and rebased
> > patches somehow interact poorly?  An arm64.release check passes locally, for
> > example, as do ia32 and x64 (haven't had time to finish arm).
> 
> Giving a go with the CQ dry run.

I see, the try bots seem to run with checks enabled in release mode or
something, and my debug-mode tests were out of date.  Sorry for the noise!

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

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

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

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

[wingo, 2015-04-24 08:13:50]

Updated patchset fixes the context->closure() ==
context->declaration_context()->closure() consistency check for script contexts.
 I wonder how this ever worked before.

Unfortunately this CL relies on the eval scopes CL, which was just rolled out
for perf reasons.  Will revisit after I find a way to reland the eval-scopes
patch.

[wingo, 2015-04-27 16:01:39]

Updated patchset to apply to master.  Unfortunately we are now failing this
test, which oddly is caught only by the mozilla tests:

  eval("with({}) this;")

It seems that DYNAMIC "this" lookups inside eval fail, but static ones succeed. 
Indeed in this example the script has no context and thus there is no "this"
binding in the context chain.  It seems actually that "this" is getting accessed
from the stack in the eval, perhaps via the copy from
ResolvePossiblyDirectEvalCall.  I tried forcing context allocation for "this"
bindings of script code, but that fails to bootstrap; probably there is an
invariant somewhere being violated.

A useful short-term step would be to resolve "this" statically, even within a
"with", which would lead to loading the copied receiver from the eval code's
stack.  Then we could clean up ResolvePossiblyDirectEvalCall.

[arv (Not doing code reviews), 2015-04-27 16:13:34]

On 2015/04/27 16:01:39, wingo wrote:
> Updated patchset to apply to master.  Unfortunately we are now failing this
> test, which oddly is caught only by the mozilla tests:
> 
>   eval("with({}) this;")
> 
> It seems that DYNAMIC "this" lookups inside eval fail, but static ones
succeed. 
> Indeed in this example the script has no context and thus there is no "this"
> binding in the context chain.  It seems actually that "this" is getting
accessed
> from the stack in the eval, perhaps via the copy from
> ResolvePossiblyDirectEvalCall.  I tried forcing context allocation for "this"
> bindings of script code, but that fails to bootstrap; probably there is an
> invariant somewhere being violated.
> 
> A useful short-term step would be to resolve "this" statically, even within a
> "with", which would lead to loading the copied receiver from the eval code's
> stack.  Then we could clean up ResolvePossiblyDirectEvalCall.

I ran into this as I was playing with lexical this a few weeks ago. What happens
with your cl in this case?

with ({this: 42}) {
  print(this);
}

[wingo, 2015-04-27 16:24:33]

On 2015/04/27 16:13:34, arv wrote:
> What happens with your cl in this case?
> 
> with ({this: 42}) {
>   print(this);
> }

It prints the global (to LPT1 :).  The relevant code is in contexts.cc.

[wingo, 2015-04-27 16:26:29]

On 2015/04/27 16:01:39, wingo wrote:
> A useful short-term step would be to resolve "this" statically, even within a
> "with", which would lead to loading the copied receiver from the eval code's
> stack.  Then we could clean up ResolvePossiblyDirectEvalCall.

Updated CL does that, and appears to pass the mozilla test.  I'll kick off some
bots.

[wingo, 2015-04-27 16:36:58]

On 2015/04/27 16:26:29, wingo wrote:
> On 2015/04/27 16:01:39, wingo wrote:
> > A useful short-term step would be to resolve "this" statically, even within
a
> > "with", which would lead to loading the copied receiver from the eval code's
> > stack.

This assessment of how we load "this" is incorrect; I think I was looking at the
code that stores ".result" instead of the code that loaded "this".  A gaffe;
"this" is indeed loaded from the context, it seems.

[wingo, 2015-04-28 08:02:17]

Yesterday Andreas asked me to figure out what was happening
with "this" and "with" in script scopes -- why that mozilla
test was failing before the latest patch.

Of course, before my patch entirely, each "eval" scope
declared a "this" binding, which was propagated to the
compiled code by the caller of eval, either via
Runtime_ResolvePossiblyDirectEval or via the various
%CompileString invocations in v8natives.js.  "This" was
never resolved dynamically -- it always mapped to the
statically declared receiver in the closest declaration
scope, which also mapped to parameter -1.

After this patch, there are (broadly) two cases: in script
context, or in function context.

In function context, use of eval will mark all locals as
needing context allocation, so "this" ends up in the
context, and accesses go there.  Use of "this" within "with"
will mark "this" as needing context allocation.  Everything
works, there is a context object with a "this" binding.

In a script, usage of eval or other dynamic features is not
recorded:

  // Inform the scope that the corresponding code contains an eval call.
  void RecordEvalCall() { if (!is_script_scope()) scope_calls_eval_ = true; }

So "this" not forced to be allocated in a context, in
general.  e.g. on this file:

  $ echo "eval('this')" > /tmp/eval.js
  $ out/x64.debug/d8 --print-scopes /tmp/eval.js
  global { // (0, 14)
    // scope has trivial outer context
    // 1 stack slots
    // temporary vars:
    TEMPORARY .result;  // local[0]
    // local vars:
    VAR this;  // parameter[-1]
    DYNAMIC_GLOBAL eval;  // 
  }
  eval { // (0, 4)
    // scope has trivial outer context
    // scope uses 'this'
    // 1 stack slots
    // temporary vars:
    TEMPORARY .result;  // local[0]
  }

Note that "this" is given stack allocation.  And yet, how
does eval("this") actually work?  The answer: by horrible
accident.  :(((

You might want to finish breakfast, make sure all food is
not at risk of coming back up before reading on.

So, Runtime_CompileString or
Runtime_ResolvePossibleDirectEval end up calling
Compiler::GetFunctionFromEval, which takes a context and a
string.  In script context, the context is usually the
native context, but it could be a script context if there
are context-allocated locals (possibly including "this"
itself).  GetFunctionFromEval ends up calling
Parser::DoParseProgram, indirectly, and that declares a new
script scope, possibly unpacks other nested scopes from the
ScopeInfo, and then nests an eval scope.

The script scope declares a "this" binding.  The eval scope
does not.

What happens next is where it gets bad.

"this" could be in the context, for example if the script
scope contains ()=>this.  Or it could be just on the stack.

If "this" is context-allocated in the script, everything
works as you would think.

If "this" is stack-allocated in the script:

    If eval code uses "this", it resolves to the "this"
    binding in the script scope, which is at parameter[-1].
    There happens to be a copy of "this" passed to the eval
    code, as we mentioned before, so this happens to work --
    even though it thinks it's getting the binding from the
    script.  Horrific.

    Inside "with", in earlier versions of this patchset,
    access to "this" was happening via dynamic lookup.
    Again, this is not the case in master; in master, "this"
    is always resolved statically.  OK.  But without "this"
    in the context, the lookup will fail.  That's the error
    I was getting with mozilla code.

    Arrow functions: normally arrow functions cause "this"
    to be context-allocated in the script, and this works
    fine.  But if the arrow function is in "eval", then
    scope resolution assumes that the variable was
    contexted-allocated in the script (since it's outside
    the arrow function scope), and "()=>this" residualizes
    an access to context slot (depth=0,
    slot=MIN_CONTEXT_SLOTS=4).  But there is no script
    context!  So where does this map?  To slot 4 in the
    native context... which happens to be the global
    proxy... so it just "works" :(((

    In later versions of the patchset, we resolve "this"
    statically even within "with".  That makes the
    eval('with({}) this') case be the same as
    'eval("()=>this")()', and have the same unfortunate
    possible aliasing of the native context.

I am not sure what The Right Solution is here.  It sounds
like making all scripts have context-allocated "this" is the
cleanest thing, but I don't know what the perf impact would
be.  An initial attempt to do this produced bootstrapping
failures.

I would like to plead for applying this patch as is and
fixing these terrible things in follow-ups, but I wonder if
this horrible accidental aliasing would not be exploitable
to gain access to the global object.  In theory "this"
should always be allocated to either parameter[-1] or to
slot 4 in the context, so probably this is not possible.
Hard to make definitive statements here, though.  Your
thoughts very much appreciated!

[arv (Not doing code reviews), 2015-04-28 14:26:18]

Amazing work.

https://codereview.chromium.org/1097283003/diff/200001/src/scopeinfo.cc
File src/scopeinfo.cc (right):

https://codereview.chromium.org/1097283003/diff/200001/src/scopeinfo.cc#newco...
src/scopeinfo.cc:321: name->Equals(*GetIsolate()->factory()->this_string());
Another option would be to use the string ".this"

https://codereview.chromium.org/1097283003/diff/200001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1097283003/diff/200001/src/scopes.h#newcode149
src/scopes.h:149: Variable::Kind kind = Variable::NORMAL) {
Can we keep the position last for consistency.

https://codereview.chromium.org/1097283003/diff/200001/src/scopes.h#newcode360
src/scopes.h:360: is_script_scope();
What about eval scope?

[wingo, 2015-04-28 15:07:42]

https://codereview.chromium.org/1097283003/diff/200001/src/scopeinfo.cc
File src/scopeinfo.cc (right):

https://codereview.chromium.org/1097283003/diff/200001/src/scopeinfo.cc#newco...
src/scopeinfo.cc:321: name->Equals(*GetIsolate()->factory()->this_string());
On 2015/04/28 14:26:18, arv wrote:
> Another option would be to use the string ".this"

Hah, you're right :)  We could actually use any string that can't be a valid
variable name.  Not sure if it would buy us anything besides simplification in
this fairly cold function, though.

https://codereview.chromium.org/1097283003/diff/200001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1097283003/diff/200001/src/scopes.h#newcode149
src/scopes.h:149: Variable::Kind kind = Variable::NORMAL) {
On 2015/04/28 14:26:18, arv wrote:
> Can we keep the position last for consistency.

Acknowledged.

https://codereview.chromium.org/1097283003/diff/200001/src/scopes.h#newcode360
src/scopes.h:360: is_script_scope();
On 2015/04/28 14:26:18, arv wrote:
> What about eval scope?

Doesn't declare a "this" :)  Grep the spec for GetThisBinding.  The only reason
eval scopes used to have a "this" binding was because it was copied from its
value in the outer scope to the eval code.

Given that functions don't have this gnarly this-aliasing bug I mentioned about
script contexts, this means that super calls in arrow functions should be able
to BindThisValue correctly.  But, I haven't fully grokked super calls yet.

[rossberg, 2015-04-28 16:27:04]

On 2015/04/28 08:02:17, wingo wrote:
> Yesterday Andreas asked me to figure out what was happening
> with "this" and "with" in script scopes -- why that mozilla
> test was failing before the latest patch.
> 
> Of course, before my patch entirely, each "eval" scope
> declared a "this" binding, which was propagated to the
> compiled code by the caller of eval, either via
> Runtime_ResolvePossiblyDirectEval or via the various
> %CompileString invocations in v8natives.js.  "This" was
> never resolved dynamically -- it always mapped to the
> statically declared receiver in the closest declaration
> scope, which also mapped to parameter -1.
> 
> After this patch, there are (broadly) two cases: in script
> context, or in function context.
> 
> In function context, use of eval will mark all locals as
> needing context allocation, so "this" ends up in the
> context, and accesses go there.  Use of "this" within "with"
> will mark "this" as needing context allocation.  Everything
> works, there is a context object with a "this" binding.
> 
> In a script, usage of eval or other dynamic features is not
> recorded:
> 
>   // Inform the scope that the corresponding code contains an eval call.
>   void RecordEvalCall() { if (!is_script_scope()) scope_calls_eval_ = true; }
> 
> So "this" not forced to be allocated in a context, in
> general.  e.g. on this file:
> 
>   $ echo "eval('this')" > /tmp/eval.js
>   $ out/x64.debug/d8 --print-scopes /tmp/eval.js
>   global { // (0, 14)
>     // scope has trivial outer context
>     // 1 stack slots
>     // temporary vars:
>     TEMPORARY .result;  // local[0]
>     // local vars:
>     VAR this;  // parameter[-1]
>     DYNAMIC_GLOBAL eval;  // 
>   }
>   eval { // (0, 4)
>     // scope has trivial outer context
>     // scope uses 'this'
>     // 1 stack slots
>     // temporary vars:
>     TEMPORARY .result;  // local[0]
>   }
> 
> Note that "this" is given stack allocation.  And yet, how
> does eval("this") actually work?  The answer: by horrible
> accident.  :(((
> 
> You might want to finish breakfast, make sure all food is
> not at risk of coming back up before reading on.
> 
> So, Runtime_CompileString or
> Runtime_ResolvePossibleDirectEval end up calling
> Compiler::GetFunctionFromEval, which takes a context and a
> string.  In script context, the context is usually the
> native context, but it could be a script context if there
> are context-allocated locals (possibly including "this"
> itself).  GetFunctionFromEval ends up calling
> Parser::DoParseProgram, indirectly, and that declares a new
> script scope, possibly unpacks other nested scopes from the
> ScopeInfo, and then nests an eval scope.
> 
> The script scope declares a "this" binding.  The eval scope
> does not.
> 
> What happens next is where it gets bad.
> 
> "this" could be in the context, for example if the script
> scope contains ()=>this.  Or it could be just on the stack.
> 
> If "this" is context-allocated in the script, everything
> works as you would think.
> 
> If "this" is stack-allocated in the script:
> 
>     If eval code uses "this", it resolves to the "this"
>     binding in the script scope, which is at parameter[-1].
>     There happens to be a copy of "this" passed to the eval
>     code, as we mentioned before, so this happens to work --
>     even though it thinks it's getting the binding from the
>     script.  Horrific.
> 
>     Inside "with", in earlier versions of this patchset,
>     access to "this" was happening via dynamic lookup.
>     Again, this is not the case in master; in master, "this"
>     is always resolved statically.  OK.  But without "this"
>     in the context, the lookup will fail.  That's the error
>     I was getting with mozilla code.
> 
>     Arrow functions: normally arrow functions cause "this"
>     to be context-allocated in the script, and this works
>     fine.  But if the arrow function is in "eval", then
>     scope resolution assumes that the variable was
>     contexted-allocated in the script (since it's outside
>     the arrow function scope), and "()=>this" residualizes
>     an access to context slot (depth=0,
>     slot=MIN_CONTEXT_SLOTS=4).  But there is no script
>     context!  So where does this map?  To slot 4 in the
>     native context... which happens to be the global
>     proxy... so it just "works" :(((
> 
>     In later versions of the patchset, we resolve "this"
>     statically even within "with".  That makes the
>     eval('with({}) this') case be the same as
>     'eval("()=>this")()', and have the same unfortunate
>     possible aliasing of the native context.
> 
> I am not sure what The Right Solution is here.  It sounds
> like making all scripts have context-allocated "this" is the
> cleanest thing, but I don't know what the perf impact would
> be.  An initial attempt to do this produced bootstrapping
> failures.
> 
> I would like to plead for applying this patch as is and
> fixing these terrible things in follow-ups, but I wonder if
> this horrible accidental aliasing would not be exploitable
> to gain access to the global object.  In theory "this"
> should always be allocated to either parameter[-1] or to
> slot 4 in the context, so probably this is not possible.
> Hard to make definitive statements here, though.  Your
> thoughts very much appreciated!

Thanks for looking into this. I read this before my breakfast, fortunately.

I wonder if the natural solution wouldn't be to pre-allocate global 'this' in
the global context? There is no reason to re-allocate it for every script, it
logically scopes over all scripts.

[wingo, 2015-04-28 19:02:13]

On 2015/04/28 16:27:04, rossberg wrote:
> I wonder if the natural solution wouldn't be to pre-allocate global 'this' in
> the global context? There is no reason to re-allocate it for every script, it
> logically scopes over all scripts.

Interesting idea.  How would that be reflected in the scope tree?  Would
there be a new kind of scope for the global scope?

[wingo, 2015-05-05 14:39:42]

Updated CL adds TODO for sorting out "this" scope in script context.

[arv (Not doing code reviews), 2015-05-05 14:59:09]

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

[arv (Not doing code reviews), 2015-05-05 14:59:10]

LGTM

Some JS style nits

https://codereview.chromium.org/1097283003/diff/220001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1097283003/diff/220001/src/parser.cc#newcode745
src/parser.cc:745: pos + 4, Variable::THIS);
I still want you to change the order of these params.

https://codereview.chromium.org/1097283003/diff/220001/src/parser.cc#newcode972
src/parser.cc:972: // context, which will have the "this" binding for script
scopes.
Might make sense. Also for module scopes there is no this binding.

https://codereview.chromium.org/1097283003/diff/220001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/arrow-functions-this.js (right):

https://codereview.chromium.org/1097283003/diff/220001/test/mjsunit/harmony/a...
test/mjsunit/harmony/arrow-functions-this.js:11: var globalSloppyArrow       =
()=>this;
no aligning of =

whitespace around =>

var globalSloppyArrow = () => this;

https://codereview.chromium.org/1097283003/diff/220001/test/mjsunit/harmony/a...
test/mjsunit/harmony/arrow-functions-this.js:17: var strictFunctionArrow     =
function(){ "use strict";
indent blocks two spaces

whitespace between ) and {

var strictFunctionArrow = function() {
  "use strict";
  return (() => this)();
};

[wingo, 2015-05-05 15:49:29]

Updated patch rebases to apply cleanly and fixes nits from Erik.

https://codereview.chromium.org/1097283003/diff/220001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1097283003/diff/220001/src/parser.cc#newcode745
src/parser.cc:745: pos + 4, Variable::THIS);
On 2015/05/05 14:59:10, arv wrote:
> I still want you to change the order of these params.

Sorry I forgot about this!  Done.  Had to add Variable::NORMAL to a bunch of
callers though.

https://codereview.chromium.org/1097283003/diff/220001/test/mjsunit/harmony/a...
File test/mjsunit/harmony/arrow-functions-this.js (right):

https://codereview.chromium.org/1097283003/diff/220001/test/mjsunit/harmony/a...
test/mjsunit/harmony/arrow-functions-this.js:11: var globalSloppyArrow       =
()=>this;
On 2015/05/05 14:59:10, arv wrote:
> no aligning of =
> 
> whitespace around =>
> 
> var globalSloppyArrow = () => this;

Done.

https://codereview.chromium.org/1097283003/diff/220001/test/mjsunit/harmony/a...
test/mjsunit/harmony/arrow-functions-this.js:17: var strictFunctionArrow     =
function(){ "use strict";
On 2015/05/05 14:59:10, arv wrote:
> indent blocks two spaces
> 
> whitespace between ) and {
> 
> var strictFunctionArrow = function() {
>   "use strict";
>   return (() => this)();
> };

Done.

[wingo, 2015-05-05 15:52:04]

Going to CQ+ as Andreas indicated offline that we should land this then follow
up with cleanups.

[wingo, 2015-05-05 15:54:38]

The CQ bit was checked by wingo@igalia.com

[wingo, 2015-05-05 15:54:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
rossberg@chromium.org, arv@chromium.org
Link to the patchset: https://codereview.chromium.org/1097283003/#ps240001
(title: "Rebase and fix nits")

[commit-bot: I haz the power, 2015-05-05 15:54:50]

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

[commit-bot: I haz the power, 2015-05-05 16:38:25]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2015-05-05 16:38:33]

Patchset 11 (id:??) landed as
https://crrev.com/18619d355192e2699203d12d9ebb9caea107b693
Cr-Commit-Position: refs/heads/master@{#28236}

[wingo, 2015-05-05 17:23:32]

A revert of this CL (patchset #11 id:240001) has been created in
https://codereview.chromium.org/1113133006/ by wingo@igalia.com.

The reason for reverting is: nosnap failures.