Fix calls of strict mode function with an implicit receiver.

Fix calls of strict mode function with an implicit receiver.

Only IA32 version for now. I'll start porting.

Strict mode functions are to get 'undefined' as the receiver when
called with an implicit receiver. Modes are bad! It forces us to have
checks on all function calls.

This change attempts to limit the cost by passing information about
whether or not a call is with an implicit or explicit receiver in ecx
as part of the calling convention. The cost is setting ecx on all
calls and checking ecx on entry to strict mode functions.

Implicit/explicit receiver state has to be maintained by ICs. Various
stubs have to not clobber ecx or save and restore it.

CallFunction stub needs to check if the receiver is implicit when it 
doesn't know from the context.

Committed: http://code.google.com/p/v8/source/detail?r=8040

[Kevin Millikin (Chromium), 2011-05-18 15:57:23]

This is pretty hairy stuff.

It's not obvious whether the CallFunction stub flags are mutually exclusive (and
one implies the other, if so which one?) or if they're individual bits and
orthogonal.

It's also confusing whether the stub is responsible for setting ecx to the
correct tag value or else merely dispatching to the correct Invoke variation
that will set ecx.  I think it would be clearer if it only did the latter.

http://codereview.chromium.org/7039036/diff/5001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/7039036/diff/5001/src/ia32/builtins-ia32.cc#ne...
src/ia32/builtins-ia32.cc:1447: // Preserve the number of arguments on the
stack. Must preserve both
"both" --> ""

http://codereview.chromium.org/7039036/diff/5001/src/ia32/builtins-ia32.cc#ne...
src/ia32/builtins-ia32.cc:1495: __ mov(edi, -1);  // account for receiver
Not your code, but this loop could also count down the number of arguments left
to push (after the current iteration, so it starts at arg_count to include the
receiver).  I.e., do not mutate the base address and instead make use of the
mutated loop variable:

__ mov(edi, ebx);
__ bind(&copy);
__ push(Operand(eax, edi, times_4));
__ dec(edi);
__ j(not_sign, &copy);

http://codereview.chromium.org/7039036/diff/5001/src/ia32/builtins-ia32.cc#ne...
src/ia32/builtins-ia32.cc:1514: // ebx = expected - actual.
Then here, I think you can use more or less the same trick:

__ lea(edi, Operand(ebp, eax, times_4, offset));
__ bind(&copy);
__ push(Operand(edi, eax, times_4));
__ dec(eax);
__ j(not_sign, &copy);

__ bind(&fill);
__ push(Immediate(masm->isolate()->factory()->undefined_value()));
__ dec(ebx);
__ j(not_sign, &fill);

http://codereview.chromium.org/7039036/diff/5001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/7039036/diff/5001/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:3916: if (ReceiverMightBeValue() ||
NonValueReceiverMightBeImplicit()) {
I can't figure out if these are separate bits or if they are mutually exclusive.
 If they are separate bits, this should be written as:

if (NonValueReceiverMightBeImplicit()) {
}
if (ReceiverMightBeValue()) {
}

If mutually exclusive:

if (NonValueReceiverMightBeImplicit()) {
} else if (ReceiverMightBeValue()) {
}

http://codereview.chromium.org/7039036/diff/5001/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:3927: __ mov(ebx, FieldOperand(ebx,
GlobalObject::kGlobalReceiverOffset));
This should go in eax?  Otherwise the ReceiverMightBeValue TO_OBJECT case below
is going to still see the hole.

http://codereview.chromium.org/7039036/diff/5001/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:3929: // Mark as implicit receiver call.
We should be able to avoid setting ecx to check it below and see which
InvokeFunction to use (InvokeFunction will then set ecx again).  Instead we
could probably make use of the hole value/non-hole value we currently have in
eax if it were preserved.

http://codereview.chromium.org/7039036/diff/5001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/7039036/diff/5001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:2209: NON_VALUE_RECEIVER_MIGHT_BE_IMPLICIT);
Why not MIGHT_BE_VALUE | MIGHT_BE_IMPLICIT?  Don't we dodge the value check this
way?

(I had convinced myself a couple of weeks ago that this could be
NO_CALL_FUNCTION_FLAGS so I actually think it's OK, but I conservatively didn't
change it then because I didn't think it would make a performance difference.)

http://codereview.chromium.org/7039036/diff/5001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:2260: EmitCallWithStub(expr,
RECEIVER_MIGHT_BE_VALUE);
Why MIGHT_BE_VALUE?  It wasn't before, but I can see that it might be implicit
in the case of not being bound by a with.

