Reviewers: Lasse Reichstein, Mads Ager,
Message:
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.
Description:
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/
Please review this at http://codereview.chromium.org/6594037/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files:
M src/api.cc
M src/arm/codegen-arm.cc
M src/arm/full-codegen-arm.cc
M src/arm/ic-arm.cc
M src/arm/lithium-codegen-arm.cc
M src/arm/stub-cache-arm.cc
M src/arm/virtual-frame-arm.h
M src/arm/virtual-frame-arm.cc
M src/builtins.h
M src/builtins.cc
M src/debug.cc
M src/handles.h
M src/handles.cc
M src/ia32/codegen-ia32.cc
M src/ia32/full-codegen-ia32.cc
M src/ia32/ic-ia32.cc
M src/ia32/lithium-codegen-ia32.cc
M src/ia32/stub-cache-ia32.cc
M src/ia32/virtual-frame-ia32.h
M src/ia32/virtual-frame-ia32.cc
M src/ic-inl.h
M src/ic.h
M src/ic.cc
M src/messages.js
M src/objects-inl.h
M src/objects.h
M src/objects.cc
M src/parser.cc
M src/runtime.h
M src/runtime.cc
M src/stub-cache.h
M src/stub-cache.cc
M src/x64/codegen-x64.cc
M src/x64/full-codegen-x64.cc
M src/x64/ic-x64.cc
M src/x64/lithium-codegen-x64.cc
M src/x64/stub-cache-x64.cc
M src/x64/virtual-frame-x64.h
M src/x64/virtual-frame-x64.cc
M test/cctest/test-api.cc
M test/cctest/test-compiler.cc
M test/cctest/test-debug.cc
M test/cctest/test-heap.cc
M test/cctest/test-mark-compact.cc
M test/es5conform/es5conform.status
M test/mjsunit/strict-mode.js
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
