Make the runtime entry for setting/changing accessors "atomic".
authorsvenpanne@chromium.org <svenpanne@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Wed, 7 Mar 2012 13:24:44 +0000 (13:24 +0000)
committersvenpanne@chromium.org <svenpanne@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Wed, 7 Mar 2012 13:24:44 +0000 (13:24 +0000)
Previously, there were 1 or 2 calls to the runtime when accessors were changed
or set. This doesn't really work well with property attributes, leading to some
hacks and complicates things even further when trying to share maps in presence
of accessors. Therefore, the runtime entry now takes the full triple (getter,
setter, attributes), where the getter and/or the setter can be null in case they
shouldn't be changed.

For now, we do basically the same on the native side as we did before on the
JavaScript side, but this will change in future CLs, the current CL is already
large enough.

Note that object literals with a getter and a setter for the same property still
do 2 calls, but this is a little bit more tricky to fix and will be handled in a
separate CL.

Review URL: https://chromiumcodereview.appspot.com/9616016

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@10956 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/arm/full-codegen-arm.cc
src/ia32/full-codegen-ia32.cc
src/messages.js
src/mips/full-codegen-mips.cc
src/objects.h
src/regexp.js
src/runtime.cc
src/v8natives.js
src/x64/full-codegen-x64.cc
test/mjsunit/object-define-property.js

index 8639698051041118a6eb108706dde664df16ff80..303f0b086e7ed815e22f34d472e7d77bd6a0d268 100644 (file)
@@ -1498,11 +1498,15 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
         __ ldr(r0, MemOperand(sp));
         __ push(r0);
         VisitForStackValue(key);
-        __ mov(r1, Operand(property->kind() == ObjectLiteral::Property::SETTER ?
-                           Smi::FromInt(1) :
-                           Smi::FromInt(0)));
-        __ push(r1);
-        VisitForStackValue(value);
+        if (property->kind() == ObjectLiteral::Property::GETTER) {
+          VisitForStackValue(value);
+          __ LoadRoot(r1, Heap::kNullValueRootIndex);
+          __ push(r1);
+        } else {
+          __ LoadRoot(r1, Heap::kNullValueRootIndex);
+          __ push(r1);
+          VisitForStackValue(value);
+        }
         __ mov(r0, Operand(Smi::FromInt(NONE)));
         __ push(r0);
         __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5);
index 86ca138ad25495ea867d0fd43a908edec1a4fb2d..b55f4280f172fcefd8e08178630552d29b9f9313 100644 (file)
@@ -1519,10 +1519,13 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
       case ObjectLiteral::Property::GETTER:
         __ push(Operand(esp, 0));  // Duplicate receiver.
         VisitForStackValue(key);
-        __ push(Immediate(property->kind() == ObjectLiteral::Property::SETTER ?
-                          Smi::FromInt(1) :
-                          Smi::FromInt(0)));
-        VisitForStackValue(value);
+        if (property->kind() == ObjectLiteral::Property::GETTER) {
+          VisitForStackValue(value);
+          __ push(Immediate(isolate()->factory()->null_value()));
+        } else {
+          __ push(Immediate(isolate()->factory()->null_value()));
+          VisitForStackValue(value);
+        }
         __ push(Immediate(Smi::FromInt(NONE)));
         __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5);
         break;
index 0afc0372d938e119527cb7f6173073b2d8b2b747..f39d59c28ad742026690bb3f20a83337999cdff2 100644 (file)
@@ -1,4 +1,4 @@
-// Copyright 2011 the V8 project authors. All rights reserved.
+// Copyright 2012 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:
@@ -769,8 +769,7 @@ function DefineOneShotAccessor(obj, name, fun) {
     hasBeenSet = true;
     value = v;
   };
