Strict Mode assignment to read only property.

Assignment to read only property.

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

I also uploaded this patch to the original CL although bunch of intervening changes make it harder to review.

http://codereview.chromium.org/6576024/

[Martin Maly, 2011-02-27 23:04:04]

I believe this is now ready for review. Still few todos where I am not sure how
strict mode applies (v8 API, Runtime_DeclareContextSlot) or bigger piece of work
carved out for future patches (SetElement). But I am convinced this is a great
state to converge and commit and handle the rest in separate patches.

With this change my main concern is any places where I may have missed passing
strict mode. Hopefully the approach I took (adding parameters, marking places in
code with TODOs and removing them as I fixed the code up), plus adding the
checks that IC strictness stays the same throughout the IC patching give me
confidence that none were missed.

Because the intervening changes make the original patch harder to review I am
creating this new one. The old one still exists and has the latest code uploaded
as well (http://codereview.chromium.org/6576024/)

Thank you!
Martin

http://codereview.chromium.org/6594037/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/api.cc#newcode2290
src/api.cc:2290: i::kNonStrictMode);  // TODO(mmaly): v8::Object::Set API. Needs
strict?
Do we need to expand the V8 API to allow for strict mode or do we keep it
non-strict?

http://codereview.chromium.org/6594037/diff/1/src/builtins.h
File src/builtins.h (left):

http://codereview.chromium.org/6594037/diff/1/src/builtins.h#oldcode139
src/builtins.h:139: StoreIC::kStoreICStrict)                  \
I deleted this enum in favor of using StrictModeFlag directly. It is way
cleaner.

http://codereview.chromium.org/6594037/diff/1/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/ia32/ic-ia32.cc#newcode1637
src/ia32/ic-ia32.cc:1637: __ TailCallRuntime(Runtime::kSetProperty, 5, 1);
I am keeping the argument count to 4-5. I hear the argument that they are quite
different and merging PropertyAttributes with strict mode feels a bit weird. It
is still an option, and so is making them optional. Still, pushing a value on
the stack is likely cheaper than checking whether the optional argument was
passed...

http://codereview.chromium.org/6594037/diff/1/src/ic-inl.h
File src/ic-inl.h (right):

http://codereview.chromium.org/6594037/diff/1/src/ic-inl.h#newcode87
src/ic-inl.h:87: #endif
This is a check that ICs do not migrate from strict mode to non-strict and vice
versa.

For added paranoia there is another check in StoreIC::set_target and
KeyedStoreIC::set_target and they are ultimately redundant. I am open to which
to keep and which to drop.

http://codereview.chromium.org/6594037/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/ic.cc#newcode1870
src/ic.cc:1870: return result;
This will be cleaned up before commit, the value is only the inline question in
the comment. It was my attempt at 3rd order paranoid check that ICs don't switch
strictness.

http://codereview.chromium.org/6594037/diff/1/src/ic.h
File src/ic.h (right):

http://codereview.chromium.org/6594037/diff/1/src/ic.h#newcode439
src/ic.h:439: 
This is the 2nd check that StoreIC doesn't switch strictness as it gets updated.

I like this check because it only affects StoreIC (or KeyedStoreIC below) but it
is still a bit removed from the actual patching. Plus it uses method shadowing
so it is a bit more fragile than the other test.

http://codereview.chromium.org/6594037/diff/1/src/ic.h#newcode505
src/ic.h:505: }
ditto for KeyedStoreIC.

http://codereview.chromium.org/6594037/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/parser.cc#newcode1644
src/parser.cc:1644: 
Handling const and var in different code paths since they take different
arguments now. const is not allowed in strict mode so no need to change it. Var
takes strict mode flag.

http://codereview.chromium.org/6594037/diff/1/src/parser.cc#newcode1658
src/parser.cc:1658: temp_scope_->StrictMode() ? kStrictMode : kNonStrictMode));
Is it worth passing some singleton object here? true/false vs. allocating new
literal per global variable?

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

http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode3934
src/runtime.cc:3934: CONVERT_SMI_CHECKED(unchecked_attributes, args[3]);
PropertyAttributes always passed.

http://codereview.chromium.org/6594037/diff/1/src/stub-cache.cc
File src/stub-cache.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/stub-cache.cc#newcode1286
src/stub-cache.cc:1286: // Leaving JavaScript. TODO(mmaly): Do we need strict
mode here?
Are we going to need a way to pass strict mode to the callback here? I cannot
think of a use scenario but there may be one.

