From 3813cbf210be615334e9d1b0ca6900fc4f163092 Mon Sep 17 00:00:00 2001 From: Erik Luo Date: Thu, 15 Mar 2018 09:56:17 -0700 Subject: [PATCH] [debug] use flag to decide whether interceptor has side effect Adds a flag onto InterceptorInfo to mark an interceptor's getter, query, and enumerator callbacks as side-effect-free. Bug: v8:7515 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: Iafc5d2fa554d6d9a38604e179ea5b884c3b77af0 Reviewed-on: https://chromium-review.googlesource.com/957870 Commit-Queue: Erik Luo Reviewed-by: Yang Guo Cr-Commit-Position: refs/heads/master@{#51969} --- src/api-arguments-inl.h | 80 +++++++++++++--------------- src/api-arguments.h | 2 +- src/debug/debug-evaluate.cc | 6 +++ src/objects-inl.h | 1 + src/objects.h | 2 + test/cctest/test-api-interceptors.cc | 49 +++++++++++++++++ 6 files changed, 96 insertions(+), 44 deletions(-) diff --git a/src/api-arguments-inl.h b/src/api-arguments-inl.h index 0a47d37894..71e4491530 100644 --- a/src/api-arguments-inl.h +++ b/src/api-arguments-inl.h @@ -13,9 +13,9 @@ namespace v8 { namespace internal { -#define FOR_EACH_CALLBACK(F) \ - F(Query, query, Object, v8::Integer) \ - F(Deleter, deleter, Object, v8::Boolean) +#define FOR_EACH_CALLBACK(F) \ + F(Query, query, Object, v8::Integer, interceptor) \ + F(Deleter, deleter, Object, v8::Boolean, Handle()) #define DCHECK_NAME_COMPATIBLE(interceptor, name) \ DCHECK(interceptor->is_named()); \ @@ -32,44 +32,44 @@ namespace internal { ExternalCallbackScope call_scope(ISOLATE, FUNCTION_ADDR(F)); \ PropertyCallbackInfo callback_info(begin()); -#define CREATE_NAMED_CALLBACK(Function, type, ReturnType, ApiReturnType) \ - Handle PropertyCallbackArguments::CallNamed##Function( \ +#define CREATE_NAMED_CALLBACK(FUNCTION, TYPE, RETURN_TYPE, API_RETURN_TYPE, \ + INFO_FOR_SIDE_EFFECT) \ + Handle PropertyCallbackArguments::CallNamed##FUNCTION( \ Handle interceptor, Handle name) { \ DCHECK_NAME_COMPATIBLE(interceptor, name); \ Isolate* isolate = this->isolate(); \ RuntimeCallTimerScope timer( \ - isolate, RuntimeCallCounterId::kNamed##Function##Callback); \ - GenericNamedProperty##Function##Callback f = \ - ToCData( \ - interceptor->type()); \ - Handle side_effect_check_not_supported; \ - PREPARE_CALLBACK_INFO(isolate, f, Handle, ApiReturnType, \ - side_effect_check_not_supported); \ + isolate, RuntimeCallCounterId::kNamed##FUNCTION##Callback); \ + GenericNamedProperty##FUNCTION##Callback f = \ + ToCData( \ + interceptor->TYPE()); \ + PREPARE_CALLBACK_INFO(isolate, f, Handle, API_RETURN_TYPE, \ + INFO_FOR_SIDE_EFFECT); \ LOG(isolate, \ - ApiNamedPropertyAccess("interceptor-named-" #type, holder(), *name)); \ + ApiNamedPropertyAccess("interceptor-named-" #TYPE, holder(), *name)); \ f(v8::Utils::ToLocal(name), callback_info); \ - return GetReturnValue(isolate); \ + return GetReturnValue(isolate); \ } FOR_EACH_CALLBACK(CREATE_NAMED_CALLBACK) #undef CREATE_NAMED_CALLBACK -#define CREATE_INDEXED_CALLBACK(Function, type, ReturnType, ApiReturnType) \ - Handle PropertyCallbackArguments::CallIndexed##Function( \ - Handle interceptor, uint32_t index) { \ - DCHECK(!interceptor->is_named()); \ - Isolate* isolate = this->isolate(); \ - RuntimeCallTimerScope timer( \ - isolate, RuntimeCallCounterId::kIndexed##Function##Callback); \ - IndexedProperty##Function##Callback f = \ - ToCData(interceptor->type()); \ - Handle side_effect_check_not_supported; \ - PREPARE_CALLBACK_INFO(isolate, f, Handle, ApiReturnType, \ - side_effect_check_not_supported); \ - LOG(isolate, ApiIndexedPropertyAccess("interceptor-indexed-" #type, \ - holder(), index)); \ - f(index, callback_info); \ - return GetReturnValue(isolate); \ +#define CREATE_INDEXED_CALLBACK(FUNCTION, TYPE, RETURN_TYPE, API_RETURN_TYPE, \ + INFO_FOR_SIDE_EFFECT) \ + Handle PropertyCallbackArguments::CallIndexed##FUNCTION( \ + Handle interceptor, uint32_t index) { \ + DCHECK(!interceptor->is_named()); \ + Isolate* isolate = this->isolate(); \ + RuntimeCallTimerScope timer( \ + isolate, RuntimeCallCounterId::kIndexed##FUNCTION##Callback); \ + IndexedProperty##FUNCTION##Callback f = \ + ToCData(interceptor->TYPE()); \ + PREPARE_CALLBACK_INFO(isolate, f, Handle, API_RETURN_TYPE, \ + INFO_FOR_SIDE_EFFECT); \ + LOG(isolate, ApiIndexedPropertyAccess("interceptor-indexed-" #TYPE, \ + holder(), index)); \ + f(index, callback_info); \ + return GetReturnValue(isolate); \ } FOR_EACH_CALLBACK(CREATE_INDEXED_CALLBACK) @@ -87,8 +87,7 @@ Handle PropertyCallbackArguments::CallNamedGetter( ApiNamedPropertyAccess("interceptor-named-getter", holder(), *name)); GenericNamedPropertyGetterCallback f = ToCData(interceptor->getter()); - Handle side_effect_check_not_supported; - return BasicCallNamedGetterCallback(f, name, side_effect_check_not_supported); + return BasicCallNamedGetterCallback(f, name, interceptor); } Handle PropertyCallbackArguments::CallNamedDescriptor( @@ -102,8 +101,7 @@ Handle PropertyCallbackArguments::CallNamedDescriptor( GenericNamedPropertyDescriptorCallback f = ToCData( interceptor->descriptor()); - Handle side_effect_check_not_supported; - return BasicCallNamedGetterCallback(f, name, side_effect_check_not_supported); + return BasicCallNamedGetterCallback(f, name, interceptor); } Handle PropertyCallbackArguments::BasicCallNamedGetterCallback( @@ -204,7 +202,7 @@ Handle PropertyCallbackArguments::CallIndexedGetter( ApiIndexedPropertyAccess("interceptor-indexed-getter", holder(), index)); IndexedPropertyGetterCallback f = ToCData(interceptor->getter()); - return BasicCallIndexedGetterCallback(f, index); + return BasicCallIndexedGetterCallback(f, index, interceptor); } Handle PropertyCallbackArguments::CallIndexedDescriptor( @@ -217,15 +215,13 @@ Handle PropertyCallbackArguments::CallIndexedDescriptor( holder(), index)); IndexedPropertyDescriptorCallback f = ToCData(interceptor->descriptor()); - return BasicCallIndexedGetterCallback(f, index); + return BasicCallIndexedGetterCallback(f, index, interceptor); } Handle PropertyCallbackArguments::BasicCallIndexedGetterCallback( - IndexedPropertyGetterCallback f, uint32_t index) { + IndexedPropertyGetterCallback f, uint32_t index, Handle info) { Isolate* isolate = this->isolate(); - Handle side_effect_check_not_supported; - PREPARE_CALLBACK_INFO(isolate, f, Handle, v8::Value, - side_effect_check_not_supported); + PREPARE_CALLBACK_INFO(isolate, f, Handle, v8::Value, info); f(index, callback_info); return GetReturnValue(isolate); } @@ -237,9 +233,7 @@ Handle PropertyCallbackArguments::CallPropertyEnumerator( v8::ToCData(interceptor->enumerator()); // TODO(cbruni): assert same type for indexed and named callback. Isolate* isolate = this->isolate(); - Handle side_effect_check_not_supported; - PREPARE_CALLBACK_INFO(isolate, f, Handle, v8::Array, - side_effect_check_not_supported); + PREPARE_CALLBACK_INFO(isolate, f, Handle, v8::Array, interceptor); f(callback_info); return GetReturnValue(isolate); } diff --git a/src/api-arguments.h b/src/api-arguments.h index eee331289e..d69849bf5c 100644 --- a/src/api-arguments.h +++ b/src/api-arguments.h @@ -158,7 +158,7 @@ class PropertyCallbackArguments Handle interceptor); inline Handle BasicCallIndexedGetterCallback( - IndexedPropertyGetterCallback f, uint32_t index); + IndexedPropertyGetterCallback f, uint32_t index, Handle info); inline Handle BasicCallNamedGetterCallback( GenericNamedPropertyGetterCallback f, Handle name, Handle info); diff --git a/src/debug/debug-evaluate.cc b/src/debug/debug-evaluate.cc index 81ab254206..3ea9aa1a65 100644 --- a/src/debug/debug-evaluate.cc +++ b/src/debug/debug-evaluate.cc @@ -937,6 +937,12 @@ bool DebugEvaluate::CallbackHasNoSideEffect(Object* callback_info) { info->name()->ShortPrint(); PrintF("' may cause side effect.\n"); } + } else if (callback_info->IsInterceptorInfo()) { + InterceptorInfo* info = InterceptorInfo::cast(callback_info); + if (info->has_no_side_effect()) return true; + if (FLAG_trace_side_effect_free_debug_evaluate) { + PrintF("[debug-evaluate] API Interceptor may cause side effect.\n"); + } } return false; } diff --git a/src/objects-inl.h b/src/objects-inl.h index 922ab907c7..05a6d6c8ed 100644 --- a/src/objects-inl.h +++ b/src/objects-inl.h @@ -2401,6 +2401,7 @@ BOOL_ACCESSORS(InterceptorInfo, flags, can_intercept_symbols, BOOL_ACCESSORS(InterceptorInfo, flags, all_can_read, kAllCanReadBit) BOOL_ACCESSORS(InterceptorInfo, flags, non_masking, kNonMasking) BOOL_ACCESSORS(InterceptorInfo, flags, is_named, kNamed) +BOOL_ACCESSORS(InterceptorInfo, flags, has_no_side_effect, kHasNoSideEffect) ACCESSORS(CallHandlerInfo, callback, Object, kCallbackOffset) ACCESSORS(CallHandlerInfo, js_callback, Object, kJsCallbackOffset) diff --git a/src/objects.h b/src/objects.h index 3183259ac4..09191bdd23 100644 --- a/src/objects.h +++ b/src/objects.h @@ -4557,6 +4557,7 @@ class InterceptorInfo: public Struct { DECL_BOOLEAN_ACCESSORS(all_can_read) DECL_BOOLEAN_ACCESSORS(non_masking) DECL_BOOLEAN_ACCESSORS(is_named) + DECL_BOOLEAN_ACCESSORS(has_no_side_effect) inline int flags() const; inline void set_flags(int flags); @@ -4582,6 +4583,7 @@ class InterceptorInfo: public Struct { static const int kAllCanReadBit = 1; static const int kNonMasking = 2; static const int kNamed = 3; + static const int kHasNoSideEffect = 4; private: DISALLOW_IMPLICIT_CONSTRUCTORS(InterceptorInfo); diff --git a/test/cctest/test-api-interceptors.cc b/test/cctest/test-api-interceptors.cc index fd811abffe..8822e938b9 100644 --- a/test/cctest/test-api-interceptors.cc +++ b/test/cctest/test-api-interceptors.cc @@ -64,6 +64,14 @@ void EmptyInterceptorGetter(Local name, void EmptyInterceptorSetter(Local name, Local value, const v8::PropertyCallbackInfo& info) {} +void EmptyInterceptorQuery(Local name, + const v8::PropertyCallbackInfo& info) {} + +void EmptyInterceptorDeleter( + Local name, const v8::PropertyCallbackInfo& info) {} + +void EmptyInterceptorEnumerator( + const v8::PropertyCallbackInfo& info) {} void SimpleAccessorGetter(Local name, const v8::PropertyCallbackInfo& info) { @@ -2587,6 +2595,47 @@ static void InterceptorForHiddenProperties( interceptor_for_hidden_properties_called = true; } +THREADED_TEST(NoSideEffectPropertyHandler) { + v8::Isolate* isolate = CcTest::isolate(); + v8::HandleScope scope(isolate); + Local templ = ObjectTemplate::New(isolate); + templ->SetHandler(v8::NamedPropertyHandlerConfiguration( + EmptyInterceptorGetter, EmptyInterceptorSetter, EmptyInterceptorQuery, + EmptyInterceptorDeleter, EmptyInterceptorEnumerator)); + + LocalContext context; + v8::Local object = + templ->NewInstance(context.local()).ToLocalChecked(); + context->Global()->Set(context.local(), v8_str("obj"), object).FromJust(); + + CHECK(v8::debug::EvaluateGlobal(isolate, v8_str("obj.x"), true).IsEmpty()); + CHECK( + v8::debug::EvaluateGlobal(isolate, v8_str("obj.x = 1"), true).IsEmpty()); + CHECK( + v8::debug::EvaluateGlobal(isolate, v8_str("'x' in obj"), true).IsEmpty()); + CHECK(v8::debug::EvaluateGlobal(isolate, v8_str("delete obj.x"), true) + .IsEmpty()); + // Wrap the variable declaration since declaring globals is a side effect. + CHECK(v8::debug::EvaluateGlobal( + isolate, v8_str("(function() { for (var p in obj) ; })()"), true) + .IsEmpty()); + + // TODO(luoe): turn on has_no_side_effect flag from API once it is exposed. + i::Handle internal_object = + i::Handle::cast(v8::Utils::OpenHandle(*object)); + internal_object->GetNamedInterceptor()->set_has_no_side_effect(true); + + v8::debug::EvaluateGlobal(isolate, v8_str("obj.x"), true).ToLocalChecked(); + CHECK( + v8::debug::EvaluateGlobal(isolate, v8_str("obj.x = 1"), true).IsEmpty()); + v8::debug::EvaluateGlobal(isolate, v8_str("'x' in obj"), true) + .ToLocalChecked(); + CHECK(v8::debug::EvaluateGlobal(isolate, v8_str("delete obj.x"), true) + .IsEmpty()); + v8::debug::EvaluateGlobal( + isolate, v8_str("(function() { for (var p in obj) ; })()"), true) + .ToLocalChecked(); +} THREADED_TEST(HiddenPropertiesWithInterceptors) { LocalContext context;