[debug] use flag to decide whether accessor has side effect.

Instead of a hard-coded list of function addresses, we now use a flag
on the AccessorInfo object to annotate whether the getter can cause any
side effect.

Future changes will extend this to InterceptorInfo, CallHandlerInfo, and
expose this through the API.

R=jgruber@chromium.org, luoe@chromium.org

Bug: v8:7515
Change-Id: Id0fedf03493c3bd81913557a5681f8f63660f6a4
Reviewed-on: https://chromium-review.googlesource.com/945909
Commit-Queue: Yang Guo <yangguo@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51756}

src/accessors.cc

@@ -28,6 +28,7 @@ Handle<AccessorInfo> Accessors::MakeAccessor(
   info->set_is_special_data_property(true);
   info->set_is_sloppy(false);
   info->set_replace_on_access(false);
+  info->set_has_no_side_effect(false);
   name = factory->InternalizeName(name);
   info->set_name(*name);
   Handle<Object> get = v8::FromCData(isolate, getter);

src/accessors.h

@@ -49,6 +49,14 @@ class JavaScriptFrame;
   V(script_source_mapping_url, ScriptSourceMappingUrl)              \
   V(string_length, StringLength)
 
+#define SIDE_EFFECT_FREE_ACCESSOR_INFO_LIST(V) \
+  V(ArrayLength)                               \
+  V(BoundFunctionLength)                       \
+  V(BoundFunctionName)                         \
+  V(FunctionName)                              \
+  V(FunctionLength)                            \
+  V(StringLength)
+
 #define ACCESSOR_SETTER_LIST(V) \
   V(ArrayLengthSetter)          \
   V(ErrorStackSetter)           \

src/api-arguments-inl.h

@@ -22,9 +22,10 @@ namespace internal {
   DCHECK(!name->IsPrivate());                     \
   DCHECK_IMPLIES(name->IsSymbol(), interceptor->can_intercept_symbols());
 
-#define PREPARE_CALLBACK_INFO(ISOLATE, F, RETURN_VALUE, API_RETURN_TYPE) \
+#define PREPARE_CALLBACK_INFO(ISOLATE, F, RETURN_VALUE, API_RETURN_TYPE, \
+                              CALLBACK_INFO)                             \
   if (ISOLATE->needs_side_effect_check() &&                              \
-      !PerformSideEffectCheck(ISOLATE, FUNCTION_ADDR(F))) {              \
+      !PerformSideEffectCheck(ISOLATE, CALLBACK_INFO)) {                 \
     return RETURN_VALUE();                                               \
   }                                                                      \
   VMState<EXTERNAL> state(ISOLATE);                                      \
@@ -41,7 +42,9 @@ namespace internal {
     GenericNamedProperty##Function##Callback f =                              \
         ToCData<GenericNamedProperty##Function##Callback>(                    \
             interceptor->type());                                             \
-    PREPARE_CALLBACK_INFO(isolate, f, Handle<ReturnType>, ApiReturnType);     \
+    Handle<Object> side_effect_check_not_supported;                           \
+    PREPARE_CALLBACK_INFO(isolate, f, Handle<ReturnType>, ApiReturnType,      \
+                          side_effect_check_not_supported);                   \
     LOG(isolate,                                                              \
         ApiNamedPropertyAccess("interceptor-named-" #type, holder(), *name)); \
     f(v8::Utils::ToLocal(name), callback_info);                               \
@@ -60,7 +63,9 @@ FOR_EACH_CALLBACK(CREATE_NAMED_CALLBACK)
         isolate, RuntimeCallCounterId::kIndexed##Function##Callback);      \
     IndexedProperty##Function##Callback f =                                \
         ToCData<IndexedProperty##Function##Callback>(interceptor->type()); \
-    PREPARE_CALLBACK_INFO(isolate, f, Handle<ReturnType>, ApiReturnType);  \
+    Handle<Object> side_effect_check_not_supported;                        \
+    PREPARE_CALLBACK_INFO(isolate, f, Handle<ReturnType>, ApiReturnType,   \
+                          side_effect_check_not_supported);                \
     LOG(isolate, ApiIndexedPropertyAccess("interceptor-indexed-" #type,    \
                                           holder(), index));               \
     f(index, callback_info);                                               \
@@ -82,7 +87,8 @@ Handle<Object> PropertyCallbackArguments::CallNamedGetter(
       ApiNamedPropertyAccess("interceptor-named-getter", holder(), *name));
   GenericNamedPropertyGetterCallback f =
       ToCData<GenericNamedPropertyGetterCallback>(interceptor->getter());
-  return BasicCallNamedGetterCallback(f, name);
+  Handle<Object> side_effect_check_not_supported;
+  return BasicCallNamedGetterCallback(f, name, side_effect_check_not_supported);
 }
 
 Handle<Object> PropertyCallbackArguments::CallNamedDescriptor(
@@ -96,14 +102,16 @@ Handle<Object> PropertyCallbackArguments::CallNamedDescriptor(
   GenericNamedPropertyDescriptorCallback f =
       ToCData<GenericNamedPropertyDescriptorCallback>(
           interceptor->descriptor());
-  return BasicCallNamedGetterCallback(f, name);
+  Handle<Object> side_effect_check_not_supported;
+  return BasicCallNamedGetterCallback(f, name, side_effect_check_not_supported);
 }
 
 Handle<Object> PropertyCallbackArguments::BasicCallNamedGetterCallback(
-    GenericNamedPropertyGetterCallback f, Handle<Name> name) {
+    GenericNamedPropertyGetterCallback f, Handle<Name> name,
+    Handle<Object> info) {
   DCHECK(!name->IsPrivate());
   Isolate* isolate = this->isolate();
-  PREPARE_CALLBACK_INFO(isolate, f, Handle<Object>, v8::Value);
+  PREPARE_CALLBACK_INFO(isolate, f, Handle<Object>, v8::Value, info);
   f(v8::Utils::ToLocal(name), callback_info);
   return GetReturnValue<Object>(isolate);
 }
@@ -117,7 +125,10 @@ Handle<Object> PropertyCallbackArguments::CallNamedSetter(
   Isolate* isolate = this->isolate();
   RuntimeCallTimerScope timer(isolate,
                               RuntimeCallCounterId::kNamedSetterCallback);
-  PREPARE_CALLBACK_INFO(isolate, f, Handle<Object>, v8::Value);
+  DCHECK(!isolate->needs_side_effect_check());
+  Handle<Object> side_effect_check_not_supported;
+  PREPARE_CALLBACK_INFO(isolate, f, Handle<Object>, v8::Value,
+                        side_effect_check_not_supported);
   LOG(isolate,
       ApiNamedPropertyAccess("interceptor-named-set", holder(), *name));
   f(v8::Utils::ToLocal(name), v8::Utils::ToLocal(value), callback_info);
@@ -133,7 +144,11 @@ Handle<Object> PropertyCallbackArguments::CallNamedDefiner(
                               RuntimeCallCounterId::kNamedDefinerCallback);
   GenericNamedPropertyDefinerCallback f =
       ToCData<GenericNamedPropertyDefinerCallback>(interceptor->definer());
-  PREPARE_CALLBACK_INFO(isolate, f, Handle<Object>, v8::Value);
+  // We should not have come this far when side effect checks are enabled.
+  DCHECK(!isolate->needs_side_effect_check());
+  Handle<Object> side_effect_check_not_supported;
+  PREPARE_CALLBACK_INFO(isolate, f, Handle<Object>, v8::Value,
+                        side_effect_check_not_supported);
   LOG(isolate,
       ApiNamedPropertyAccess("interceptor-named-define", holder(), *name));
   f(v8::Utils::ToLocal(name), desc, callback_info);
@@ -148,7 +163,11 @@ Handle<Object> PropertyCallbackArguments::CallIndexedSetter(
                               RuntimeCallCounterId::kIndexedSetterCallback);
   IndexedPropertySetterCallback f =
       ToCData<IndexedPropertySetterCallback>(interceptor->setter());
-  PREPARE_CALLBACK_INFO(isolate, f, Handle<Object>, v8::Value);
+  // We should not have come this far when side effect checks are enabled.
+  DCHECK(!isolate->needs_side_effect_check());
+  Handle<Object> side_effect_check_not_supported;
+  PREPARE_CALLBACK_INFO(isolate, f, Handle<Object>, v8::Value,
+                        side_effect_check_not_supported);
   LOG(isolate,
       ApiIndexedPropertyAccess("interceptor-indexed-set", holder(), index));
   f(index, v8::Utils::ToLocal(value), callback_info);
@@ -164,7 +183,11 @@ Handle<Object> PropertyCallbackArguments::CallIndexedDefiner(
                               RuntimeCallCounterId::kIndexedDefinerCallback);
   IndexedPropertyDefinerCallback f =
       ToCData<IndexedPropertyDefinerCallback>(interceptor->definer());
-  PREPARE_CALLBACK_INFO(isolate, f, Handle<Object>, v8::Value);
+  // We should not have come this far when side effect checks are enabled.
+  DCHECK(!isolate->needs_side_effect_check());
+  Handle<Object> side_effect_check_not_supported;
+  PREPARE_CALLBACK_INFO(isolate, f, Handle<Object>, v8::Value,
+                        side_effect_check_not_supported);
   LOG(isolate,
       ApiIndexedPropertyAccess("interceptor-indexed-define", holder(), index));
   f(index, desc, callback_info);
@@ -200,7 +223,9 @@ Handle<Object> PropertyCallbackArguments::CallIndexedDescriptor(
 Handle<Object> PropertyCallbackArguments::BasicCallIndexedGetterCallback(
     IndexedPropertyGetterCallback f, uint32_t index) {
   Isolate* isolate = this->isolate();
-  PREPARE_CALLBACK_INFO(isolate, f, Handle<Object>, v8::Value);
+  Handle<Object> side_effect_check_not_supported;
+  PREPARE_CALLBACK_INFO(isolate, f, Handle<Object>, v8::Value,
+                        side_effect_check_not_supported);
   f(index, callback_info);
   return GetReturnValue<Object>(isolate);
 }
@@ -212,7 +237,9 @@ Handle<JSObject> PropertyCallbackArguments::CallPropertyEnumerator(
       v8::ToCData<IndexedPropertyEnumeratorCallback>(interceptor->enumerator());
   // TODO(cbruni): assert same type for indexed and named callback.
   Isolate* isolate = this->isolate();
-  PREPARE_CALLBACK_INFO(isolate, f, Handle<JSObject>, v8::Array);
+  Handle<Object> side_effect_check_not_supported;
+  PREPARE_CALLBACK_INFO(isolate, f, Handle<JSObject>, v8::Array,
+                        side_effect_check_not_supported);
   f(callback_info);
   return GetReturnValue<JSObject>(isolate);
 }
@@ -228,7 +255,7 @@ Handle<Object> PropertyCallbackArguments::CallAccessorGetter(
   LOG(isolate, ApiNamedPropertyAccess("accessor-getter", holder(), *name));
   AccessorNameGetterCallback f =
       ToCData<AccessorNameGetterCallback>(info->getter());
-  return BasicCallNamedGetterCallback(f, name);
+  return BasicCallNamedGetterCallback(f, name, info);
 }
 
 Handle<Object> PropertyCallbackArguments::CallAccessorSetter(
@@ -239,7 +266,11 @@ Handle<Object> PropertyCallbackArguments::CallAccessorSetter(
                               RuntimeCallCounterId::kAccessorSetterCallback);
   AccessorNameSetterCallback f =
       ToCData<AccessorNameSetterCallback>(accessor_info->setter());
-  PREPARE_CALLBACK_INFO(isolate, f, Handle<Object>, void);
+  // We should not have come this far when side effect checks are enabled.
+  DCHECK(!isolate->needs_side_effect_check());
+  Handle<Object> side_effect_check_not_supported;
+  PREPARE_CALLBACK_INFO(isolate, f, Handle<Object>, void,
+                        side_effect_check_not_supported);
   LOG(isolate, ApiNamedPropertyAccess("accessor-setter", holder(), *name));
   f(v8::Utils::ToLocal(name), v8::Utils::ToLocal(value), callback_info);
   return GetReturnValue<Object>(isolate);

src/api-arguments.cc

@@ -20,7 +20,7 @@ Handle<Object> FunctionCallbackArguments::Call(CallHandlerInfo* handler) {
   v8::FunctionCallback f =
       v8::ToCData<v8::FunctionCallback>(handler->callback());
   if (isolate->needs_side_effect_check() &&
-      !isolate->debug()->PerformSideEffectCheckForCallback(FUNCTION_ADDR(f))) {
+      !isolate->debug()->PerformSideEffectCheckForCallback(handler)) {
     return Handle<Object>();
   }
   VMState<EXTERNAL> state(isolate);
@@ -48,9 +48,9 @@ Handle<JSObject> PropertyCallbackArguments::CallIndexedEnumerator(
   return CallPropertyEnumerator(interceptor);
 }
 
-bool PropertyCallbackArguments::PerformSideEffectCheck(Isolate* isolate,
-                                                       Address function) {
-  return isolate->debug()->PerformSideEffectCheckForCallback(function);
+bool PropertyCallbackArguments::PerformSideEffectCheck(
+    Isolate* isolate, Handle<Object> callback_info) {
+  return isolate->debug()->PerformSideEffectCheckForCallback(*callback_info);
 }
 
 }  // namespace internal

src/api-arguments.h

@@ -160,13 +160,14 @@ class PropertyCallbackArguments
   inline Handle<Object> BasicCallIndexedGetterCallback(
       IndexedPropertyGetterCallback f, uint32_t index);
   inline Handle<Object> BasicCallNamedGetterCallback(
-      GenericNamedPropertyGetterCallback f, Handle<Name> name);
+      GenericNamedPropertyGetterCallback f, Handle<Name> name,
+      Handle<Object> info);
 
   inline JSObject* holder() {
     return JSObject::cast(this->begin()[T::kHolderIndex]);
   }
 
-  bool PerformSideEffectCheck(Isolate* isolate, Address function);
+  bool PerformSideEffectCheck(Isolate* isolate, Handle<Object> callback_info);
 
   // Don't copy PropertyCallbackArguments, because they would both have the
   // same prev_ pointer.

src/debug/debug-evaluate.cc

@@ -800,16 +800,6 @@ bool BuiltinHasNoSideEffect(Builtins::Name id) {
   }
 }
 
-static const Address accessors_with_no_side_effect[] = {
-    // Whitelist for accessors.
-    FUNCTION_ADDR(Accessors::StringLengthGetter),
-    FUNCTION_ADDR(Accessors::ArrayLengthGetter),
-    FUNCTION_ADDR(Accessors::FunctionLengthGetter),
-    FUNCTION_ADDR(Accessors::FunctionNameGetter),
-    FUNCTION_ADDR(Accessors::BoundFunctionLengthGetter),
-    FUNCTION_ADDR(Accessors::BoundFunctionNameGetter),
-};
-
 }  // anonymous namespace
 
 // static
@@ -890,14 +880,17 @@ bool DebugEvaluate::FunctionHasNoSideEffect(Handle<SharedFunctionInfo> info) {
 }
 
 // static
-bool DebugEvaluate::CallbackHasNoSideEffect(Address function_addr) {
-  for (size_t i = 0; i < arraysize(accessors_with_no_side_effect); i++) {
-    if (function_addr == accessors_with_no_side_effect[i]) return true;
-  }
-
-  if (FLAG_trace_side_effect_free_debug_evaluate) {
-    PrintF("[debug-evaluate] API Callback at %p may cause side effect.\n",
-           reinterpret_cast<void*>(function_addr));
+bool DebugEvaluate::CallbackHasNoSideEffect(Object* callback_info) {
+  DisallowHeapAllocation no_gc;
+  if (callback_info->IsAccessorInfo()) {
+    // List of whitelisted internal accessors can be found in accessors.h.
+    AccessorInfo* info = AccessorInfo::cast(callback_info);
+    if (info->has_no_side_effect()) return true;
+    if (FLAG_trace_side_effect_free_debug_evaluate) {
+      PrintF("[debug-evaluate] API Callback '");
+      info->name()->ShortPrint();
+      PrintF("' may cause side effect.\n");
+    }
   }
   return false;
 }

src/debug/debug-evaluate.h

@@ -32,7 +32,7 @@ class DebugEvaluate : public AllStatic {
                                    bool throw_on_side_effect);
 
   static bool FunctionHasNoSideEffect(Handle<SharedFunctionInfo> info);
-  static bool CallbackHasNoSideEffect(Address function_addr);
+  static bool CallbackHasNoSideEffect(Object* callback_info);
 
  private:
   // This class builds a context chain for evaluation of expressions

src/debug/debug.cc

@@ -2194,9 +2194,9 @@ bool Debug::PerformSideEffectCheck(Handle<JSFunction> function) {
   return true;
 }
 
-bool Debug::PerformSideEffectCheckForCallback(Address function) {
+bool Debug::PerformSideEffectCheckForCallback(Object* callback_info) {
   DCHECK(isolate_->needs_side_effect_check());
-  if (DebugEvaluate::CallbackHasNoSideEffect(function)) return true;
+  if (DebugEvaluate::CallbackHasNoSideEffect(callback_info)) return true;
   side_effect_check_failed_ = true;
   // Throw an uncatchable termination exception.
   isolate_->TerminateExecution();

src/debug/debug.h

@@ -337,7 +337,7 @@ class Debug {
   }
 
   bool PerformSideEffectCheck(Handle<JSFunction> function);
-  bool PerformSideEffectCheckForCallback(Address function);
+  bool PerformSideEffectCheckForCallback(Object* callback_info);
 
   // Flags and states.
   DebugScope* debugger_entry() {

src/heap/setup-heap-internal.cc

@@ -695,6 +695,12 @@ void Heap::CreateInternalAccessorInfoObjects() {
   roots_[k##AccessorName##AccessorRootIndex] = *acessor_info;
   ACCESSOR_INFO_LIST(INIT_ACCESSOR_INFO)
 #undef INIT_ACCESSOR_INFO
+
+#define INIT_SIDE_EFFECT_FLAG(AccessorName)                      \
+  AccessorInfo::cast(roots_[k##AccessorName##AccessorRootIndex]) \
+      ->set_has_no_side_effect(true);
+  SIDE_EFFECT_FREE_ACCESSOR_INFO_LIST(INIT_SIDE_EFFECT_FLAG)
+#undef INIT_SIDE_EFFECT_FLAG
 }
 
 }  // namespace internal

src/objects-inl.h

@@ -3156,6 +3156,8 @@ BIT_FIELD_ACCESSORS(AccessorInfo, flags, is_special_data_property,
 BIT_FIELD_ACCESSORS(AccessorInfo, flags, replace_on_access,
                     AccessorInfo::ReplaceOnAccessBit)
 BIT_FIELD_ACCESSORS(AccessorInfo, flags, is_sloppy, AccessorInfo::IsSloppyBit)
+BIT_FIELD_ACCESSORS(AccessorInfo, flags, has_no_side_effect,
+                    AccessorInfo::HasNoSideEffectBit)
 BIT_FIELD_ACCESSORS(AccessorInfo, flags, initial_property_attributes,
                     AccessorInfo::InitialAttributesBits)
 

src/objects.h

@@ -4399,6 +4399,7 @@ class AccessorInfo: public Struct {
   DECL_BOOLEAN_ACCESSORS(is_special_data_property)
   DECL_BOOLEAN_ACCESSORS(replace_on_access)
   DECL_BOOLEAN_ACCESSORS(is_sloppy)
+  DECL_BOOLEAN_ACCESSORS(has_no_side_effect)
 
   // The property attributes used when an API object template is instantiated
   // for the first time. Changing of this value afterwards does not affect
@@ -4447,6 +4448,7 @@ class AccessorInfo: public Struct {
   V(IsSpecialDataPropertyBit, bool, 1, _)    \
   V(IsSloppyBit, bool, 1, _)                 \
   V(ReplaceOnAccessBit, bool, 1, _)          \
+  V(HasNoSideEffectBit, bool, 1, _)          \
   V(InitialAttributesBits, PropertyAttributes, 3, _)
 
   DEFINE_BIT_FIELDS(ACCESSOR_INFO_FLAGS_BIT_FIELDS)

test/debugger/debug/side-effect/debug-evaluate-no-side-effect.js

@@ -82,6 +82,11 @@ function listener(event, exec_state, event_data, data) {
     fail("try { set_a() } catch (e) {}");
     // Test that call to set accessor fails.
     fail("array.length = 4");
+    fail("'x'.length = 1");
+    fail("set_a.name = 'set_b'");
+    fail("set_a.length = 1");
+    fail("bound.name = 'bound'");
+    fail("bound.length = 1");
     // Test that call to non-whitelisted get accessor fails.
     fail("error.stack");
     // Eval is not allowed.