http://codereview.chromium.org/7039036/diff/5001/src/ia32/macro-assembler-ia3...
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/7039036/diff/5001/src/ia32/macro-assembler-ia3...
src/ia32/macro-assembler-ia32.cc:1515: void
MacroAssembler::SetReceiverType(Register dst, ReceiverType type) {
I'm not sure it helps to have the register as an argument, since it is fixed. 
Maybe it's there to be explicit at the call sites?  If so, you could assert it's
ecx here.

http://codereview.chromium.org/7039036/diff/5001/src/v8globals.h
File src/v8globals.h (right):

http://codereview.chromium.org/7039036/diff/5001/src/v8globals.h#newcode315
src/v8globals.h:315: // Receiver is a non-value but might be implicit.
As it is, these are used as if mutually exclusive, not mix and match bits. 
That's a little confusing.  I think they should be used as separate bits even if
01 doesn't make sense.

I think "Receiver is a JSObject but might be implicit." is clearer.  You could
shorten the name to RECEIVER_MIGHT_BE_IMPLICIT if you want.  Maybe even better:
CHECK_IF_RECEIVER_IS_VALUE and CHECK_IF_RECEIVER_IS_IMPLICIT.

Even more radical is to move this enum into CallFunctionStub and just do
something like:

class CallFunctionStub {
  ...
  enum ReceiverCheck { NONE, VALUE = 0x1, IMPLICIT = 0x2 };

http://codereview.chromium.org/7039036/diff/5001/src/v8globals.h#newcode506
src/v8globals.h:506: enum ReceiverType {
Maybe this is not quite the right name.  There is no explicit receiver in with
(...) { ... f() ... }.  Maybe it's better to put the focus on the kind of call:

enum CallKind { CALL_AS_METHOD, CALL_AS_FUNCTION }?

[Mads Ager (chromium), 2011-05-23 16:31:34]

Could you have another look Kevin. Resolving the value check in CallFunctionStub
was easy. It is no longer needed.

I'll start the porting work.

http://codereview.chromium.org/7039036/diff/5001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/7039036/diff/5001/src/ia32/builtins-ia32.cc#ne...
src/ia32/builtins-ia32.cc:1447: // Preserve the number of arguments on the
stack. Must preserve both
On 2011/05/18 15:57:23, Kevin Millikin wrote:
> "both" --> ""

Done.

http://codereview.chromium.org/7039036/diff/5001/src/ia32/builtins-ia32.cc#ne...
src/ia32/builtins-ia32.cc:1495: __ mov(edi, -1);  // account for receiver
On 2011/05/18 15:57:23, Kevin Millikin wrote:
> Not your code, but this loop could also count down the number of arguments
left
> to push (after the current iteration, so it starts at arg_count to include the
> receiver).  I.e., do not mutate the base address and instead make use of the
> mutated loop variable:
> 
> __ mov(edi, ebx);
> __ bind(&copy);
> __ push(Operand(eax, edi, times_4));
> __ dec(edi);
> __ j(not_sign, &copy);

Won't this push the arguments in the wrong order? I would like to not change
this in this change. This is getting big and hairy as it is.

http://codereview.chromium.org/7039036/diff/5001/src/ia32/builtins-ia32.cc#ne...
src/ia32/builtins-ia32.cc:1514: // ebx = expected - actual.
On 2011/05/18 15:57:23, Kevin Millikin wrote:
> Then here, I think you can use more or less the same trick:
> 
> __ lea(edi, Operand(ebp, eax, times_4, offset));
> __ bind(&copy);
> __ push(Operand(edi, eax, times_4));
> __ dec(eax);
> __ j(not_sign, &copy);
> 
> __ bind(&fill);
> __ push(Immediate(masm->isolate()->factory()->undefined_value()));
> __ dec(ebx);
> __ j(not_sign, &fill);

Same as above. I think this will push arguments in the wrong order. I would like
to leave as is for now.

http://codereview.chromium.org/7039036/diff/5001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/7039036/diff/5001/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:3916: if (ReceiverMightBeValue() ||
NonValueReceiverMightBeImplicit()) {
On 2011/05/18 15:57:23, Kevin Millikin wrote:
> I can't figure out if these are separate bits or if they are mutually
exclusive.
>  If they are separate bits, this should be written as:
> 
> if (NonValueReceiverMightBeImplicit()) {
> }
> if (ReceiverMightBeValue()) {
> }
> 
> If mutually exclusive:
> 
> if (NonValueReceiverMightBeImplicit()) {
> } else if (ReceiverMightBeValue()) {
> }

I can't actually hit the non-object case in the call function stub. I cannot see
how we can ever get in that situation. Certainly non of our tests hit that case
(I tested with an int3()). I'll just get rid of the receiver value check
completely.

Soren and I digged into it today. It was put in when we used CallFunctionStub
for keyed calls. We don't do that anymore (only for synthetic where we know it
is not a value) and therefore we cannot hit this case.

http://codereview.chromium.org/7039036/diff/5001/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:3927: __ mov(ebx, FieldOperand(ebx,
GlobalObject::kGlobalReceiverOffset));
On 2011/05/18 15:57:23, Kevin Millikin wrote:
> This should go in eax?  Otherwise the ReceiverMightBeValue TO_OBJECT case
below
> is going to still see the hole.

