From fb2768bdf7aea355f4e33a9430e293d3b5a21347 Mon Sep 17 00:00:00 2001
From: Benedikt Meurer <bmeurer@chromium.org>
Date: Mon, 30 Nov 2020 16:17:59 +0100
Subject: [PATCH] [debug] Mark side-effect free builtins and intrinsics as
 such.

While working on C++ debug evaluate, we found that several builtins and
intrinsics aren't marked as side effect free, although they are clearly
side effect free, and that breaks the C++ side effect free evaluation.

- %DefineClass() and %TypedArray%.of(), and
- various WebAssembly getters ("buffer", "exports" and "length") as
  well as the C++ functions for the debug proxy.

Also-By: pfaffe@chromium.org
Bug: chromium:1137514
Change-Id: Iebd333dc2014f1ad218908f64c9199c157dc08b5
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2565135
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71498}
---
 src/debug/debug-evaluate.cc                   |  2 +
 src/wasm/wasm-js.cc                           | 39 ++++++++++++-------
 .../debug-evaluate-no-side-effect-builtins.js |  2 +-
 3 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/src/debug/debug-evaluate.cc b/src/debug/debug-evaluate.cc
index 912bc4bf28..6bf3a220c3 100644
--- a/src/debug/debug-evaluate.cc
+++ b/src/debug/debug-evaluate.cc
@@ -346,6 +346,7 @@ bool IntrinsicHasNoSideEffect(Runtime::FunctionId id) {
   V(CreateObjectLiteral)                      \
   V(CreateObjectLiteralWithoutAllocationSite) \
   V(CreateRegExpLiteral)                      \
+  V(DefineClass)                              \
   /* Called from builtins */                  \
   V(AllocateInYoungGeneration)                \
   V(AllocateInOldGeneration)                  \
@@ -593,6 +594,7 @@ DebugInfo::SideEffectState BuiltinGetSideEffectState(Builtins::Name id) {
     case Builtins::kTrace:
     // TypedArray builtins.
     case Builtins::kTypedArrayConstructor:
+    case Builtins::kTypedArrayOf:
     case Builtins::kTypedArrayPrototypeAt:
     case Builtins::kTypedArrayPrototypeBuffer:
     case Builtins::kTypedArrayPrototypeByteLength:
diff --git a/src/wasm/wasm-js.cc b/src/wasm/wasm-js.cc
index eddc580b2d..5167499775 100644
--- a/src/wasm/wasm-js.cc
+++ b/src/wasm/wasm-js.cc
@@ -1992,9 +1992,11 @@ void WebAssemblyGlobalType(const v8::FunctionCallbackInfo<v8::Value>& args) {
 // TODO(titzer): we use the API to create the function template because the
 // internal guts are too ugly to replicate here.
 static i::Handle<i::FunctionTemplateInfo> NewFunctionTemplate(
-    i::Isolate* i_isolate, FunctionCallback func, bool has_prototype) {
+    i::Isolate* i_isolate, FunctionCallback func, bool has_prototype,
+    SideEffectType side_effect_type = SideEffectType::kHasSideEffect) {
   Isolate* isolate = reinterpret_cast<Isolate*>(i_isolate);
-  Local<FunctionTemplate> templ = FunctionTemplate::New(isolate, func);
+  Local<FunctionTemplate> templ = FunctionTemplate::New(
+      isolate, func, {}, {}, 0, ConstructorBehavior::kAllow, side_effect_type);
   has_prototype ? templ->ReadOnlyPrototype() : templ->RemovePrototype();
   return v8::Utils::OpenHandle(*templ);
 }
@@ -2008,22 +2010,26 @@ static i::Handle<i::ObjectTemplateInfo> NewObjectTemplate(
 
 namespace internal {
 
-Handle<JSFunction> CreateFunc(Isolate* isolate, Handle<String> name,
-                              FunctionCallback func, bool has_prototype) {
+Handle<JSFunction> CreateFunc(
+    Isolate* isolate, Handle<String> name, FunctionCallback func,
+    bool has_prototype,
+    SideEffectType side_effect_type = SideEffectType::kHasSideEffect) {
   Handle<FunctionTemplateInfo> temp =
-      NewFunctionTemplate(isolate, func, has_prototype);
+      NewFunctionTemplate(isolate, func, has_prototype, side_effect_type);
   Handle<JSFunction> function =
       ApiNatives::InstantiateFunction(temp, name).ToHandleChecked();
   DCHECK(function->shared().HasSharedName());
   return function;
 }
 
-Handle<JSFunction> InstallFunc(Isolate* isolate, Handle<JSObject> object,
-                               const char* str, FunctionCallback func,
-                               int length, bool has_prototype = false,
-                               PropertyAttributes attributes = NONE) {
+Handle<JSFunction> InstallFunc(
+    Isolate* isolate, Handle<JSObject> object, const char* str,
+    FunctionCallback func, int length, bool has_prototype = false,
+    PropertyAttributes attributes = NONE,
+    SideEffectType side_effect_type = SideEffectType::kHasSideEffect) {
   Handle<String> name = v8_str(isolate, str);
-  Handle<JSFunction> function = CreateFunc(isolate, name, func, has_prototype);
+  Handle<JSFunction> function =
+      CreateFunc(isolate, name, func, has_prototype, side_effect_type);
   function->shared().set_length(length);
   JSObject::AddProperty(isolate, object, name, function, attributes);
   return function;
@@ -2045,7 +2051,8 @@ void InstallGetter(Isolate* isolate, Handle<JSObject> object, const char* str,
                    FunctionCallback func) {
   Handle<String> name = v8_str(isolate, str);
   Handle<JSFunction> function =
-      CreateFunc(isolate, GetterName(isolate, name), func, false);
+      CreateFunc(isolate, GetterName(isolate, name), func, false,
+                 SideEffectType::kHasNoSideEffect);
 
   Utils::ToLocal(object)->SetAccessorProperty(Utils::ToLocal(name),
                                               Utils::ToLocal(function),
@@ -2942,8 +2949,10 @@ Handle<JSProxy> GetJSProxy(
   Handle<BigInt> callee_fp = BigInt::FromInt64(isolate, frame->callee_fp());
   JSObject::AddProperty(isolate, handler, "callee_fp", callee_fp, DONT_ENUM);
 
-  InstallFunc(isolate, handler, "get", get_callback, 3, false, READ_ONLY);
-  InstallFunc(isolate, handler, "has", has_callback, 2, false, READ_ONLY);
+  InstallFunc(isolate, handler, "get", get_callback, 3, false, READ_ONLY,
+              SideEffectType::kHasNoSideEffect);
+  InstallFunc(isolate, handler, "has", has_callback, 2, false, READ_ONLY,
+              SideEffectType::kHasNoSideEffect);
 
   return factory->NewJSProxy(target, handler);
 }
@@ -3007,9 +3016,9 @@ Handle<JSProxy> WasmJs::GetJSDebugProxy(WasmFrame* frame) {
   // The top level proxy delegates lookups to the index space proxies.
   Handle<JSObject> handler = factory->NewJSObjectWithNullProto();
   InstallFunc(isolate, handler, "get", ToplevelGetTrapCallback, 3, false,
-              READ_ONLY);
+              READ_ONLY, SideEffectType::kHasNoSideEffect);
   InstallFunc(isolate, handler, "has", ToplevelHasTrapCallback, 2, false,
-              READ_ONLY);
+              READ_ONLY, SideEffectType::kHasNoSideEffect);
 
   Handle<JSObject> target = factory->NewJSObjectWithNullProto();
 
diff --git a/test/debugger/debug/side-effect/debug-evaluate-no-side-effect-builtins.js b/test/debugger/debug/side-effect/debug-evaluate-no-side-effect-builtins.js
index c515c5d589..29c13e71d4 100644
--- a/test/debugger/debug/side-effect/debug-evaluate-no-side-effect-builtins.js
+++ b/test/debugger/debug/side-effect/debug-evaluate-no-side-effect-builtins.js
@@ -121,7 +121,7 @@ function listener(event, exec_state, event_data, data) {
     success(true, `!!typed_array.buffer`);
     success(0, `typed_array.byteOffset`);
     success(3, `typed_array.byteLength`);
-    fail(`Uint8Array.of(1, 2)`);
+    success({0: 1, 1: 2}, `Uint8Array.of(1, 2)`);
     function_param = [
       "forEach", "every", "some", "reduce", "reduceRight", "find", "filter",
       "map", "findIndex"