Strict mode poison pills for function.caller and function.arguments

3 patches in this CR to keep things a bit easier to follow.

First patch is straight-up revert of Mads' revert of my original commit http://code.google.com/p/v8/source/detail?r=7168

Second patch is fix for the Kraken issue. It turns out that V8 temporarily sets function prototype as read only while builtins are being processed but later switches it back to read/write. I only did the first part for strict mode functions so now I am doing the second half as well.

Third patch is series of renames Mads suggested. I went for the long names. Given how rarely they are actually used the more descriptive name seemed better.

As for testing on Kraken ... I tested few of the Kraken tests manually on command line V8 (thanks, Mads, for the link) but also built Chrome with this change and ran the web Kraken test to make sure things are alright.

Thank you!
Martin

[Martin Maly, 2011-03-15 22:56:39]

Trying again...

[Mads Ager (chromium), 2011-03-16 09:29:13]

http://codereview.chromium.org/6694044/diff/1023/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/6694044/diff/1023/src/bootstrapper.cc#newcode510
src/bootstrapper.cc:510: Handle<DescriptorArray>
Genesis::ComputeStrictFunctionDescriptor(
Keep the 'Instance' in the name? ComputeStrictFunctionInstanceDescriptor?

Can the structure of this be closer to ComputeFunctionInstanceDescriptor using
CopyAddDescriptor? (Or the other way around, use this structure for the other
one.) Would be nice to use the same structure for doing the same thing.

http://codereview.chromium.org/6694044/diff/1023/src/bootstrapper.cc#newcode560
src/bootstrapper.cc:560: Handle<JSFunction> pill =
Factory::NewFunctionWithoutPrototypeStrict(name);
This means that you will get two ThrowTypeError functions doesn't it? I think
that the spec says that there should be only one?

If that is the case, rename this to CreateThrowTypeErrorFunction and have it
return the pill. Then set it as the get and set for both of the callback arrays?

http://codereview.chromium.org/6694044/diff/1023/src/bootstrapper.cc#newcode576
src/bootstrapper.cc:576: void Genesis::CreateThrowTypeError(Handle<JSFunction>
empty) {
This method seems to have a strange name. It is really a mix of allocating
strict mode function maps and setting up two accessors. Rename to
CreateStrictFunctionsMaps or something along those lines?

http://codereview.chromium.org/6694044/diff/1023/src/bootstrapper.cc#newcode1943
src/bootstrapper.cc:1943: // Extract arguments and caller from the original
strict function map.
It seems a bit complicated to have to extract the arguments and caller arrays
here, but I don't really have a good suggestion.

http://codereview.chromium.org/6694044/diff/1023/src/bootstrapper.cc#newcode1956
src/bootstrapper.cc:1956: Handle<Map> map = Factory::NewMap(JS_FUNCTION_TYPE,
JSFunction::kSize);
Is there a reason why you do not use CopyMapDropDescriptors here as we do for
the normal function map and avoid the set_prototype below?

http://codereview.chromium.org/6694044/diff/1023/src/factory.h
File src/factory.h (right):

http://codereview.chromium.org/6694044/diff/1023/src/factory.h#newcode235
src/factory.h:235: static Handle<JSFunction> NewFunctionWithoutPrototypeStrict(
NewStrictFunctionWithoutPrototype?

http://codereview.chromium.org/6694044/diff/1023/src/factory.h#newcode413
src/factory.h:413: static Handle<JSFunction>
NewFunctionWithoutPrototypeStrictHelper(
NewStrictFunction... ?

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

http://codereview.chromium.org/6694044/diff/1023/test/mjsunit/strict-mode.js#...
test/mjsunit/strict-mode.js:1025: (function KrakenTest() {
Kraken -> WritablePrototype?

[Martin Maly, 2011-03-16 23:40:35]

Thanks, Mads. Made the changes.

martin

http://codereview.chromium.org/6694044/diff/1023/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/6694044/diff/1023/src/bootstrapper.cc#newcode510
src/bootstrapper.cc:510: Handle<DescriptorArray>
Genesis::ComputeStrictFunctionDescriptor(
On 2011/03/16 09:29:13, Mads Ager wrote:
> Keep the 'Instance' in the name? ComputeStrictFunctionInstanceDescriptor?
> 
> Can the structure of this be closer to ComputeFunctionInstanceDescriptor using
> CopyAddDescriptor? (Or the other way around, use this structure for the other
> one.) Would be nice to use the same structure for doing the same thing.

Done. Used what I think is cleaner code which directly sets the descriptors
rather than iteratively append them, copying the descriptor array each time.

http://codereview.chromium.org/6694044/diff/1023/src/bootstrapper.cc#newcode560
src/bootstrapper.cc:560: Handle<JSFunction> pill =
Factory::NewFunctionWithoutPrototypeStrict(name);
On 2011/03/16 09:29:13, Mads Ager wrote:
> This means that you will get two ThrowTypeError functions doesn't it? I think
> that the spec says that there should be only one?
> 
> If that is the case, rename this to CreateThrowTypeErrorFunction and have it
> return the pill. Then set it as the get and set for both of the callback
arrays?

Yes. The reason I am doing it this way is to provide better error message.
Experiments with jsc showed that it creates new instance of poison pill per
strict mode function so I felt it is better choice to give better error message
at the expense of having 4 instances of poison pills in the whole system (rather
than unbounded number which jsc appears to allocate).

Also, renamed to CreateThrowTypeErrorFunction. Thanks for the suggestion!

http://codereview.chromium.org/6694044/diff/1023/src/bootstrapper.cc#newcode576
src/bootstrapper.cc:576: void Genesis::CreateThrowTypeError(Handle<JSFunction>
empty) {
On 2011/03/16 09:29:13, Mads Ager wrote:
> This method seems to have a strange name. It is really a mix of allocating
> strict mode function maps and setting up two accessors. Rename to
> CreateStrictFunctionsMaps or something along those lines?

Done.

http://codereview.chromium.org/6694044/diff/1023/src/bootstrapper.cc#newcode1943
src/bootstrapper.cc:1943: // Extract arguments and caller from the original
strict function map.
On 2011/03/16 09:29:13, Mads Ager wrote:
> It seems a bit complicated to have to extract the arguments and caller arrays
> here, but I don't really have a good suggestion.

On a second look ... there is an option which I find cleanest so far. The
Genesis object can maintain some state. empty_function_ (unused) was an example.
The writable prototype maps are created alongside the rest of the maps, stored
away in the Genesis object state and here simply installed, no computation
necessary.

I am also deleting the unused empty_function_.

http://codereview.chromium.org/6694044/diff/1023/src/bootstrapper.cc#newcode1956
src/bootstrapper.cc:1956: Handle<Map> map = Factory::NewMap(JS_FUNCTION_TYPE,
JSFunction::kSize);
On 2011/03/16 09:29:13, Mads Ager wrote:
> Is there a reason why you do not use CopyMapDropDescriptors here as we do for
> the normal function map and avoid the set_prototype below?

:) I confess it is perhaps reflection of my continued learning of the codebase
(and general fear of functions that appear to 'do a whole lot'). I was trying to
use simpler building blocks which I understood well at the time.

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

http://codereview.chromium.org/6694044/diff/1023/test/mjsunit/strict-mode.js#...
test/mjsunit/strict-mode.js:1025: (function KrakenTest() {
On 2011/03/16 09:29:13, Mads Ager wrote:
> Kraken -> WritablePrototype?

Done. (TestStrictFunctionWritablePrototype)

[Mads Ager (chromium), 2011-03-17 07:48:25]

LGTM

http://codereview.chromium.org/6694044/diff/10001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/6694044/diff/10001/src/bootstrapper.cc#newcode296
src/bootstrapper.cc:296: // Function instance maps. Funcion literal maps are
created initially with
Funcion -> Function

http://codereview.chromium.org/6694044/diff/10001/src/bootstrapper.cc#newcode589
src/bootstrapper.cc:589: 
Delete this line?

http://codereview.chromium.org/6694044/diff/10001/src/factory.cc
File src/factory.cc (right):

http://codereview.chromium.org/6694044/diff/10001/src/factory.cc#newcode834
src/factory.cc:834: Handle<JSFunction>
Factory::NewFunctionWithoutPrototypeStrictHelper(
Instead of duplicating the code here, would it make sense to parameterize
NewFunctionWithoutPrototype with a strict mode flag and then select the right
map in NewFunctionWithoutPrototypeHelper? Or maybe have only one helper that
takes a strict mode flags and uses that to select the map?

http://codereview.chromium.org/6694044/diff/10001/src/factory.cc#newcode845
src/factory.cc:845: Handle<JSFunction>
Factory::NewFunctionWithoutPrototypeStrict(
NewStrictFunctionWithoutPrototype?

[Martin Maly, 2011-03-17 16:00:55]

Done with the final touches. Will now try to land.

http://codereview.chromium.org/6694044/diff/10001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

http://codereview.chromium.org/6694044/diff/10001/src/bootstrapper.cc#newcode296
src/bootstrapper.cc:296: // Function instance maps. Funcion literal maps are
created initially with
On 2011/03/17 07:48:25, Mads Ager wrote:
> Funcion -> Function

Done.

http://codereview.chromium.org/6694044/diff/10001/src/bootstrapper.cc#newcode589
src/bootstrapper.cc:589: 
On 2011/03/17 07:48:25, Mads Ager wrote:
> Delete this line?

Done.

http://codereview.chromium.org/6694044/diff/10001/src/factory.cc
File src/factory.cc (right):

http://codereview.chromium.org/6694044/diff/10001/src/factory.cc#newcode834
src/factory.cc:834: Handle<JSFunction>
Factory::NewFunctionWithoutPrototypeStrictHelper(
On 2011/03/17 07:48:25, Mads Ager wrote:
> Instead of duplicating the code here, would it make sense to parameterize
> NewFunctionWithoutPrototype with a strict mode flag and then select the right
> map in NewFunctionWithoutPrototypeHelper? Or maybe have only one helper that
> takes a strict mode flags and uses that to select the map?

Done.

http://codereview.chromium.org/6694044/diff/10001/src/factory.cc#newcode845
src/factory.cc:845: Handle<JSFunction>
Factory::NewFunctionWithoutPrototypeStrict(
On 2011/03/17 07:48:25, Mads Ager wrote:
> NewStrictFunctionWithoutPrototype?

This function went away now that I removed the code duplication.