http://codereview.chromium.org/6594037/diff/1/src/x64/ic-x64.cc
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/x64/ic-x64.cc#newcode1597
src/x64/ic-x64.cc:1597: void StoreIC::GenerateGlobalProxy(MacroAssembler* masm,
StrictModeFlag strict) {
I should probably rename this to strict_mode also before commit.

[Lasse Reichstein, 2011-02-28 11:18:30]

LGTM

http://codereview.chromium.org/6594037/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/api.cc#newcode2290
src/api.cc:2290: i::kNonStrictMode);  // TODO(mmaly): v8::Object::Set API. Needs
strict?
I'd say to keep it non-strict. The C++-API isn't part of the language, but is a
separate way to work on the JS data. We can reuse the non-strict JS
functionality for that because it doesn't add restrictions.

http://codereview.chromium.org/6594037/diff/1/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/ia32/ic-ia32.cc#newcode1637
src/ia32/ic-ia32.cc:1637: __ TailCallRuntime(Runtime::kSetProperty, 5, 1);
You could combine the values into a "StoreICFlag" encoded value (e.g., using
BitFields), but this should be fine.

http://codereview.chromium.org/6594037/diff/1/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2555: __
push(Immediate(Smi::FromInt(strict_mode_)));  // strict mode
Comment isn't necessary here. The variable is self-explanatory :).

http://codereview.chromium.org/6594037/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:3720: : kNonStrictMode)));
Is the conditional necessary here? It seems
Code::ExtractExtraICStateFromFlags(flags) returns kStrictMode directly (or at
most something castable to it).

http://codereview.chromium.org/6594037/diff/1/src/ia32/virtual-frame-ia32.cc
File src/ia32/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/ia32/virtual-frame-ia32.cc#...
src/ia32/virtual-frame-ia32.cc:1042: strict_mode == kStrictMode ?
Builtins::StoreIC_Initialize_Strict
Put parentheses around any non-trivial condition of a ?: operator.

http://codereview.chromium.org/6594037/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/ic.cc#newcode1868
src/ic.cc:1868: //
I don't know. Do you have a way to reproduce the problem?

http://codereview.chromium.org/6594037/diff/1/src/ic.cc#newcode1946
src/ic.cc:1946: //
Again, can you reproduce it? That would make it much easier to figure out who to
ask about it :)

http://codereview.chromium.org/6594037/diff/1/src/ic.cc#newcode1947
src/ic.cc:1947: // ASSERT(extra_ic_state == ic.target()->extra_ic_state());
Should be removed before commit.

http://codereview.chromium.org/6594037/diff/1/src/ic.h
File src/ic.h (right):

http://codereview.chromium.org/6594037/diff/1/src/ic.h#newcode436
src/ic.h:436: ASSERT(code->extra_ic_state() == target()->extra_ic_state());
If the "extra_ic_state" is only strict/non-strict, just call it strict_ic_state
and return the correct enum value directly. The way you use it seems to assume
that the value is exactly kStrictMode.
If we ever need more state, we can add more accessors.

http://codereview.chromium.org/6594037/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/parser.cc#newcode1658
src/parser.cc:1658: temp_scope_->StrictMode() ? kStrictMode : kNonStrictMode));
I don't think so. The NumberLiteral will hold a smi, so it won't make Heap
allocation, only Zone allocation.

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

http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode550
src/runtime.cc:550: // Passing non-strict per Ecma-262 5th Ed. 12.14. Catch,
bullet #4.
Upper-case ECMA.

http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode1184
src/runtime.cc:1184: // TODO(mmaly): Runtime_DeclareContextSlot. Needs strict
mode?
I don't think the declaration needs it.

http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode1397
src/runtime.cc:1397: // TODO(mmaly): Strict mode not needed (const disallowed).
Agree. Remove TODOs (but retain rest of comment).

http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode1680
src/runtime.cc:1680: // No need for strict mode here. Called from SetupArray
(array.js).
Actually, if we make our builtins code strict, we should probably pass true
here.
Try using strict, and see if it works anyway.

http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode3808
src/runtime.cc:3808: // TODO(mmaly): Implement SetElement strict mode.
Make an entry in the v8 bug-tracker for the feature and use the issue number
instead of your name in the TODO.