Doesn't matter. We unconditionally jmp over the TO_OBJECT case if we see the
hole value here.

Will be dead code now.

http://codereview.chromium.org/7039036/diff/5001/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:3929: // Mark as implicit receiver call.
On 2011/05/18 15:57:23, Kevin Millikin wrote:
> We should be able to avoid setting ecx to check it below and see which
> InvokeFunction to use (InvokeFunction will then set ecx again).  Instead we
> could probably make use of the hole value/non-hole value we currently have in
> eax if it were preserved.

Done.

http://codereview.chromium.org/7039036/diff/5001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/7039036/diff/5001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:2209: NON_VALUE_RECEIVER_MIGHT_BE_IMPLICIT);
On 2011/05/18 15:57:23, Kevin Millikin wrote:
> Why not MIGHT_BE_VALUE | MIGHT_BE_IMPLICIT?  Don't we dodge the value check
this
> way?
> 
> (I had convinced myself a couple of weeks ago that this could be
> NO_CALL_FUNCTION_FLAGS so I actually think it's OK, but I conservatively
didn't
> change it then because I didn't think it would make a performance difference.)

Yes, this does dodge the value check. I convinced myself that this was OK. I'll
add some more test cases to have proper test coverage for this case.

Come to think of it I cannot actually see how we can get a non-value in here.
Adding an int3 where we call ToObject doesn't show anything in our tests hitting
the non-value CallFunctionStub path.

I will get rid of it completely.

http://codereview.chromium.org/7039036/diff/5001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:2260: EmitCallWithStub(expr,
RECEIVER_MIGHT_BE_VALUE);
On 2011/05/18 15:57:23, Kevin Millikin wrote:
> Why MIGHT_BE_VALUE?  It wasn't before, but I can see that it might be implicit
> in the case of not being bound by a with.

Yes, I need to change the name of the flags to make this clearer. This is as you
say MIGHT_BE_IMPLICIT.

http://codereview.chromium.org/7039036/diff/5001/src/ia32/macro-assembler-ia3...
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/7039036/diff/5001/src/ia32/macro-assembler-ia3...
src/ia32/macro-assembler-ia32.cc:1515: void
MacroAssembler::SetReceiverType(Register dst, ReceiverType type) {
On 2011/05/18 15:57:23, Kevin Millikin wrote:
> I'm not sure it helps to have the register as an argument, since it is fixed. 
> Maybe it's there to be explicit at the call sites?  If so, you could assert
it's
> ecx here.

The point was exactly to make it clear at the call site which register is used.
Otherwise it becomes very hard to figure out what is going on when reading the
call site. I will add the assert. Thanks!

http://codereview.chromium.org/7039036/diff/5001/src/v8globals.h
File src/v8globals.h (right):

http://codereview.chromium.org/7039036/diff/5001/src/v8globals.h#newcode506
src/v8globals.h:506: enum ReceiverType {
On 2011/05/18 15:57:23, Kevin Millikin wrote:
> Maybe this is not quite the right name.  There is no explicit receiver in with
> (...) { ... f() ... }.  Maybe it's better to put the focus on the kind of
call:
> 
> enum CallKind { CALL_AS_METHOD, CALL_AS_FUNCTION }?

I agree. Your suggestion is better. Done.

[Mads Ager (chromium), 2011-05-24 07:14:14]

http://codereview.chromium.org/7039036/diff/8001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/7039036/diff/8001/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:3953: // If the receiver might be a value (string,
number or boolean) check for this
I will update this comment.

http://codereview.chromium.org/7039036/diff/8001/src/ia32/macro-assembler-ia3...
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/7039036/diff/8001/src/ia32/macro-assembler-ia3...
src/ia32/macro-assembler-ia32.cc:1658: call_wrapper);
This needs the call kind as well. I will add it.

