Implement strict mode arguments caller/callee.

Implement strict mode arguments caller/callee.


BUG=
TEST=test/mjsunit/strict-mode.js

[Martin Maly, 2011-03-15 02:50:36]

Implementing strict mode poison pills for arguments object.
Kevin, I hope this doesn't interfere much with your arguments rewrite...

Thank you!
Martin

http://codereview.chromium.org/6698015/diff/1/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6698015/diff/1/src/heap.h#newcode596
src/heap.h:596: 
The length and callee properties switch places in the in-object representation
of arguments. Only length is present always, callee only for non-strict mode
arguments.
calle for strict mode arguments is an accessor pair hanging off the map.

http://codereview.chromium.org/6698015/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6698015/diff/1/src/runtime.cc#newcode4373
src/runtime.cc:4373: // Handle special arguments properties.
Runtime_GetArgumentsProperty ... it only appears to be called as a slow path for
_Arguments[?] inline call from JS runtime code. This code may not be hit since I
couldn't find _Arguments[x] with "callee" as property name.
Not sure if caller should be handled here as well since I suspect this code is
not used...

[Lasse Reichstein, 2011-03-15 09:58:40]

LGTM if Kevin agrees.

http://codereview.chromium.org/6698015/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6698015/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:4807: int arguments_object_size) {
Could you get these values from methods on ArgumentsAccessStub that switch on
the type_?

http://codereview.chromium.org/6698015/diff/1/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/6698015/diff/1/src/bootstrapper.cc#newcode998
src/bootstrapper.cc:998: //       callee must be added as the second property.
These are swapped because length is the only one requiring an in-object field
now?

http://codereview.chromium.org/6698015/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/6698015/diff/1/src/heap.cc#newcode2942
src/heap.cc:2942: if (arguments_object_size == kArgumentsObjectSize) {
Save the is_strict_callee boolean as a variable, and use it both above and here,
instead of checking a derived value here.

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

http://codereview.chromium.org/6698015/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:3660: __ mov(FieldOperand(eax, JSObject::kHeaderSize
+ kPointerSize), ebx);
use 
  FieldOperand(eax, JSObject::kHeaderSize + Heap::arguments_callee_index *
kPointerSize) 
instead of implicitly duplicating the constant in the expression.

http://codereview.chromium.org/6698015/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:3666: __ mov(FieldOperand(eax,
JSObject::kHeaderSize), ecx);
Ditto here. Just do 
FieldOperand(eax, JSObject::kHeaderSize + Heap::arguments_length_index *
kPointerSize)

http://codereview.chromium.org/6698015/diff/1/test/mjsunit/strict-mode.js
File test/mjsunit/strict-mode.js (right):

http://codereview.chromium.org/6698015/diff/1/test/mjsunit/strict-mode.js#new...
test/mjsunit/strict-mode.js:1061: CheckPillDescriptor(args, "callee");
How about a strict function calling a non-strict function that returns
arguments? This should still poison caller (but not callee according to 10.6,
[[Get]], step 3.b).
I think that might not be implemented yet, correct?

[Martin Maly, 2011-03-16 01:21:24]

Updated. Kevin, please let me know if this is OK.

Cheers
Martin

http://codereview.chromium.org/6698015/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6698015/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:4807: int arguments_object_size) {
On 2011/03/15 09:58:40, Lasse Reichstein wrote:
> Could you get these values from methods on ArgumentsAccessStub that switch on
> the type_?

Done.

http://codereview.chromium.org/6698015/diff/1/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/6698015/diff/1/src/bootstrapper.cc#newcode998
src/bootstrapper.cc:998: //       callee must be added as the second property.
On 2011/03/15 09:58:40, Lasse Reichstein wrote:
> These are swapped because length is the only one requiring an in-object field
> now?

Yes. We always have length, calee is only on non-strict arguments objects.

http://codereview.chromium.org/6698015/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/6698015/diff/1/src/heap.cc#newcode2942
src/heap.cc:2942: if (arguments_object_size == kArgumentsObjectSize) {
On 2011/03/15 09:58:40, Lasse Reichstein wrote:
> Save the is_strict_callee boolean as a variable, and use it both above and
here,
> instead of checking a derived value here.

Done.

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

http://codereview.chromium.org/6698015/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:3660: __ mov(FieldOperand(eax, JSObject::kHeaderSize
+ kPointerSize), ebx);
On 2011/03/15 09:58:40, Lasse Reichstein wrote:
> use 
>   FieldOperand(eax, JSObject::kHeaderSize + Heap::arguments_callee_index *
> kPointerSize) 
> instead of implicitly duplicating the constant in the expression.