http://codereview.chromium.org/6594037/diff/1/src/stub-cache.cc
File src/stub-cache.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/stub-cache.cc#newcode1286
src/stub-cache.cc:1286: // Leaving JavaScript. TODO(mmaly): Do we need strict
mode here?
No. The callback is C++-code. It's not strict or non-strict, that only refers to
JS code.
If we ever need a callback that needs to know whether it's called from strict
mode or not, we'll need to find a way to tell it, but currently we don't have
one.

http://codereview.chromium.org/6594037/diff/1/src/x64/ic-x64.cc
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/x64/ic-x64.cc#newcode1597
src/x64/ic-x64.cc:1597: void StoreIC::GenerateGlobalProxy(MacroAssembler* masm,
StrictModeFlag strict) {
Do! :)

[Vitaly Repeshko, 2011-02-28 11:35:05]

http://codereview.chromium.org/6594037/diff/1/src/ic.h
File src/ic.h (right):

http://codereview.chromium.org/6594037/diff/1/src/ic.h#newcode436
src/ic.h:436: ASSERT(code->extra_ic_state() == target()->extra_ic_state());
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> If the "extra_ic_state" is only strict/non-strict, just call it
strict_ic_state
> and return the correct enum value directly. The way you use it seems to assume
> that the value is exactly kStrictMode.
> If we ever need more state, we can add more accessors.

Please don't kill extra_ic_state. It's also used as an opaque extra state to
gather more type feedback. But having another accessor that wraps it is a
probably a good idea.

[Mads Ager (chromium), 2011-02-28 12:23:06]

Only had a quick look but looks good to me as well.

http://codereview.chromium.org/6594037/diff/1/src/builtins.h
File src/builtins.h (right):

http://codereview.chromium.org/6594037/diff/1/src/builtins.h#newcode154
src/builtins.h:154: V(KeyedStoreIC_Initialize_Strict,   KEYED_STORE_IC,
UNINITIALIZED,      \
This is the reason why I don't like indentation like this. Either we should
indent them all so they align or we should drop the alignment altogether.

http://codereview.chromium.org/6594037/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/ic.cc#newcode1485
src/ic.cc:1485: // Strict mode doesn't allow setting non-existent global
property.
Update comment to reflect the read-only part of this code.

http://codereview.chromium.org/6594037/diff/1/src/ic.cc#newcode1488
src/ic.cc:1488: return TypeError("strict_rdonly_property", object, name);
Please don't abbreviate read to rd.

http://codereview.chromium.org/6594037/diff/1/src/ic.cc#newcode1868
src/ic.cc:1868: //
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> I don't know. Do you have a way to reproduce the problem?

I would be interested in knowing what fails here as well. 

My guess here would be lazy deoptimization. If SetProperty performs a call to
something that either force deletes a property or enters the debugger, then we
will destroy the code object (overwriting return points with a call to
deoptimization). Therefore the target will be invalid at this point.

So, overall, the assert at this point will not work. We should never, ever use
target() or set_target() after we have invoked something that can call arbitrary
code (such as GetProperty, SetProperty). I don't think we do, but if we do that
will be bad in the presence of lazy deoptimization.

[Martin Maly, 2011-03-01 01:40:29]

Thanks for the review. I made the changes and am going ahead with the commit.

Thank you!
Martin

http://codereview.chromium.org/6594037/diff/1/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/api.cc#newcode2290
src/api.cc:2290: i::kNonStrictMode);  // TODO(mmaly): v8::Object::Set API. Needs
strict?
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> I'd say to keep it non-strict. The C++-API isn't part of the language, but is
a
> separate way to work on the JS data. We can reuse the non-strict JS
> functionality for that because it doesn't add restrictions.

Done.

http://codereview.chromium.org/6594037/diff/1/src/builtins.h
File src/builtins.h (right):

