That was my intuition too, but I wasn't sure whether there may be some sneaky way to assign to something somehow differently (like get hold of the first class runtime environment and then do <environent>.arguments = <whatever>. I couldn't find any way to achieve that but that doesn't mean one doesn't exist. Now that you share the same view I think that this change is not necessary. Not all of it anyway, parts will still be required for getting eval work. I'll send update.
Martin On Tue, Feb 1, 2011 at 1:08 PM, Lasse R.H. Nielsen <[email protected]> wrote: > I have thought about this, and I am now wondering it is really necessary to > make the "arguments" variable non-writable at all. > In strict mode code, you can't assign to "arguments" (11.13.1 step 4), so > it should be invisible whether it's writable or not, and you shouldn't be > able to see the arguments variable from non-strict code or through any > aliasing of the variable. > > I.e., can we make a unit test that fails? :) > > Cheers > /L > > On Tue, Feb 1, 2011 at 18:08, <[email protected]> wrote: > >> Reviewers: Lasse Reichstein, Mads Ager, >> >> Description: >> Make arguments read only in strict mode. >> Propagate strict mode flag through the lazy compilation. >> >> BUG= >> TEST=test/mjsunit/strict-mode-arguments.js >> >> >> Please review this at http://codereview.chromium.org/6349020/ >> >> SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge >> >> Affected files: >> M src/compiler.cc >> M src/objects-inl.h >> M src/objects.h >> M src/parser.cc >> M src/scopes.h >> M src/scopes.cc >> M src/variables.h >> A test/mjsunit/strict-mode-arguments.js >> >> >> Index: src/compiler.cc >> diff --git a/src/compiler.cc b/src/compiler.cc >> index >> 5c18c3e53ebc8d6d3eb2dfc0e36301481621d20d..424bae80706093b7a22bc48bb9e9b326cf951b18 >> 100755 >> --- a/src/compiler.cc >> +++ b/src/compiler.cc >> @@ -762,6 +762,7 @@ void >> Compiler::SetFunctionInfo(Handle<SharedFunctionInfo> function_info, >> *lit->this_property_assignments()); >> function_info->set_try_full_codegen(lit->try_full_codegen()); >> >> function_info->set_allows_lazy_compilation(lit->AllowsLazyCompilation()); >> + function_info->set_strict_mode(lit->strict_mode()); >> } >> >> >> Index: src/objects-inl.h >> diff --git a/src/objects-inl.h b/src/objects-inl.h >> index >> db9e2ef7b10f95e01cfce86773eed94bd6a6ce32..a729db49b8863c10a79b429fc607054b53430ee2 >> 100644 >> --- a/src/objects-inl.h >> +++ b/src/objects-inl.h >> @@ -2984,6 +2984,18 @@ void >> SharedFunctionInfo::set_optimization_disabled(bool disable) { >> } >> >> >> +bool SharedFunctionInfo::strict_mode() { >> + return BooleanBit::get(compiler_hints(), kStrictModeFunction); >> +} >> + >> + >> +void SharedFunctionInfo::set_strict_mode(bool value) { >> + set_compiler_hints(BooleanBit::set(compiler_hints(), >> + kStrictModeFunction, >> + value)); >> +} >> + >> + >> ACCESSORS(CodeCache, default_cache, FixedArray, kDefaultCacheOffset) >> ACCESSORS(CodeCache, normal_type_cache, Object, kNormalTypeCacheOffset) >> >> Index: src/objects.h >> diff --git a/src/objects.h b/src/objects.h >> index >> 8c63022db8a0ac94928987882ddf258aabc599fd..0b81674e44983e36e56fbeb40d39e2ac1092e613 >> 100644 >> --- a/src/objects.h >> +++ b/src/objects.h >> @@ -4170,6 +4170,10 @@ class SharedFunctionInfo: public HeapObject { >> inline bool optimization_disabled(); >> inline void set_optimization_disabled(bool value); >> >> + // Indicates whether the function is a strict mode function. >> + inline bool strict_mode(); >> + inline void set_strict_mode(bool value); >> + >> // Indicates whether or not the code in the shared function support >> // deoptimization. >> inline bool has_deoptimization_support(); >> @@ -4351,6 +4355,7 @@ class SharedFunctionInfo: public HeapObject { >> static const int kCodeAgeShift = 4; >> static const int kCodeAgeMask = 0x7; >> static const int kOptimizationDisabled = 7; >> + static const int kStrictModeFunction = 8; >> >> DISALLOW_IMPLICIT_CONSTRUCTORS(SharedFunctionInfo); >> }; >> Index: src/parser.cc >> diff --git a/src/parser.cc b/src/parser.cc >> index >> c0976988ea3c077800842816c03981ee5cf7b248..8d88032eadbe4e5824f5af9b8e17b7111cb65805 >> 100644 >> --- a/src/parser.cc >> +++ b/src/parser.cc >> @@ -747,6 +747,10 @@ FunctionLiteral* >> Parser::ParseLazy(Handle<SharedFunctionInfo> info, >> scope); >> TemporaryScope temp_scope(&this->temp_scope_); >> >> + if (info->strict_mode()) { >> + temp_scope.EnableStrictMode(); >> + } >> + >> FunctionLiteralType type = >> info->is_expression() ? EXPRESSION : DECLARATION; >> bool ok = true; >> @@ -3540,7 +3544,7 @@ FunctionLiteral* >> Parser::ParseFunctionLiteral(Handle<String> var_name, >> end_pos = scanner().location().end_pos; >> } >> >> - // Validate strict mode. >> + // Strict mode. >> if (temp_scope_->StrictMode()) { >> if (IsEvalOrArguments(name)) { >> int position = function_token_position != RelocInfo::kNoPosition >> @@ -3564,6 +3568,9 @@ FunctionLiteral* >> Parser::ParseFunctionLiteral(Handle<String> var_name, >> return NULL; >> } >> CheckOctalLiteral(start_pos, end_pos, CHECK_OK); >> + >> + // In strict mode, arguments are const. >> + top_scope_->SetArgumentsConst(); >> } >> >> FunctionLiteral* function_literal = >> Index: src/scopes.cc >> diff --git a/src/scopes.cc b/src/scopes.cc >> index >> d3f54ad3f2d9ad8b47fefae2d89fd692bd3e2eda..afc89ac98d2ddde3222385bc114665ac5b208b76 >> 100644 >> --- a/src/scopes.cc >> +++ b/src/scopes.cc >> @@ -491,6 +491,15 @@ int Scope::ContextChainLength(Scope* scope) { >> } >> >> >> +// In strict mode, arguments are const. >> +void Scope::SetArgumentsConst() { >> + Variable* arguments = LocalLookup(Factory::arguments_symbol()); >> + ASSERT(arguments != NULL); // functions have 'arguments' declared >> implicitly >> + ASSERT(arguments->mode() == Variable::VAR); >> + arguments->set_mode(Variable::CONST); >> +} >> + >> + >> #ifdef DEBUG >> static const char* Header(Scope::Type type) { >> switch (type) { >> Index: src/scopes.h >> diff --git a/src/scopes.h b/src/scopes.h >> index >> a9220eb827566e94e7a126f1085bdc90d6c3a67c..a8f2db4e03e844a5e2bcd3a77405072d627131d4 >> 100644 >> --- a/src/scopes.h >> +++ b/src/scopes.h >> @@ -299,6 +299,8 @@ class Scope: public ZoneObject { >> return variables_.Lookup(name) != NULL; >> } >> >> + void SetArgumentsConst(); >> + >> // >> --------------------------------------------------------------------------- >> // Debugging. >> >> Index: src/variables.h >> diff --git a/src/variables.h b/src/variables.h >> index >> 5d27a02d5375ec70b84934e637b0a18a0e621f41..75af977c908c771ec0ffecebf572f433e76ef88f >> 100644 >> --- a/src/variables.h >> +++ b/src/variables.h >> @@ -135,6 +135,8 @@ class Variable: public ZoneObject { >> >> Handle<String> name() const { return name_; } >> Mode mode() const { return mode_; } >> + void set_mode(Mode mode) { mode_ = mode; } >> + >> bool is_accessed_from_inner_scope() const { >> return is_accessed_from_inner_scope_; >> } >> Index: test/mjsunit/strict-mode-arguments.js >> diff --git a/test/mjsunit/strict-mode-arguments.js >> b/test/mjsunit/strict-mode-arguments.js >> new file mode 100644 >> index >> 0000000000000000000000000000000000000000..ac57e77a6742432bf63ab19726ac6b8173295193 >> --- /dev/null >> +++ b/test/mjsunit/strict-mode-arguments.js >> @@ -0,0 +1,44 @@ >> +// Copyright 2011 the V8 project authors. All rights reserved. >> +// Redistribution and use in source and binary forms, with or without >> +// modification, are permitted provided that the following conditions are >> +// met: >> +// >> +// * Redistributions of source code must retain the above copyright >> +// notice, this list of conditions and the following disclaimer. >> +// * Redistributions in binary form must reproduce the above >> +// copyright notice, this list of conditions and the following >> +// disclaimer in the documentation and/or other materials provided >> +// with the distribution. >> +// * Neither the name of Google Inc. nor the names of its >> +// contributors may be used to endorse or promote products derived >> +// from this software without specific prior written permission. >> +// >> +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS >> +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT >> +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR >> +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT >> +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, >> +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT >> +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, >> +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY >> +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT >> +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE >> +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. >> + >> +"use strict"; >> + >> +// arguments must be read only in strict mode. >> +// NOTE: this test will be obsoleted in future changes when >> +// assignment to arguments/eval will become syntax error in >> +// strict mode eval code >> +function TestArgumentsIsReadOnlyInStrict() { >> + // Lazy compiled. It verifies that the strict mode is correctly >> + // propagated from the initial parse to lazy compilation of the >> function. >> + function inner(a) { >> + eval('arguments = ["assigned"];'); >> + return arguments[0]; >> + } >> + return inner('argument'); >> +} >> + >> +assertEquals('argument', TestArgumentsIsReadOnlyInStrict()); >> >> >> > > > -- > Lasse R.H. Nielsen > [email protected] > 'Faith without judgement merely degrades the spirit divine' > Google Denmark ApS - Frederiksborggade 20B, 1 sal - 1360 København K - > Denmark - CVR nr. 28 86 69 84 > > -- > v8-dev mailing list > [email protected] > http://groups.google.com/group/v8-dev -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