Done, although the new code is less pretty :( especially in arm version where
the only way to achieve <80 lines was to introduce additional local

http://codereview.chromium.org/6698015/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:3666: __ mov(FieldOperand(eax,
JSObject::kHeaderSize), ecx);
On 2011/03/15 09:58:40, Lasse Reichstein wrote:
> Ditto here. Just do 
> FieldOperand(eax, JSObject::kHeaderSize + Heap::arguments_length_index *
> kPointerSize)

Done.

http://codereview.chromium.org/6698015/diff/1/test/mjsunit/strict-mode.js
File test/mjsunit/strict-mode.js (right):

http://codereview.chromium.org/6698015/diff/1/test/mjsunit/strict-mode.js#new...
test/mjsunit/strict-mode.js:1061: CheckPillDescriptor(args, "callee");
On 2011/03/15 09:58:40, Lasse Reichstein wrote:
> How about a strict function calling a non-strict function that returns
> arguments? This should still poison caller (but not callee according to 10.6,
> [[Get]], step 3.b).
> I think that might not be implemented yet, correct?

Correct. That part is not yet implemented.

http://codereview.chromium.org/6698015/diff/6001/src/code-stubs.h
File src/code-stubs.h (right):

http://codereview.chromium.org/6698015/diff/6001/src/code-stubs.h#newcode684
src/code-stubs.h:684: }
This is tragic. Using ?: results in linker not being able to find the
kArguments* constants. Unless I am missing something it smells like gcc bug...
so I had to resort to if .. else to work around it. It didn't make difference
whether the code was in inline method or not.

[Kevin Millikin (Chromium), 2011-03-16 09:48:41]

Not a full review, but what I looked at all LGTM.  No conflict with my ongoing
changes.

I have a few comments below.

http://codereview.chromium.org/6698015/diff/6001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6698015/diff/6001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4860: STATIC_ASSERT(Heap::arguments_callee_index ==
1);
That name is old school.

You might as well change it to kArgumentsCalleeIndex as part of this change.

http://codereview.chromium.org/6698015/diff/6001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4862: MemOperand callee_operand = FieldMemOperand(
You're right, this is ugly.  I would write instead:

const int kCalleeOffset =
    JSObject::kHeaderSize + Heap::kArgumentsCalleeIndex * kPointerSize;
__ str(r3, FieldMemOperand(r0, kCalleeOffset));

http://codereview.chromium.org/6698015/diff/6001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/6698015/diff/6001/src/arm/codegen-arm.cc#newco...
src/arm/codegen-arm.cc:605: ? ArgumentsAccessStub::NEW_OBJECT_STRICT
OBJECT is just noise.  How about NEW_STRICT and NEW_NON_STRICT?

http://codereview.chromium.org/6698015/diff/6001/src/code-stubs.h
File src/code-stubs.h (right):

http://codereview.chromium.org/6698015/diff/6001/src/code-stubs.h#newcode684
src/code-stubs.h:684: }
On 2011/03/16 01:21:24, Martin Maly wrote:
> This is tragic. Using ?: results in linker not being able to find the
> kArguments* constants. Unless I am missing something it smells like gcc bug...
> so I had to resort to if .. else to work around it. It didn't make difference
> whether the code was in inline method or not.

It looks like it's declared in heap.h, but there is no definition in heap.cc. 
We get away with that all the time, but the compiler doesn't have to allow it. 
Fix is to add:

const int Heap::kArgumentsObjectSizeStruct;
const int Heap::kArgumentsObjectSize;

in heap.cc.

(Relevant part of the standard:

9.4.2.4 4 If a static data member is of const integral or const enumeration
type, its declaration in the class definition can specify a constant-initializer
which shall be an integral constant expression (_expr.const_).  In that case,
the member can appear in integral constant expressions within its scope.  The
member shall still be defined in a namespace scope if it is used in the program
and the namespace scope definition shall not contain an initializer.)

http://codereview.chromium.org/6698015/diff/6001/src/contexts.h
File src/contexts.h (right):

