Reviewers: Mads Ager,
Message:
Fix for bug 1176. An invalid assumption in the assert.
http://codereview.chromium.org/6469083/diff/1/src/ia32/codegen-ia32.cc
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/6469083/diff/1/src/ia32/codegen-ia32.cc#newcode8238
src/ia32/codegen-ia32.cc:8238: ASSERT(strict_mode_flag() ==
kNonStrictMode || variable->is_this());
It is not the absolutely most paranoid version of the assert one can
come up with. Strictly speaking, only 1 branches out of the 3 below can
see "this" but I feel this assert is quite adequate for the purpose it
serves. Alternative would be to litter the code with 2 more asserts and
more comments.
I believe this strikes the right balance.
http://codereview.chromium.org/6469083/diff/1/src/ia32/codegen-ia32.cc#newcode8260
src/ia32/codegen-ia32.cc:8260: frame_->Push(Factory::false_value());
aligning the code layout of ia32, x64 and arm codegens, they were
inconsistent.
Description:
Fix for bug http://code.google.com/p/v8/issues/detail?id=1176.
Updated the assert assumption of which was not correct with
respect to "delete this".
BUG=http://code.google.com/p/v8/issues/detail?id=1176.
TEST=test/mjsunit/regress/regress-1176.js
Please review this at http://codereview.chromium.org/6469083/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files:
M src/arm/codegen-arm.cc
M src/arm/full-codegen-arm.cc
M src/ia32/codegen-ia32.cc
M src/ia32/full-codegen-ia32.cc
M src/x64/codegen-x64.cc
M src/x64/full-codegen-x64.cc
A test/mjsunit/regress/regress-1176.js
M test/mjsunit/strict-mode.js
Index: src/arm/codegen-arm.cc
diff --git a/src/arm/codegen-arm.cc b/src/arm/codegen-arm.cc
index
a3921d8efccb6a2bc2596f2d715aca135aa5947d..b83f33d530da71a58fe271af1aeea16e1c11947d
100644
--- a/src/arm/codegen-arm.cc
+++ b/src/arm/codegen-arm.cc
@@ -5850,8 +5850,8 @@ void
CodeGenerator::VisitUnaryOperation(UnaryOperation* node) {
} else if (variable != NULL) {
// Delete of an unqualified identifier is disallowed in strict mode
- // so this code can only be reached in non-strict mode.
- ASSERT(strict_mode_flag() == kNonStrictMode);
+ // (but "delete this" is).
+ ASSERT(strict_mode_flag() == kNonStrictMode || variable->is_this());
Slot* slot = variable->AsSlot();
if (variable->is_global()) {
LoadGlobal();
Index: src/arm/full-codegen-arm.cc
diff --git a/src/arm/full-codegen-arm.cc b/src/arm/full-codegen-arm.cc
index
9b589e61c3dc12cb445f5adcc6b09c5512722f6d..56d92f892dabe1f70909c3163b7dcc2c79a61762
100644
--- a/src/arm/full-codegen-arm.cc
+++ b/src/arm/full-codegen-arm.cc
@@ -3373,8 +3373,8 @@ void
FullCodeGenerator::VisitUnaryOperation(UnaryOperation* expr) {
}
} else if (var != NULL) {
// Delete of an unqualified identifier is disallowed in strict mode
- // so this code can only be reached in non-strict mode.
- ASSERT(strict_mode_flag() == kNonStrictMode);
+ // (but "delete this" is).
+ ASSERT(strict_mode_flag() == kNonStrictMode || var->is_this());
if (var->is_global()) {
__ ldr(r2, GlobalObjectOperand());
__ mov(r1, Operand(var->name()));
Index: src/ia32/codegen-ia32.cc
diff --git a/src/ia32/codegen-ia32.cc b/src/ia32/codegen-ia32.cc
index
02e29191dc60031b97cf48ab3059f1d4a32e266c..ecdc8f2fd3fed48793056877746de37923773932
100644
--- a/src/ia32/codegen-ia32.cc
+++ b/src/ia32/codegen-ia32.cc
@@ -8234,8 +8234,8 @@ void
CodeGenerator::VisitUnaryOperation(UnaryOperation* node) {
Variable* variable =
node->expression()->AsVariableProxy()->AsVariable();
if (variable != NULL) {
// Delete of an unqualified identifier is disallowed in strict mode
- // so this code can only be reached in non-strict mode.
- ASSERT(strict_mode_flag() == kNonStrictMode);
+ // (but "delete this" is).
+ ASSERT(strict_mode_flag() == kNonStrictMode || variable->is_this());
Slot* slot = variable->AsSlot();
if (variable->is_global()) {
LoadGlobal();
@@ -8244,7 +8244,6 @@ void
CodeGenerator::VisitUnaryOperation(UnaryOperation* node) {
Result answer = frame_->InvokeBuiltin(Builtins::DELETE,
CALL_FUNCTION, 3);
frame_->Push(&answer);
- return;
} else if (slot != NULL && slot->type() == Slot::LOOKUP) {
// Call the runtime to delete from the context holding the named
@@ -8255,13 +8254,11 @@ void
CodeGenerator::VisitUnaryOperation(UnaryOperation* node) {
frame_->EmitPush(Immediate(variable->name()));
Result answer = frame_->CallRuntime(Runtime::kDeleteContextSlot,
2);
frame_->Push(&answer);
- return;
+ } else {
+ // Default: Result of deleting non-global, not dynamically
+ // introduced variables is false.
+ frame_->Push(Factory::false_value());
}
-
- // Default: Result of deleting non-global, not dynamically
- // introduced variables is false.
- frame_->Push(Factory::false_value());
-
} else {
// Default: Result of deleting expressions is true.
Load(node->expression()); // may have side-effects
Index: src/ia32/full-codegen-ia32.cc
diff --git a/src/ia32/full-codegen-ia32.cc b/src/ia32/full-codegen-ia32.cc
index
d32e9aa87732c40716de28fbb8ec0e154ac614bc..998f5ac1dbc068ca52660a143fb2f1f2d97e0533
100644
--- a/src/ia32/full-codegen-ia32.cc
+++ b/src/ia32/full-codegen-ia32.cc
@@ -3743,8 +3743,8 @@ void
FullCodeGenerator::VisitUnaryOperation(UnaryOperation* expr) {
}
} else if (var != NULL) {
// Delete of an unqualified identifier is disallowed in strict mode
- // so this code can only be reached in non-strict mode.
- ASSERT(strict_mode_flag() == kNonStrictMode);
+ // (but "delete this" is).
+ ASSERT(strict_mode_flag() == kNonStrictMode || var->is_this());
if (var->is_global()) {
__ push(GlobalObjectOperand());
__ push(Immediate(var->name()));
Index: src/x64/codegen-x64.cc
diff --git a/src/x64/codegen-x64.cc b/src/x64/codegen-x64.cc
index
150ed664b0a4b6c300e0551f54c4fa1bdd212d92..3a6491bd64d4f99aa144168ddafb20f6a068ea67
100644
--- a/src/x64/codegen-x64.cc
+++ b/src/x64/codegen-x64.cc
@@ -7239,8 +7239,8 @@ void
CodeGenerator::VisitUnaryOperation(UnaryOperation* node) {
Variable* variable =
node->expression()->AsVariableProxy()->AsVariable();
if (variable != NULL) {
// Delete of an unqualified identifier is disallowed in strict mode
- // so this code can only be reached in non-strict mode.
- ASSERT(strict_mode_flag() == kNonStrictMode);
+ // (but "delete this" is).
+ ASSERT(strict_mode_flag() == kNonStrictMode || variable->is_this());
Slot* slot = variable->AsSlot();
if (variable->is_global()) {
LoadGlobal();
@@ -7249,7 +7249,6 @@ void
CodeGenerator::VisitUnaryOperation(UnaryOperation* node) {
Result answer = frame_->InvokeBuiltin(Builtins::DELETE,
CALL_FUNCTION, 3);
frame_->Push(&answer);
- return;
} else if (slot != NULL && slot->type() == Slot::LOOKUP) {
// Call the runtime to delete from the context holding the named
@@ -7260,13 +7259,11 @@ void
CodeGenerator::VisitUnaryOperation(UnaryOperation* node) {
frame_->EmitPush(variable->name());
Result answer = frame_->CallRuntime(Runtime::kDeleteContextSlot,
2);
frame_->Push(&answer);
- return;
+ } else {
+ // Default: Result of deleting non-global, not dynamically
+ // introduced variables is false.
+ frame_->Push(Factory::false_value());
}
-
- // Default: Result of deleting non-global, not dynamically
- // introduced variables is false.
- frame_->Push(Factory::false_value());
-
} else {
// Default: Result of deleting expressions is true.
Load(node->expression()); // may have side-effects
Index: src/x64/full-codegen-x64.cc
diff --git a/src/x64/full-codegen-x64.cc b/src/x64/full-codegen-x64.cc
index
11b07d770a213d570114c54021ef9b5d8e135b53..787f705a0fd22c6fec991e1eefd62a9284e7d93b
100644
--- a/src/x64/full-codegen-x64.cc
+++ b/src/x64/full-codegen-x64.cc
@@ -3075,8 +3075,8 @@ void
FullCodeGenerator::VisitUnaryOperation(UnaryOperation* expr) {
}
} else if (var != NULL) {
// Delete of an unqualified identifier is disallowed in strict mode
- // so this code can only be reached in non-strict mode.
- ASSERT(strict_mode_flag() == kNonStrictMode);
+ // (but "delete this" is).
+ ASSERT(strict_mode_flag() == kNonStrictMode || var->is_this());
if (var->is_global()) {
__ push(GlobalObjectOperand());
__ Push(var->name());
Index: test/mjsunit/regress/regress-1176.js
diff --git a/test/mjsunit/regress/regress-1176.js
b/test/mjsunit/regress/regress-1176.js
new file mode 100644
index
0000000000000000000000000000000000000000..58eda1bf360214c33b589397ad0459e02aa986cb
--- /dev/null
+++ b/test/mjsunit/regress/regress-1176.js
@@ -0,0 +1,33 @@
+// 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";
+function strict_delete_this() {
+ // "delete this" is allowed in strict mode.
+ delete this;
+}
+strict_delete_this();
Index: test/mjsunit/strict-mode.js
diff --git a/test/mjsunit/strict-mode.js b/test/mjsunit/strict-mode.js
index
fbba64ed66606c125dbbec74a5849402c3b41144..e0e5e8369e8d0a948d095056fafddfc346fb2c67
100644
--- a/test/mjsunit/strict-mode.js
+++ b/test/mjsunit/strict-mode.js
@@ -291,6 +291,13 @@ CheckStrictMode("function strict() { var variable;
delete variable; }",
SyntaxError);
CheckStrictMode("var variable; delete variable;", SyntaxError);
+(function TestStrictDelete() {
+ "use strict";
+ // "delete this" is allowed in strict mode and should work.
+ function strict_delete() { delete this; }
+ strict_delete();
+})();
+
// Prefix unary operators other than delete, ++, -- are valid in strict
mode
(function StrictModeUnaryOperators() {
"use strict";
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
