Revision: 6508
Author: [email protected]
Date: Thu Jan 27 00:35:39 2011
Log: Fix reintroduction of global variables that have been deleted.
Deletion of global properties puts 'the hole' in the global property
cell and updates the property details in the property dictionary with
the information that the property has been deleted. When setting
global properties that have been deleted in generated code we just
store the new value in the global property cell. This does not update
the property details in the property dictionary. Therefore, it looks
like the property is not there eventhough it was just reintroduced.
Perform 'the hole' checks in generated code for global property stores
and bail out of ICs and optimized code if storing to a property cell
that contains 'the hole'.
Review URL: http://codereview.chromium.org/6306014
http://code.google.com/p/v8/source/detail?r=6508
Modified:
/branches/bleeding_edge/src/arm/lithium-arm.cc
/branches/bleeding_edge/src/arm/lithium-arm.h
/branches/bleeding_edge/src/arm/lithium-codegen-arm.cc
/branches/bleeding_edge/src/arm/stub-cache-arm.cc
/branches/bleeding_edge/src/hydrogen-instructions.h
/branches/bleeding_edge/src/hydrogen.cc
/branches/bleeding_edge/src/ia32/lithium-codegen-ia32.cc
/branches/bleeding_edge/src/ia32/lithium-ia32.cc
/branches/bleeding_edge/src/ia32/stub-cache-ia32.cc
/branches/bleeding_edge/src/objects-inl.h
/branches/bleeding_edge/src/x64/stub-cache-x64.cc
/branches/bleeding_edge/test/mjsunit/delete-global-properties.js
=======================================
--- /branches/bleeding_edge/src/arm/lithium-arm.cc Wed Jan 26 19:22:08 2011
+++ /branches/bleeding_edge/src/arm/lithium-arm.cc Thu Jan 27 00:35:39 2011
@@ -1602,7 +1602,14 @@
LInstruction* LChunkBuilder::DoStoreGlobal(HStoreGlobal* instr) {
- return new LStoreGlobal(UseRegisterAtStart(instr->value()));
+ if (instr->check_hole_value()) {
+ LOperand* temp = TempRegister();
+ LOperand* value = UseRegister(instr->value());
+ return AssignEnvironment(new LStoreGlobal(value, temp));
+ } else {
+ LOperand* value = UseRegisterAtStart(instr->value());
+ return new LStoreGlobal(value, NULL);
+ }
}
=======================================
--- /branches/bleeding_edge/src/arm/lithium-arm.h Wed Jan 26 00:03:48 2011
+++ /branches/bleeding_edge/src/arm/lithium-arm.h Thu Jan 27 00:35:39 2011
@@ -1254,10 +1254,11 @@
};
-class LStoreGlobal: public LTemplateInstruction<0, 1, 0> {
+class LStoreGlobal: public LTemplateInstruction<0, 1, 1> {
public:
- explicit LStoreGlobal(LOperand* value) {
+ LStoreGlobal(LOperand* value, LOperand* temp) {
inputs_[0] = value;
+ temps_[0] = temp;
}
DECLARE_CONCRETE_INSTRUCTION(StoreGlobal, "store-global")
=======================================
--- /branches/bleeding_edge/src/arm/lithium-codegen-arm.cc Wed Jan 26
19:22:08 2011
+++ /branches/bleeding_edge/src/arm/lithium-codegen-arm.cc Thu Jan 27
00:35:39 2011
@@ -2195,8 +2195,26 @@
void LCodeGen::DoStoreGlobal(LStoreGlobal* instr) {
Register value = ToRegister(instr->InputAt(0));
- __ mov(ip, Operand(Handle<Object>(instr->hydrogen()->cell())));
- __ str(value, FieldMemOperand(ip, JSGlobalPropertyCell::kValueOffset));
+ Register scratch = scratch0();
+
+ // Load the cell.
+ __ mov(scratch, Operand(Handle<Object>(instr->hydrogen()->cell())));
+
+ // If the cell we are storing to contains the hole it could have
+ // been deleted from the property dictionary. In that case, we need
+ // to update the property details in the property dictionary to mark
+ // it as no longer deleted.
+ if (instr->hydrogen()->check_hole_value()) {
+ Register scratch2 = ToRegister(instr->TempAt(0));
+ __ ldr(scratch2,
+ FieldMemOperand(scratch, JSGlobalPropertyCell::kValueOffset));
+ __ LoadRoot(ip, Heap::kTheHoleValueRootIndex);
+ __ cmp(scratch2, ip);
+ DeoptimizeIf(eq, instr->environment());
+ }
+
+ // Store the value.
+ __ str(value, FieldMemOperand(scratch,
JSGlobalPropertyCell::kValueOffset));
}
=======================================
--- /branches/bleeding_edge/src/arm/stub-cache-arm.cc Wed Jan 26 00:12:56
2011
+++ /branches/bleeding_edge/src/arm/stub-cache-arm.cc Thu Jan 27 00:35:39
2011
@@ -2649,9 +2649,18 @@
__ cmp(r3, Operand(Handle<Map>(object->map())));
__ b(ne, &miss);
+ // Check that the value in the cell is not the hole. If it is, this
+ // cell could have been deleted and reintroducing the global needs
+ // to update the property details in the property dictionary of the
+ // global object. We bail out to the runtime system to do that.
+ __ mov(r4, Operand(Handle<JSGlobalPropertyCell>(cell)));
+ __ LoadRoot(r5, Heap::kTheHoleValueRootIndex);
+ __ ldr(r6, FieldMemOperand(r4, JSGlobalPropertyCell::kValueOffset));
+ __ cmp(r5, r6);
+ __ b(eq, &miss);
+
// Store the value in the cell.
- __ mov(r2, Operand(Handle<JSGlobalPropertyCell>(cell)));
- __ str(r0, FieldMemOperand(r2, JSGlobalPropertyCell::kValueOffset));
+ __ str(r0, FieldMemOperand(r4, JSGlobalPropertyCell::kValueOffset));
__ IncrementCounter(&Counters::named_store_global_inline, 1, r4, r3);
__ Ret();
=======================================
--- /branches/bleeding_edge/src/hydrogen-instructions.h Wed Jan 26 09:32:02
2011
+++ /branches/bleeding_edge/src/hydrogen-instructions.h Thu Jan 27 00:35:39
2011
@@ -2575,12 +2575,17 @@
class HStoreGlobal: public HUnaryOperation {
public:
- HStoreGlobal(HValue* value, Handle<JSGlobalPropertyCell> cell)
- : HUnaryOperation(value), cell_(cell) {
+ HStoreGlobal(HValue* value,
+ Handle<JSGlobalPropertyCell> cell,
+ bool check_hole_value)
+ : HUnaryOperation(value),
+ cell_(cell),
+ check_hole_value_(check_hole_value) {
SetFlag(kChangesGlobalVars);
}
Handle<JSGlobalPropertyCell> cell() const { return cell_; }
+ bool check_hole_value() const { return check_hole_value_; }
virtual Representation RequiredInputRepresentation(int index) const {
return Representation::Tagged();
@@ -2591,6 +2596,7 @@
private:
Handle<JSGlobalPropertyCell> cell_;
+ bool check_hole_value_;
};
=======================================
--- /branches/bleeding_edge/src/hydrogen.cc Wed Jan 26 09:30:22 2011
+++ /branches/bleeding_edge/src/hydrogen.cc Thu Jan 27 00:35:39 2011
@@ -3370,9 +3370,10 @@
LookupGlobalPropertyCell(var, &lookup, true);
CHECK_BAILOUT;
+ bool check_hole = !lookup.IsDontDelete() || lookup.IsReadOnly();
Handle<GlobalObject> global(graph()->info()->global_object());
Handle<JSGlobalPropertyCell> cell(global->GetPropertyCell(&lookup));
- HInstruction* instr = new HStoreGlobal(value, cell);
+ HInstruction* instr = new HStoreGlobal(value, cell, check_hole);
instr->set_position(position);
AddInstruction(instr);
if (instr->HasSideEffects()) AddSimulate(ast_id);
=======================================
--- /branches/bleeding_edge/src/ia32/lithium-codegen-ia32.cc Wed Jan 26
12:48:48 2011
+++ /branches/bleeding_edge/src/ia32/lithium-codegen-ia32.cc Thu Jan 27
00:35:39 2011
@@ -1911,7 +1911,19 @@
void LCodeGen::DoStoreGlobal(LStoreGlobal* instr) {
Register value = ToRegister(instr->InputAt(0));
- __ mov(Operand::Cell(instr->hydrogen()->cell()), value);
+ Operand cell_operand = Operand::Cell(instr->hydrogen()->cell());
+
+ // If the cell we are storing to contains the hole it could have
+ // been deleted from the property dictionary. In that case, we need
+ // to update the property details in the property dictionary to mark
+ // it as no longer deleted. We deoptimize in that case.
+ if (instr->hydrogen()->check_hole_value()) {
+ __ cmp(cell_operand, Factory::the_hole_value());
+ DeoptimizeIf(equal, instr->environment());
+ }
+
+ // Store the value.
+ __ mov(cell_operand, value);
}
=======================================
--- /branches/bleeding_edge/src/ia32/lithium-ia32.cc Wed Jan 26 00:03:48
2011
+++ /branches/bleeding_edge/src/ia32/lithium-ia32.cc Thu Jan 27 00:35:39
2011
@@ -1645,7 +1645,8 @@
LInstruction* LChunkBuilder::DoStoreGlobal(HStoreGlobal* instr) {
- return new LStoreGlobal(UseRegisterAtStart(instr->value()));
+ LStoreGlobal* result = new
LStoreGlobal(UseRegisterAtStart(instr->value()));
+ return instr->check_hole_value() ? AssignEnvironment(result) : result;
}
=======================================
--- /branches/bleeding_edge/src/ia32/stub-cache-ia32.cc Wed Jan 26 00:12:56
2011
+++ /branches/bleeding_edge/src/ia32/stub-cache-ia32.cc Thu Jan 27 00:35:39
2011
@@ -2582,13 +2582,23 @@
Immediate(Handle<Map>(object->map())));
__ j(not_equal, &miss, not_taken);
- // Store the value in the cell.
+
+ // Compute the cell operand to use.
+ Operand cell_operand = Operand::Cell(Handle<JSGlobalPropertyCell>(cell));
if (Serializer::enabled()) {
__ mov(ecx, Immediate(Handle<JSGlobalPropertyCell>(cell)));
- __ mov(FieldOperand(ecx, JSGlobalPropertyCell::kValueOffset), eax);
- } else {
- __ mov(Operand::Cell(Handle<JSGlobalPropertyCell>(cell)), eax);
- }
+ cell_operand = FieldOperand(ecx, JSGlobalPropertyCell::kValueOffset);
+ }
+
+ // Check that the value in the cell is not the hole. If it is, this
+ // cell could have been deleted and reintroducing the global needs
+ // to update the property details in the property dictionary of the
+ // global object. We bail out to the runtime system to do that.
+ __ cmp(cell_operand, Factory::the_hole_value());
+ __ j(equal, &miss);
+
+ // Store the value in the cell.
+ __ mov(cell_operand, eax);
// Return the value (register eax).
__ IncrementCounter(&Counters::named_store_global_inline, 1);
=======================================
--- /branches/bleeding_edge/src/objects-inl.h Wed Jan 19 12:05:22 2011
+++ /branches/bleeding_edge/src/objects-inl.h Thu Jan 27 00:35:39 2011
@@ -57,8 +57,7 @@
PropertyDetails PropertyDetails::AsDeleted() {
- PropertyDetails d(DONT_ENUM, NORMAL);
- Smi* smi = Smi::FromInt(AsSmi()->value() | DeletedField::encode(1));
+ Smi* smi = Smi::FromInt(value_ | DeletedField::encode(1));
return PropertyDetails(smi);
}
=======================================
--- /branches/bleeding_edge/src/x64/stub-cache-x64.cc Wed Jan 26 00:12:56
2011
+++ /branches/bleeding_edge/src/x64/stub-cache-x64.cc Thu Jan 27 00:35:39
2011
@@ -2438,9 +2438,17 @@
Handle<Map>(object->map()));
__ j(not_equal, &miss);
+ // Check that the value in the cell is not the hole. If it is, this
+ // cell could have been deleted and reintroducing the global needs
+ // to update the property details in the property dictionary of the
+ // global object. We bail out to the runtime system to do that.
+ __ Move(rbx, Handle<JSGlobalPropertyCell>(cell));
+ __ CompareRoot(FieldOperand(rbx, JSGlobalPropertyCell::kValueOffset),
+ Heap::kTheHoleValueRootIndex);
+ __ j(equal, &miss);
+
// Store the value in the cell.
- __ Move(rcx, Handle<JSGlobalPropertyCell>(cell));
- __ movq(FieldOperand(rcx, JSGlobalPropertyCell::kValueOffset), rax);
+ __ movq(FieldOperand(rbx, JSGlobalPropertyCell::kValueOffset), rax);
// Return the value (register rax).
__ IncrementCounter(&Counters::named_store_global_inline, 1);
=======================================
--- /branches/bleeding_edge/test/mjsunit/delete-global-properties.js Tue
Dec 7 03:01:02 2010
+++ /branches/bleeding_edge/test/mjsunit/delete-global-properties.js Thu
Jan 27 00:35:39 2011
@@ -1,4 +1,4 @@
-// Copyright 2008 the V8 project authors. All rights reserved.
+// 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:
@@ -32,6 +32,17 @@
assertTrue("tmp" in this);
function f() { return 1; }
assertFalse(delete f); // should be DONT_DELETE
-assertEquals(1, f());
-
-/* Perhaps related to bugs/11? */
+assertEquals(1, f());
+
+// Check that deleting and reintroducing global variables works.
+// Get into the IC case for storing to a deletable global property.
+function introduce_x() { x = 42; }
+for (var i = 0; i < 10; i++) introduce_x();
+// Check that the property has been introduced.
+assertTrue(this.hasOwnProperty('x'));
+// Check that deletion works.
+delete x;
+assertFalse(this.hasOwnProperty('x'));
+// Check that reintroduction works.
+introduce_x();
+assertTrue(this.hasOwnProperty('x'));
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