http://codereview.chromium.org/6698015/diff/6001/src/contexts.h#newcode90
src/contexts.h:90: V(ARGUMENTS_BOILERPLATE_STRICT_INDEX, JSObject, \
Probably reads better as STRICT_ARGUMENTS_BOILERPLATE_INDEX, JSObject,
strict_arguments_boilerplate

http://codereview.chromium.org/6698015/diff/6001/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6698015/diff/6001/src/heap.h#newcode593
src/heap.h:593: static const int arguments_length_index = 0;
kArgumentsLengthIndex

[Martin Maly, 2011-03-16 22:22:25]

Thanks, Kevin. Good suggestions.
Since both Lasse and you LGTM'd this I will plan to land this as soon as the
change this depends on (http://codereview.chromium.org/6694044/) lands.

Cheers!
Martin

http://codereview.chromium.org/6698015/diff/6001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/6698015/diff/6001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4860: STATIC_ASSERT(Heap::arguments_callee_index ==
1);
On 2011/03/16 09:48:41, Kevin Millikin wrote:
> That name is old school.
> 
> You might as well change it to kArgumentsCalleeIndex as part of this change.

Done.

http://codereview.chromium.org/6698015/diff/6001/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4862: MemOperand callee_operand = FieldMemOperand(
On 2011/03/16 09:48:41, Kevin Millikin wrote:
> You're right, this is ugly.  I would write instead:
> 
> const int kCalleeOffset =
>     JSObject::kHeaderSize + Heap::kArgumentsCalleeIndex * kPointerSize;
> __ str(r3, FieldMemOperand(r0, kCalleeOffset));

Done. Good suggestion!

http://codereview.chromium.org/6698015/diff/6001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/6698015/diff/6001/src/arm/codegen-arm.cc#newco...
src/arm/codegen-arm.cc:605: ? ArgumentsAccessStub::NEW_OBJECT_STRICT
On 2011/03/16 09:48:41, Kevin Millikin wrote:
> OBJECT is just noise.  How about NEW_STRICT and NEW_NON_STRICT?

Done.

http://codereview.chromium.org/6698015/diff/6001/src/code-stubs.h
File src/code-stubs.h (right):

http://codereview.chromium.org/6698015/diff/6001/src/code-stubs.h#newcode684
src/code-stubs.h:684: }
On 2011/03/16 09:48:41, Kevin Millikin wrote:
> On 2011/03/16 01:21:24, Martin Maly wrote:
> > This is tragic. Using ?: results in linker not being able to find the
> > kArguments* constants. Unless I am missing something it smells like gcc
bug...
> > so I had to resort to if .. else to work around it. It didn't make
difference
> > whether the code was in inline method or not.
> 
> It looks like it's declared in heap.h, but there is no definition in heap.cc. 
> We get away with that all the time, but the compiler doesn't have to allow it.

> Fix is to add:
> 
> const int Heap::kArgumentsObjectSizeStruct;
> const int Heap::kArgumentsObjectSize;
> 
> in heap.cc.
> 
> (Relevant part of the standard:
> 
> 9.4.2.4 4 If a static data member is of const integral or const enumeration
> type, its declaration in the class definition can specify a
constant-initializer
> which shall be an integral constant expression (_expr.const_).  In that case,
> the member can appear in integral constant expressions within its scope.  The
> member shall still be defined in a namespace scope if it is used in the
program
> and the namespace scope definition shall not contain an initializer.)

Done. Thanks for the detailed info!

http://codereview.chromium.org/6698015/diff/6001/src/contexts.h
File src/contexts.h (right):

http://codereview.chromium.org/6698015/diff/6001/src/contexts.h#newcode90
src/contexts.h:90: V(ARGUMENTS_BOILERPLATE_STRICT_INDEX, JSObject, \
On 2011/03/16 09:48:41, Kevin Millikin wrote:
> Probably reads better as STRICT_ARGUMENTS_BOILERPLATE_INDEX, JSObject,
> strict_arguments_boilerplate

Done. Using STRICT_MODE_ARGUMENTS_BOILERPLATE_INDEX which is consistent with
what Mads recommended for the other names.

http://codereview.chromium.org/6698015/diff/6001/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6698015/diff/6001/src/heap.h#newcode593
src/heap.h:593: static const int arguments_length_index = 0;
On 2011/03/16 09:48:41, Kevin Millikin wrote:
> kArgumentsLengthIndex

Done.