[Kevin Millikin (Chromium), 2011-05-24 08:25:26]

This looks good enough to port to the other platforms.

http://codereview.chromium.org/7039036/diff/5001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/7039036/diff/5001/src/ia32/builtins-ia32.cc#ne...
src/ia32/builtins-ia32.cc:1495: __ mov(edi, -1);  // account for receiver
On 2011/05/23 16:31:34, Mads Ager wrote:
> On 2011/05/18 15:57:23, Kevin Millikin wrote:
> > Not your code, but this loop could also count down the number of arguments
> left
> > to push (after the current iteration, so it starts at arg_count to include
the
> > receiver).  I.e., do not mutate the base address and instead make use of the
> > mutated loop variable:
> > 
> > __ mov(edi, ebx);
> > __ bind(&copy);
> > __ push(Operand(eax, edi, times_4));
> > __ dec(edi);
> > __ j(not_sign, &copy);
> 
> Won't this push the arguments in the wrong order? I would like to not change
> this in this change. This is getting big and hairy as it is.

It pushes in the right order (high to low addresses) but pushes the wrong block
of stuff.  I forgot the corresponding change to the base address.  Leave it as
is if you want.

http://codereview.chromium.org/7039036/diff/5001/src/ia32/builtins-ia32.cc#ne...
src/ia32/builtins-ia32.cc:1514: // ebx = expected - actual.
On 2011/05/23 16:31:34, Mads Ager wrote:
> On 2011/05/18 15:57:23, Kevin Millikin wrote:
> > Then here, I think you can use more or less the same trick:
> > 
> > __ lea(edi, Operand(ebp, eax, times_4, offset));
> > __ bind(&copy);
> > __ push(Operand(edi, eax, times_4));
> > __ dec(eax);
> > __ j(not_sign, &copy);
> > 
> > __ bind(&fill);
> > __ push(Immediate(masm->isolate()->factory()->undefined_value()));
> > __ dec(ebx);
> > __ j(not_sign, &fill);
> 
> Same as above. I think this will push arguments in the wrong order. I would
like
> to leave as is for now.

It copies the wrong block of stuff.  Sorry about that.  I forgot the
corresponding change to the base address (i.e., I double counted eax):

__ bind(&copy);
__ push(Operand(ebp, eax, times_4, offset));
__ dec(eax);
__ j(not_sign, &copy);

__ bind(&fill);
__ push(Immediate(masm->isolate()->factory()->undefined_value()));
__ dec(ebx);
__ j(not_sign, &fill);

It's OK to leave it as you have it in this change, but I think the above is
simpler than what's in the change.  In this case, the CL doesn't really leave it
as is anyway (the subtraction and negation is new, as is counting from a
non-positive index up to zero rather than from zero up to a non-negative index).

[Mads Ager (chromium), 2011-05-24 13:04:14]

Ported to other platforms. Whew.

http://codereview.chromium.org/7039036/diff/13002/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/7039036/diff/13002/src/arm/full-codegen-arm.cc...
src/arm/full-codegen-arm.cc:3668: RelocInfo::Mode mode = RelocInfo::CODE_TARGET;
Using CODE_TARGET here instead of CODE_TARGET_WITH_AST_ID. Bill is working on a
patch to make that the right thing. Same thing other places in the ARM code.
Currently assertions are hit in debug mode, but all tests pass in release mode.
With Bill's change we should be good.

[Kevin Millikin (Chromium), 2011-05-24 13:21:36]

OK, LGTM.

http://codereview.chromium.org/7039036/diff/13002/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/7039036/diff/13002/src/arm/full-codegen-arm.cc...
src/arm/full-codegen-arm.cc:138: // object). ecx is zero for method calls and
non-zero for function
ecx ==> r5.

http://codereview.chromium.org/7039036/diff/13002/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/7039036/diff/13002/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:150: // object). ecx is zero for method calls and
non-zero for function
ecx ==> r5.

[Mads Ager (chromium), 2011-05-24 13:57:53]

http://codereview.chromium.org/7039036/diff/13002/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/7039036/diff/13002/src/arm/full-codegen-arm.cc...
src/arm/full-codegen-arm.cc:138: // object). ecx is zero for method calls and
non-zero for function
On 2011/05/24 13:21:36, Kevin Millikin wrote:
> ecx ==> r5.

Done.

http://codereview.chromium.org/7039036/diff/13002/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/7039036/diff/13002/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:150: // object). ecx is zero for method calls and
non-zero for function
On 2011/05/24 13:21:36, Kevin Millikin wrote:
> ecx ==> r5.

Done.