-  %DefineOrRedefineAccessorProperty(obj, name, GETTER, getter, DONT_ENUM);
-  %DefineOrRedefineAccessorProperty(obj, name, SETTER, setter, DONT_ENUM);
+  %DefineOrRedefineAccessorProperty(obj, name, getter, setter, DONT_ENUM);
 }
 
 function CallSite(receiver, fun, pos) {
index 5559788b361e31d86e957951f080797282bc3ffc..e259fc460033025342f66e848d4643112b78837c 100644 (file)
@@ -1500,11 +1500,15 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
         __ lw(a0, MemOperand(sp));
         __ push(a0);
         VisitForStackValue(key);
-        __ li(a1, Operand(property->kind() == ObjectLiteral::Property::SETTER ?
-                           Smi::FromInt(1) :
-                           Smi::FromInt(0)));
-        __ push(a1);
-        VisitForStackValue(value);
+        if (property->kind() == ObjectLiteral::Property::GETTER) {
+          VisitForStackValue(value);
+          __ LoadRoot(a1, Heap::kNullValueRootIndex);
+          __ push(a1);
+        } else {
+          __ LoadRoot(a1, Heap::kNullValueRootIndex);
+          __ push(a1);
+          VisitForStackValue(value);
+        }
         __ li(a0, Operand(Smi::FromInt(NONE)));
         __ push(a0);
         __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5);