http://codereview.chromium.org/6594037/diff/1/src/builtins.h#newcode154
src/builtins.h:154: V(KeyedStoreIC_Initialize_Strict,   KEYED_STORE_IC,
UNINITIALIZED,      \
On 2011/02/28 12:23:06, Mads Ager wrote:
> This is the reason why I don't like indentation like this. Either we should
> indent them all so they align or we should drop the alignment altogether. 

Done.

http://codereview.chromium.org/6594037/diff/1/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/ia32/ic-ia32.cc#newcode1637
src/ia32/ic-ia32.cc:1637: __ TailCallRuntime(Runtime::kSetProperty, 5, 1);
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> You could combine the values into a "StoreICFlag" encoded value (e.g., using
> BitFields), but this should be fine.

I will keep it as-is for now.

http://codereview.chromium.org/6594037/diff/1/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2555: __
push(Immediate(Smi::FromInt(strict_mode_)));  // strict mode
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> Comment isn't necessary here. The variable is self-explanatory :).

Done.

http://codereview.chromium.org/6594037/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:3720: : kNonStrictMode)));
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> Is the conditional necessary here? It seems
> Code::ExtractExtraICStateFromFlags(flags) returns kStrictMode directly (or at
> most something castable to it).


Stritly speaking, the extra state is 2 bits so I think the best thing is to mask
out the strict mode bit and push that directly. Done that.

http://codereview.chromium.org/6594037/diff/1/src/ia32/virtual-frame-ia32.cc
File src/ia32/virtual-frame-ia32.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/ia32/virtual-frame-ia32.cc#...
src/ia32/virtual-frame-ia32.cc:1042: strict_mode == kStrictMode ?
Builtins::StoreIC_Initialize_Strict
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> Put parentheses around any non-trivial condition of a ?: operator.

Done across the change.

http://codereview.chromium.org/6594037/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/ic.cc#newcode1485
src/ic.cc:1485: // Strict mode doesn't allow setting non-existent global
property.
On 2011/02/28 12:23:06, Mads Ager wrote:
> Update comment to reflect the read-only part of this code.

Done.

http://codereview.chromium.org/6594037/diff/1/src/ic.cc#newcode1488
src/ic.cc:1488: return TypeError("strict_rdonly_property", object, name);
On 2011/02/28 12:23:06, Mads Ager wrote:
> Please don't abbreviate read to rd.

Done.

http://codereview.chromium.org/6594037/diff/1/src/ic.cc#newcode1947
src/ic.cc:1947: // ASSERT(extra_ic_state == ic.target()->extra_ic_state());
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> Should be removed before commit.

Done.

http://codereview.chromium.org/6594037/diff/1/src/ic.h
File src/ic.h (right):

http://codereview.chromium.org/6594037/diff/1/src/ic.h#newcode436
src/ic.h:436: ASSERT(code->extra_ic_state() == target()->extra_ic_state());
On 2011/02/28 11:35:05, Vitaly wrote:
> On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> > If the "extra_ic_state" is only strict/non-strict, just call it
> strict_ic_state
> > and return the correct enum value directly. The way you use it seems to
assume
> > that the value is exactly kStrictMode.
> > If we ever need more state, we can add more accessors.
> 
> Please don't kill extra_ic_state. It's also used as an opaque extra state to
> gather more type feedback. But having another accessor that wraps it is a
> probably a good idea. 

I am keeping the extra_ic_state but changing the condition to mask only for the
bit that this code cares about. This leaves the rest of the VM to use the other
bit.

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

http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode550
src/runtime.cc:550: // Passing non-strict per Ecma-262 5th Ed. 12.14. Catch,
bullet #4.
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> Upper-case ECMA.

Done.

http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode1184
src/runtime.cc:1184: // TODO(mmaly): Runtime_DeclareContextSlot. Needs strict
mode?
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> I don't think the declaration needs it.

Removed comment.

http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode1397
src/runtime.cc:1397: // TODO(mmaly): Strict mode not needed (const disallowed).
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> Agree. Remove TODOs (but retain rest of comment).

Done.

http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode1680
src/runtime.cc:1680: // No need for strict mode here. Called from SetupArray
(array.js).
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> Actually, if we make our builtins code strict, we should probably pass true
> here.
> Try using strict, and see if it works anyway.

I am not sure it really matters one way or another. the strict would only result
in an exception if the property already existed and was read only. It won't
affect in any way the attributes of the property or how the function is later
called. strict may be safer because if our initialization code has bugs we get
an exception rather than silently ignoring the assignment.

http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode3808
src/runtime.cc:3808: // TODO(mmaly): Implement SetElement strict mode.
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> Make an entry in the v8 bug-tracker for the feature and use the issue number
> instead of your name in the TODO.

Done. Issue 1220.

http://codereview.chromium.org/6594037/diff/1/src/x64/ic-x64.cc
File src/x64/ic-x64.cc (right):

http://codereview.chromium.org/6594037/diff/1/src/x64/ic-x64.cc#newcode1597
src/x64/ic-x64.cc:1597: void StoreIC::GenerateGlobalProxy(MacroAssembler* masm,
StrictModeFlag strict) {
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> Do! :)

Done across the board.