Revision: 10788
Author: svenpa...@chromium.org
Date: Wed Feb 22 02:52:57 2012
Log: Cleaned up setting of accessors.
This CL is an intermediate step only, in the end we need to have a single
DefineOrRedefineAccessorProperty call for a single Object.defineProperty
call. Currently we can end up making two such calls, making the necessary
access
checks extremely ugly and hard (impossible?) to get right for complete spec
conformance.
The bulk of the change is quite mechanical:
* Prepare an AccessorPair *before* we add it to our data structures,
eliminating the previous voodoo-like threading of a placeholder.
* The previous item makes it possible to activate our check that we do not
share AccessorPairs by accident.
* Split a monster method into 2 quite unrelated methods.
* Use templated To method in a few places.
Review URL: https://chromiumcodereview.appspot.com/9428026
http://code.google.com/p/v8/source/detail?r=10788
Modified:
/branches/bleeding_edge/src/heap.cc
/branches/bleeding_edge/src/objects.cc
/branches/bleeding_edge/src/objects.h
=======================================
--- /branches/bleeding_edge/src/heap.cc Mon Feb 20 06:02:59 2012
+++ /branches/bleeding_edge/src/heap.cc Wed Feb 22 02:52:57 2012
@@ -5081,9 +5081,7 @@
lo_space_->Verify();
- // TODO(svenpanne) We should enable this when our fast->slow->fast-mode
dance
- // for setting accessor properties is fixed.
- // VerifyNoAccessorPairSharing();
+ VerifyNoAccessorPairSharing();
}
=======================================
--- /branches/bleeding_edge/src/objects.cc Tue Feb 21 06:09:45 2012
+++ /branches/bleeding_edge/src/objects.cc Wed Feb 22 02:52:57 2012
@@ -4362,15 +4362,14 @@
}
-// Search for a getter or setter in an elements dictionary and update its
-// attributes. Returns either undefined if the element is non-deletable,
or the
-// getter/setter pair if there is an existing one, or the hole value if the
-// element does not exist or is a normal non-getter/setter data element.
-static Object* UpdateGetterSetterInDictionary(
+// Try to update an accessor in an elements dictionary. Return true if the
+// update succeeded, and false otherwise.
+static bool UpdateGetterSetterInDictionary(
SeededNumberDictionary* dictionary,
uint32_t index,
- PropertyAttributes attributes,
- Heap* heap) {
+ bool is_getter,
+ Object* fun,
+ PropertyAttributes attributes) {
int entry = dictionary->FindEntry(index);
if (entry != SeededNumberDictionary::kNotFound) {
Object* result = dictionary->ValueAt(entry);
@@ -4382,108 +4381,116 @@
dictionary->DetailsAtPut(entry,
PropertyDetails(attributes, CALLBACKS,
index));
}
- return result;
+ AccessorPair::cast(result)->set(is_getter, fun);
+ return true;
}
}
- return heap->the_hole_value();
+ return false;
}
-MaybeObject* JSObject::DefineGetterSetter(String* name,
- PropertyAttributes attributes) {
- Heap* heap = GetHeap();
- // Make sure that the top context does not change when doing callbacks or
- // interceptor calls.
- AssertNoContextChange ncc;
-
- // Try to flatten before operating on the string.
- name->TryFlatten();
-
- if (!CanSetCallback(name)) {
- return heap->undefined_value();
- }
-
- uint32_t index = 0;
- bool is_element = name->AsArrayIndex(&index);
-
- if (is_element) {
- switch (GetElementsKind()) {
- case FAST_SMI_ONLY_ELEMENTS:
- case FAST_ELEMENTS:
- case FAST_DOUBLE_ELEMENTS:
- break;
- case EXTERNAL_PIXEL_ELEMENTS:
- case EXTERNAL_BYTE_ELEMENTS:
- case EXTERNAL_UNSIGNED_BYTE_ELEMENTS:
- case EXTERNAL_SHORT_ELEMENTS:
- case EXTERNAL_UNSIGNED_SHORT_ELEMENTS:
- case EXTERNAL_INT_ELEMENTS:
- case EXTERNAL_UNSIGNED_INT_ELEMENTS:
- case EXTERNAL_FLOAT_ELEMENTS:
- case EXTERNAL_DOUBLE_ELEMENTS:
- // Ignore getters and setters on pixel and external array
- // elements.
- return heap->undefined_value();
- case DICTIONARY_ELEMENTS: {
- Object* probe =
UpdateGetterSetterInDictionary(element_dictionary(),
- index,
- attributes,
- heap);
- if (!probe->IsTheHole()) return probe;
- // Otherwise allow to override it.
- break;
- }
- case NON_STRICT_ARGUMENTS_ELEMENTS: {
- // Ascertain whether we have read-only properties or an existing
- // getter/setter pair in an arguments elements dictionary backing
- // store.
- FixedArray* parameter_map = FixedArray::cast(elements());
- uint32_t length = parameter_map->length();
- Object* probe =
- index < (length - 2) ? parameter_map->get(index + 2) : NULL;
- if (probe == NULL || probe->IsTheHole()) {
- FixedArray* arguments = FixedArray::cast(parameter_map->get(1));
- if (arguments->IsDictionary()) {
- SeededNumberDictionary* dictionary =
- SeededNumberDictionary::cast(arguments);
- probe = UpdateGetterSetterInDictionary(dictionary,
- index,
- attributes,
- heap);
- if (!probe->IsTheHole()) return probe;
- }
- }
- break;
- }
- }
- } else {
- // Lookup the name.
- LookupResult result(heap->isolate());
- LocalLookupRealNamedProperty(name, &result);
- if (result.IsFound()) {
- // TODO(mstarzinger): We should check for result.IsDontDelete() here
once
- // we only call into the runtime once to set both getter and setter.
- if (result.type() == CALLBACKS) {
- Object* obj = result.GetCallbackObject();
- // Need to preserve old getters/setters.
- if (obj->IsAccessorPair()) {
- // Use set to update attributes.
- return SetPropertyCallback(name, obj, attributes);
- }
+MaybeObject* JSObject::DefineElementAccessor(uint32_t index,
+ bool is_getter,
+ Object* fun,
+ PropertyAttributes
attributes) {
+ switch (GetElementsKind()) {
+ case FAST_SMI_ONLY_ELEMENTS:
+ case FAST_ELEMENTS:
+ case FAST_DOUBLE_ELEMENTS:
+ break;
+ case EXTERNAL_PIXEL_ELEMENTS:
+ case EXTERNAL_BYTE_ELEMENTS:
+ case EXTERNAL_UNSIGNED_BYTE_ELEMENTS:
+ case EXTERNAL_SHORT_ELEMENTS:
+ case EXTERNAL_UNSIGNED_SHORT_ELEMENTS:
+ case EXTERNAL_INT_ELEMENTS:
+ case EXTERNAL_UNSIGNED_INT_ELEMENTS:
+ case EXTERNAL_FLOAT_ELEMENTS:
+ case EXTERNAL_DOUBLE_ELEMENTS:
+ // Ignore getters and setters on pixel and external array elements.
+ return GetHeap()->undefined_value();
+ case DICTIONARY_ELEMENTS:
+ if (UpdateGetterSetterInDictionary(element_dictionary(),
+ index,
+ is_getter,
+ fun,
+ attributes)) {
+ return GetHeap()->undefined_value();
+ }
+ break;
+ case NON_STRICT_ARGUMENTS_ELEMENTS: {
+ // Ascertain whether we have read-only properties or an existing
+ // getter/setter pair in an arguments elements dictionary backing
+ // store.
+ FixedArray* parameter_map = FixedArray::cast(elements());
+ uint32_t length = parameter_map->length();
+ Object* probe =
+ index < (length - 2) ? parameter_map->get(index + 2) : NULL;
+ if (probe == NULL || probe->IsTheHole()) {
+ FixedArray* arguments = FixedArray::cast(parameter_map->get(1));
+ if (arguments->IsDictionary()) {
+ SeededNumberDictionary* dictionary =
+ SeededNumberDictionary::cast(arguments);
+ if (UpdateGetterSetterInDictionary(dictionary,
+ index,
+ is_getter,
+ fun,
+ attributes)) {
+ return GetHeap()->undefined_value();
+ }
+ }
+ }
+ break;
+ }
+ }
+
+ AccessorPair* accessors;
+ { MaybeObject* maybe_accessors = GetHeap()->AllocateAccessorPair();
+ if (!maybe_accessors->To(&accessors)) return maybe_accessors;
+ }
+ accessors->set(is_getter, fun);
+
+ { MaybeObject* maybe_ok = SetElementCallback(index, accessors,
attributes);
+ if (maybe_ok->IsFailure()) return maybe_ok;
+ }
+ return GetHeap()->undefined_value();
+}
+
+
+MaybeObject* JSObject::DefinePropertyAccessor(String* name,
+ bool is_getter,
+ Object* fun,
+ PropertyAttributes
attributes) {
+ // Lookup the name.
+ LookupResult result(GetHeap()->isolate());
+ LocalLookupRealNamedProperty(name, &result);
+ if (result.IsFound()) {
+ // TODO(mstarzinger): We should check for result.IsDontDelete() here
once
+ // we only call into the runtime once to set both getter and setter.
+ if (result.type() == CALLBACKS) {
+ Object* obj = result.GetCallbackObject();
+ // Need to preserve old getters/setters.
+ if (obj->IsAccessorPair()) {
+ AccessorPair::cast(obj)->set(is_getter, fun);
+ // Use set to update attributes.
+ { MaybeObject* maybe_ok = SetPropertyCallback(name, obj,
attributes);
+ if (maybe_ok->IsFailure()) return maybe_ok;
+ }
+ return GetHeap()->undefined_value();
}
}
}
AccessorPair* accessors;
- { MaybeObject* maybe_accessors = heap->AllocateAccessorPair();
- if (!maybe_accessors->To<AccessorPair>(&accessors)) return
maybe_accessors;
- }
-
- if (is_element) {
- return SetElementCallback(index, accessors, attributes);
- } else {
- return SetPropertyCallback(name, accessors, attributes);
- }
+ { MaybeObject* maybe_accessors = GetHeap()->AllocateAccessorPair();
+ if (!maybe_accessors->To(&accessors)) return maybe_accessors;
+ }
+ accessors->set(is_getter, fun);
+
+ { MaybeObject* maybe_ok = SetPropertyCallback(name, accessors,
attributes);
+ if (maybe_ok->IsFailure()) return maybe_ok;
+ }
+ return GetHeap()->undefined_value();
}
@@ -4517,19 +4524,15 @@
PropertyDetails details = PropertyDetails(attributes, CALLBACKS);
// Normalize elements to make this operation simple.
- SeededNumberDictionary* dictionary = NULL;
- { Object* result;
- MaybeObject* maybe = NormalizeElements();
- if (!maybe->ToObject(&result)) return maybe;
- dictionary = SeededNumberDictionary::cast(result);
+ SeededNumberDictionary* dictionary;
+ { MaybeObject* maybe_dictionary = NormalizeElements();
+ if (!maybe_dictionary->To(&dictionary)) return maybe_dictionary;
}
ASSERT(HasDictionaryElements() || HasDictionaryArgumentsElements());
// Update the dictionary with the new CALLBACKS property.
- { Object* result;
- MaybeObject* maybe = dictionary->Set(index, structure, details);
- if (!maybe->ToObject(&result)) return maybe;
- dictionary = SeededNumberDictionary::cast(result);
+ { MaybeObject* maybe_dictionary = dictionary->Set(index, structure,
details);
+ if (!maybe_dictionary->To(&dictionary)) return maybe_dictionary;
}
dictionary->set_requires_slow_elements();
@@ -4541,8 +4544,7 @@
// switch to a direct backing store without the parameter map. This
// would allow GC of the context.
FixedArray* parameter_map = FixedArray::cast(elements());
- uint32_t length = parameter_map->length();
- if (index < length - 2) {
+ if (index < static_cast<uint32_t>(parameter_map->length()) - 2) {
parameter_map->set(index + 2, GetHeap()->the_hole_value());
}
parameter_map->set(1, dictionary);
@@ -4550,7 +4552,7 @@
set_elements(dictionary);
}
- return structure;
+ return GetHeap()->undefined_value();
}
@@ -4564,19 +4566,18 @@
< DescriptorArray::kMaxNumberOfDescriptors);
// Normalize object to make this operation simple.
- Object* ok;
{ MaybeObject* maybe_ok = NormalizeProperties(CLEAR_INOBJECT_PROPERTIES,
0);
- if (!maybe_ok->ToObject(&ok)) return maybe_ok;
+ if (maybe_ok->IsFailure()) return maybe_ok;
}
// For the global object allocate a new map to invalidate the global
inline
// caches which have a global property cell reference directly in the
code.
if (IsGlobalObject()) {
- Object* new_map;
+ Map* new_map;
{ MaybeObject* maybe_new_map = map()->CopyDropDescriptors();
- if (!maybe_new_map->ToObject(&new_map)) return maybe_new_map;
- }
- set_map(Map::cast(new_map));
+ if (!maybe_new_map->To(&new_map)) return maybe_new_map;
+ }
+ set_map(new_map);
// When running crankshaft, changing the map is not enough. We
// need to deoptimize all functions that rely on this global
// object.
@@ -4584,17 +4585,15 @@
}
// Update the dictionary with the new CALLBACKS property.
- Object* result;
- { MaybeObject* maybe_result = SetNormalizedProperty(name, structure,
details);
- if (!maybe_result->ToObject(&result)) return maybe_result;
+ { MaybeObject* maybe_ok = SetNormalizedProperty(name, structure,
details);
+ if (maybe_ok->IsFailure()) return maybe_ok;
}
if (convert_back_to_fast) {
- { MaybeObject* maybe_ok = TransformToFastProperties(0);
- if (!maybe_ok->ToObject(&ok)) return maybe_ok;
- }
- }
- return result;
+ MaybeObject* maybe_ok = TransformToFastProperties(0);
+ if (maybe_ok->IsFailure()) return maybe_ok;
+ }
+ return GetHeap()->undefined_value();
}
MaybeObject* JSObject::DefineAccessor(String* name,
@@ -4618,17 +4617,19 @@
fun, attributes);
}
- Object* accessors;
- { MaybeObject* maybe_accessors = DefineGetterSetter(name, attributes);
- if (!maybe_accessors->To<Object>(&accessors)) return maybe_accessors;
- }
- if (accessors->IsUndefined()) return accessors;
- if (is_getter) {
- AccessorPair::cast(accessors)->set_getter(fun);
- } else {
- AccessorPair::cast(accessors)->set_setter(fun);
- }
- return this;
+ // Make sure that the top context does not change when doing callbacks or
+ // interceptor calls.
+ AssertNoContextChange ncc;
+
+ // Try to flatten before operating on the string.
+ name->TryFlatten();
+
+ if (!CanSetCallback(name)) return isolate->heap()->undefined_value();
+
+ uint32_t index = 0;
+ return name->AsArrayIndex(&index) ?
+ DefineElementAccessor(index, is_getter, fun, attributes) :
+ DefinePropertyAccessor(name, is_getter, fun, attributes);
}
@@ -4691,10 +4692,9 @@
break;
}
- Object* ok;
{ MaybeObject* maybe_ok =
SetElementCallback(index, info, info->property_attributes());
- if (!maybe_ok->ToObject(&ok)) return maybe_ok;
+ if (maybe_ok->IsFailure()) return maybe_ok;
}
} else {
// Lookup the name.
@@ -4705,10 +4705,9 @@
if (result.IsProperty() && (result.IsReadOnly() ||
result.IsDontDelete())) {
return isolate->heap()->undefined_value();
}
- Object* ok;
{ MaybeObject* maybe_ok =
SetPropertyCallback(name, info, info->property_attributes());
- if (!maybe_ok->ToObject(&ok)) return maybe_ok;
+ if (maybe_ok->IsFailure()) return maybe_ok;
}
}
@@ -12672,6 +12671,11 @@
details.index());
descriptors->Set(next_descriptor++, &d, witness);
} else if (type == CALLBACKS) {
+ if (value->IsAccessorPair()) {
+ MaybeObject* maybe_copy =
+ AccessorPair::cast(value)->CopyWithoutTransitions();
+ if (!maybe_copy->To(&value)) return maybe_copy;
+ }
CallbacksDescriptor d(String::cast(key),
value,
details.attributes(),
=======================================
--- /branches/bleeding_edge/src/objects.h Mon Feb 20 05:22:02 2012
+++ /branches/bleeding_edge/src/objects.h Wed Feb 22 02:52:57 2012
@@ -2139,10 +2139,16 @@
String* name,
Object* structure,
PropertyAttributes attributes);
- MUST_USE_RESULT MaybeObject* DefineGetterSetter(
+ MUST_USE_RESULT MaybeObject* DefineElementAccessor(
+ uint32_t index,
+ bool is_getter,
+ Object* fun,
+ PropertyAttributes attributes);
+ MUST_USE_RESULT MaybeObject* DefinePropertyAccessor(
String* name,
+ bool is_getter,
+ Object* fun,
PropertyAttributes attributes);
-
void LookupInDescriptor(String* name, LookupResult* result);
// Returns the hidden properties backing store object, currently
@@ -7836,6 +7842,15 @@
static inline AccessorPair* cast(Object* obj);
MUST_USE_RESULT MaybeObject* CopyWithoutTransitions();
+
+ // TODO(svenpanne) Evil temporary helper, will vanish soon...
+ void set(bool modify_getter, Object* value) {
+ if (modify_getter) {
+ set_getter(value);
+ } else {
+ set_setter(value);
+ }
+ }
#ifdef OBJECT_PRINT
void AccessorPairPrint(FILE* out = stdout);
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