index 76958c199b6c20947bc07a0dbb861b737c804386..65cd6fb3fc6b5586a6efad4ade5a10bafb98b509 100644 (file)
@@ -854,6 +854,8 @@ class JSReceiver;
 class Object : public MaybeObject {
  public:
   // Type testing.
+  bool IsObject() { return true; }
+
 #define IS_TYPE_FUNCTION_DECL(type_)  inline bool Is##type_();
   OBJECT_TYPE_LIST(IS_TYPE_FUNCTION_DECL)
   HEAP_OBJECT_TYPE_LIST(IS_TYPE_FUNCTION_DECL)
index b724f68183511701da41555a346c436179cf6c01..ace0be1564572e5b48df5ccf287869f7b310dc3d 100644 (file)
@@ -1,4 +1,4 @@
-// Copyright 2006-2009 the V8 project authors. All rights reserved.
+// Copyright 2012 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:
@@ -421,18 +421,12 @@ function SetUpRegExp() {
     LAST_INPUT(lastMatchInfo) = ToString(string);
   };
 
-  %DefineOrRedefineAccessorProperty($RegExp, 'input', GETTER, RegExpGetInput,
-                                    DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, 'input', SETTER, RegExpSetInput,
-                                    DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, '$_', GETTER, RegExpGetInput,
-                                    DONT_ENUM | DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, '$_', SETTER, RegExpSetInput,
-                                    DONT_ENUM | DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, '$input', GETTER, RegExpGetInput,
-                                    DONT_ENUM | DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, '$input', SETTER, RegExpSetInput,
-                                    DONT_ENUM | DONT_DELETE);
+  %DefineOrRedefineAccessorProperty($RegExp, 'input', RegExpGetInput,
+                                    RegExpSetInput, DONT_DELETE);
+  %DefineOrRedefineAccessorProperty($RegExp, '$_', RegExpGetInput,
+                                    RegExpSetInput, DONT_ENUM | DONT_DELETE);
+  %DefineOrRedefineAccessorProperty($RegExp, '$input', RegExpGetInput,
+                                    RegExpSetInput, DONT_ENUM | DONT_DELETE);
 
   // The properties multiline and $* are aliases for each other.  When this
   // value is set in SpiderMonkey, the value it is set to is coerced to a
@@ -446,13 +440,10 @@ function SetUpRegExp() {
   var RegExpGetMultiline = function() { return multiline; };
   var RegExpSetMultiline = function(flag) { multiline = flag ? true : false; };
 
-  %DefineOrRedefineAccessorProperty($RegExp, 'multiline', GETTER,
-                                    RegExpGetMultiline, DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, 'multiline', SETTER,
+  %DefineOrRedefineAccessorProperty($RegExp, 'multiline', RegExpGetMultiline,
                                     RegExpSetMultiline, DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, '$*', GETTER, RegExpGetMultiline,
-                                    DONT_ENUM | DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, '$*', SETTER, RegExpSetMultiline,
+  %DefineOrRedefineAccessorProperty($RegExp, '$*', RegExpGetMultiline,
+                                    RegExpSetMultiline,
                                     DONT_ENUM | DONT_DELETE);
 
 
@@ -460,44 +451,28 @@ function SetUpRegExp() {
 
 
   // Static properties set by a successful match.
-  %DefineOrRedefineAccessorProperty($RegExp, 'lastMatch', GETTER,
-                                    RegExpGetLastMatch, DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, 'lastMatch', SETTER, NoOpSetter,
+  %DefineOrRedefineAccessorProperty($RegExp, 'lastMatch', RegExpGetLastMatch,
+                                    NoOpSetter, DONT_DELETE);
+  %DefineOrRedefineAccessorProperty($RegExp, '$&', RegExpGetLastMatch,
+                                    NoOpSetter, DONT_ENUM | DONT_DELETE);
+  %DefineOrRedefineAccessorProperty($RegExp, 'lastParen', RegExpGetLastParen,
+                                    NoOpSetter, DONT_DELETE);
+  %DefineOrRedefineAccessorProperty($RegExp, '$+', RegExpGetLastParen,
+                                    NoOpSetter, DONT_ENUM | DONT_DELETE);
+  %DefineOrRedefineAccessorProperty($RegExp, 'leftContext',
+                                    RegExpGetLeftContext, NoOpSetter,
                                     DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, '$&', GETTER, RegExpGetLastMatch,
-                                    DONT_ENUM | DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, '$&', SETTER, NoOpSetter,
-                                    DONT_ENUM | DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, 'lastParen', GETTER,
-                                    RegExpGetLastParen, DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, 'lastParen', SETTER, NoOpSetter,
+  %DefineOrRedefineAccessorProperty($RegExp, '$`', RegExpGetLeftContext,
+                                    NoOpSetter, DONT_ENUM | DONT_DELETE);
+  %DefineOrRedefineAccessorProperty($RegExp, 'rightContext',
+                                    RegExpGetRightContext, NoOpSetter,
                                     DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, '$+', GETTER, RegExpGetLastParen,
-                                    DONT_ENUM | DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, '$+', SETTER, NoOpSetter,
-                                    DONT_ENUM | DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, 'leftContext', GETTER,
-                                    RegExpGetLeftContext, DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, 'leftContext', SETTER, NoOpSetter,
-                                    DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, '$`', GETTER, RegExpGetLeftContext,
-                                    DONT_ENUM | DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, '$`', SETTER, NoOpSetter,
-                                    DONT_ENUM | DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, 'rightContext', GETTER,
-                                    RegExpGetRightContext, DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, 'rightContext', SETTER, NoOpSetter,
-                                    DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, "$'", GETTER,
-                                    RegExpGetRightContext,
-                                    DONT_ENUM | DONT_DELETE);
-  %DefineOrRedefineAccessorProperty($RegExp, "$'", SETTER, NoOpSetter,
-                                    DONT_ENUM | DONT_DELETE);
+  %DefineOrRedefineAccessorProperty($RegExp, "$'", RegExpGetRightContext,
+                                    NoOpSetter, DONT_ENUM | DONT_DELETE);
 
   for (var i = 1; i < 10; ++i) {
-    %DefineOrRedefineAccessorProperty($RegExp, '$' + i, GETTER,
-                                      RegExpMakeCaptureGetter(i), DONT_DELETE);
-    %DefineOrRedefineAccessorProperty($RegExp, '$' + i, SETTER, NoOpSetter,
+    %DefineOrRedefineAccessorProperty($RegExp, '$' + i,
+                                      RegExpMakeCaptureGetter(i), NoOpSetter,
                                       DONT_DELETE);
   }
 }
index a96152d8d1fdf5c10c9facbcac103535d29ed22f..9209b9e169cee922799358dba69f9686d65a787a 100644 (file)
@@ -995,23 +995,14 @@ enum PropertyDescriptorIndices {
   DESCRIPTOR_SIZE
 };
 
-// Returns an array with the property description:
-//  if args[1] is not a property on args[0]
-//          returns undefined
-//  if args[1] is a data property on args[0]
-//         [false, value, Writeable, Enumerable, Configurable]
-//  if args[1] is an accessor on args[0]
-//         [true, GetFunction, SetFunction, Enumerable, Configurable]
-RUNTIME_FUNCTION(MaybeObject*, Runtime_GetOwnProperty) {
-  ASSERT(args.length() == 2);
+
+static MaybeObject* GetOwnProperty(Isolate* isolate,
+                                   Handle<JSObject> obj,
+                                   Handle<String> name) {
   Heap* heap = isolate->heap();
-  HandleScope scope(isolate);
   Handle<FixedArray> elms = isolate->factory()->NewFixedArray(DESCRIPTOR_SIZE);
   Handle<JSArray> desc = isolate->factory()->NewJSArrayWithElements(elms);
   LookupResult result(isolate);
-  CONVERT_ARG_HANDLE_CHECKED(JSObject, obj, 0);
-  CONVERT_ARG_HANDLE_CHECKED(String, name, 1);
-
   // This could be an element.
   uint32_t index;
   if (name->AsArrayIndex(&index)) {
@@ -1145,6 +1136,22 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_GetOwnProperty) {
 }
 
 
+// Returns an array with the property description:
+//  if args[1] is not a property on args[0]
+//          returns undefined
+//  if args[1] is a data property on args[0]
+//         [false, value, Writeable, Enumerable, Configurable]
+//  if args[1] is an accessor on args[0]
+//         [true, GetFunction, SetFunction, Enumerable, Configurable]
+RUNTIME_FUNCTION(MaybeObject*, Runtime_GetOwnProperty) {
+  ASSERT(args.length() == 2);
+  HandleScope scope(isolate);
+  CONVERT_ARG_HANDLE_CHECKED(JSObject, obj, 0);
+  CONVERT_ARG_HANDLE_CHECKED(String, name, 1);
+  return GetOwnProperty(isolate, obj, name);
+}
+
+
 RUNTIME_FUNCTION(MaybeObject*, Runtime_PreventExtensions) {
   ASSERT(args.length() == 1);
   CONVERT_ARG_CHECKED(JSObject, obj, 0);
@@ -4307,6 +4314,12 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_KeyedGetProperty) {
                                     args.at<Object>(1));
 }
 
+
+static bool IsValidAccessor(Handle<Object> obj) {
+  return obj->IsUndefined() || obj->IsSpecFunction() || obj->IsNull();
+}
+
+
 // Implements part of 8.12.9 DefineOwnProperty.
 // There are 3 cases that lead here:
 // Step 4b - define a new accessor property.
@@ -4317,18 +4330,37 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_DefineOrRedefineAccessorProperty) {
   ASSERT(args.length() == 5);
   HandleScope scope(isolate);
   CONVERT_ARG_HANDLE_CHECKED(JSObject, obj, 0);
-  CONVERT_ARG_CHECKED(String, name, 1);
-  CONVERT_SMI_ARG_CHECKED(flag, 2);
-  Object* fun = args[3];
+  RUNTIME_ASSERT(!obj->IsNull());
+  CONVERT_ARG_HANDLE_CHECKED(String, name, 1);
+  CONVERT_ARG_HANDLE_CHECKED(Object, getter, 2);
+  RUNTIME_ASSERT(IsValidAccessor(getter));
+  CONVERT_ARG_HANDLE_CHECKED(Object, setter, 3);
+  RUNTIME_ASSERT(IsValidAccessor(setter));
   CONVERT_SMI_ARG_CHECKED(unchecked, 4);
-
   RUNTIME_ASSERT((unchecked & ~(READ_ONLY | DONT_ENUM | DONT_DELETE)) == 0);
   PropertyAttributes attr = static_cast<PropertyAttributes>(unchecked);
 
-  RUNTIME_ASSERT(!obj->IsNull());
-  RUNTIME_ASSERT(fun->IsSpecFunction() || fun->IsUndefined());
-  AccessorComponent component = flag == 0 ? ACCESSOR_GETTER : ACCESSOR_SETTER;
-  return obj->DefineAccessor(name, component, fun, attr);
+  // TODO(svenpanne) Define getter/setter/attributes in a single step.
+  if (getter->IsNull() && setter->IsNull()) {
+    JSArray* array;
+    { MaybeObject* maybe_array = GetOwnProperty(isolate, obj, name);
+      if (!maybe_array->To(&array)) return maybe_array;
+    }
+    Object* current = FixedArray::cast(array->elements())->get(GETTER_INDEX);
+    getter = Handle<Object>(current, isolate);
+  }
+  if (!getter->IsNull()) {
+    MaybeObject* ok =
+        obj->DefineAccessor(*name, ACCESSOR_GETTER, *getter, attr);
+    if (ok->IsFailure()) return ok;
+  }
+  if (!setter->IsNull()) {
+    MaybeObject* ok =
+        obj->DefineAccessor(*name, ACCESSOR_SETTER, *setter, attr);
+    if (ok->IsFailure()) return ok;
+  }
+
+  return isolate->heap()->undefined_value();
 }
 
 // Implements part of 8.12.9 DefineOwnProperty.
@@ -4342,9 +4374,8 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_DefineOrRedefineDataProperty) {
   HandleScope scope(isolate);
   CONVERT_ARG_HANDLE_CHECKED(JSObject, js_object, 0);
   CONVERT_ARG_HANDLE_CHECKED(String, name, 1);
-  Handle<Object> obj_value = args.at<Object>(2);
+  CONVERT_ARG_HANDLE_CHECKED(Object, obj_value, 2);
   CONVERT_SMI_ARG_CHECKED(unchecked, 3);
-
   RUNTIME_ASSERT((unchecked & ~(READ_ONLY | DONT_ENUM | DONT_DELETE)) == 0);
   PropertyAttributes attr = static_cast<PropertyAttributes>(unchecked);
 
index 8906f9eca209c86127f4aee3bd43acb93429d571..26724a24199d3384e224573a6bc50138071d4d75 100644 (file)
@@ -1,4 +1,4 @@
-// Copyright 2011 the V8 project authors. All rights reserved.
+// Copyright 2012 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:
@@ -834,10 +834,6 @@ function DefineObjectProperty(obj, p, desc, should_throw) {
     }
 
     %DefineOrRedefineDataProperty(obj, p, value, flag);
-  } else if (IsGenericDescriptor(desc)) {
-    // Step 12 - updating an existing accessor property with generic
-    //           descriptor. Changing flags only.
-    %DefineOrRedefineAccessorProperty(obj, p, GETTER, current.getGet(), flag);
   } else {
     // There are 3 cases that lead here:
     // Step 4b - defining a new accessor property.
@@ -845,12 +841,9 @@ function DefineObjectProperty(obj, p, desc, should_throw) {
     //                 property.
     // Step 12 - updating an existing accessor property with an accessor
     //           descriptor.
-    if (desc.hasGetter()) {
-      %DefineOrRedefineAccessorProperty(obj, p, GETTER, desc.getGet(), flag);
-    }
-    if (desc.hasSetter()) {
-      %DefineOrRedefineAccessorProperty(obj, p, SETTER, desc.getSet(), flag);
-    }
+    var getter = desc.hasGetter() ? desc.getGet() : null;
+    var setter = desc.hasSetter() ? desc.getSet() : null;
+    %DefineOrRedefineAccessorProperty(obj, p, getter, setter, flag);
   }
   return true;
 }
index 7a60adc0b37db09bf368fc6ee61556f461d016a6..49adf6af660454adabaf994c0c18bf946e974be7 100644 (file)
@@ -1459,10 +1459,13 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) {
       case ObjectLiteral::Property::GETTER:
         __ push(Operand(rsp, 0));  // Duplicate receiver.
         VisitForStackValue(key);
-        __ Push(property->kind() == ObjectLiteral::Property::SETTER ?
-                Smi::FromInt(1) :
-                Smi::FromInt(0));
-        VisitForStackValue(value);
+        if (property->kind() == ObjectLiteral::Property::GETTER) {
+          VisitForStackValue(value);
+          __ PushRoot(Heap::kNullValueRootIndex);
+        } else {
+          __ PushRoot(Heap::kNullValueRootIndex);
+          VisitForStackValue(value);
+        }
         __ Push(Smi::FromInt(NONE));
         __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5);
         break;
index 9384a357bae1da251e9b44c0123bf037ff40ea4a..fdaf82d105e4de50c2a0708c6b6e95ae053178ad 100644 (file)
@@ -1,4 +1,4 @@
-// Copyright 2010 the V8 project authors. All rights reserved.
+// Copyright 2012 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:
@@ -503,7 +503,7 @@ try {
 // Defining properties null should fail even when we have
 // other allowed values
 try {
-  %DefineOrRedefineAccessorProperty(null, 'foo', 0, func, 0);
+  %DefineOrRedefineAccessorProperty(null, 'foo', func, null, 0);
 } catch (e) {
   assertTrue(/illegal access/.test(e));
 }