[debug-evaluate] check transitively called builtins

Bug: v8:8558
Change-Id: Ib179947fb1b00f32a574ae3084e3107cc264a851
Reviewed-on: https://chromium-review.googlesource.com/c/1369951
Commit-Queue: Yang Guo <yangguo@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58345}

src/builtins/setup-builtins-internal.cc

@@ -8,6 +8,7 @@
 #include "src/builtins/builtins.h"
 #include "src/code-events.h"
 #include "src/compiler/code-assembler.h"
+
 #include "src/handles-inl.h"
 #include "src/interface-descriptors.h"
 #include "src/interpreter/bytecodes.h"

src/debug/debug-evaluate.cc

@@ -374,43 +374,6 @@ bool IntrinsicHasNoSideEffect(Runtime::FunctionId id) {
 #undef INLINE_INTRINSIC_WHITELIST
 }
 
-#ifdef DEBUG
-bool BuiltinToIntrinsicHasNoSideEffect(Builtins::Name builtin_id,
-                                       Runtime::FunctionId intrinsic_id) {
-  // First check the intrinsic whitelist.
-  if (IntrinsicHasNoSideEffect(intrinsic_id)) return true;
-
-// Whitelist intrinsics called from specific builtins.
-#define BUILTIN_INTRINSIC_WHITELIST(V, W)                            \
-  /* Arrays */                                                       \
-  V(Builtins::kArrayFilter, W(CreateDataProperty))                   \
-  V(Builtins::kArrayMap, W(CreateDataProperty))                      \
-  V(Builtins::kArrayPrototypeSlice,                                  \
-    W(CreateDataProperty) W(SetKeyedProperty) W(SetNamedProperty))   \
-  /* TypedArrays */                                                  \
-  V(Builtins::kTypedArrayConstructor,                                \
-    W(TypedArrayCopyElements) W(ThrowInvalidTypedArrayAlignment))    \
-  V(Builtins::kTypedArrayPrototypeFilter, W(TypedArrayCopyElements)) \
-  V(Builtins::kTypedArrayPrototypeMap, W(SetKeyedProperty) W(SetNamedProperty))
-
-#define CASE(Builtin, ...) \
-  case Builtin:            \
-    return (__VA_ARGS__ false);
-
-#define MATCH(Intrinsic) intrinsic_id == Runtime::k##Intrinsic ||
-
-  switch (builtin_id) {
-    BUILTIN_INTRINSIC_WHITELIST(CASE, MATCH)
-    default:
-      return false;
-  }
-
-#undef MATCH
-#undef CASE
-#undef BUILTIN_INTRINSIC_WHITELIST
-}
-#endif  // DEBUG
-
 bool BytecodeHasNoSideEffect(interpreter::Bytecode bytecode) {
   typedef interpreter::Bytecode Bytecode;
   typedef interpreter::Bytecodes Bytecodes;
@@ -927,33 +890,144 @@ DebugInfo::SideEffectState DebugEvaluate::FunctionGetSideEffectState(
       return DebugInfo::kHasSideEffects;
     DebugInfo::SideEffectState state =
         BuiltinGetSideEffectState(static_cast<Builtins::Name>(builtin_index));
+    return state;
+  }
+
+  return DebugInfo::kHasSideEffects;
+}
+
 #ifdef DEBUG
-    if (state == DebugInfo::kHasNoSideEffect) {
-      // TODO(yangguo): Check builtin-to-builtin calls too.
-      Code code = isolate->builtins()->builtin(builtin_index);
-      int mode = RelocInfo::ModeMask(RelocInfo::EXTERNAL_REFERENCE);
+static bool TransitivelyCalledBuiltinHasNoSideEffect(Builtins::Name caller,
+                                                     Builtins::Name callee) {
+  switch (callee) {
+      // Transitively called Builtins:
+    case Builtins::kAbort:
+    case Builtins::kAbortJS:
+    case Builtins::kAdaptorWithBuiltinExitFrame:
+    case Builtins::kArrayConstructorImpl:
+    case Builtins::kArrayEveryLoopContinuation:
+    case Builtins::kArrayFilterLoopContinuation:
+    case Builtins::kArrayFindIndexLoopContinuation:
+    case Builtins::kArrayFindLoopContinuation:
+    case Builtins::kArrayIncludesHoleyDoubles:
+    case Builtins::kArrayIncludesPackedDoubles:
+    case Builtins::kArrayIncludesSmiOrObject:
+    case Builtins::kArrayIndexOfHoleyDoubles:
+    case Builtins::kArrayIndexOfPackedDoubles:
+    case Builtins::kArrayIndexOfSmiOrObject:
+    case Builtins::kArrayMapLoopContinuation:
+    case Builtins::kArrayReduceLoopContinuation:
+    case Builtins::kArrayReduceRightLoopContinuation:
+    case Builtins::kArraySomeLoopContinuation:
+    case Builtins::kArrayTimSort:
+    case Builtins::kCall_ReceiverIsAny:
+    case Builtins::kCallWithArrayLike:
+    case Builtins::kCEntry_Return1_DontSaveFPRegs_ArgvOnStack_NoBuiltinExit:
+    case Builtins::kCEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit:
+    case Builtins::kCEntry_Return1_DontSaveFPRegs_ArgvInRegister_NoBuiltinExit:
+    case Builtins::kCEntry_Return1_SaveFPRegs_ArgvOnStack_NoBuiltinExit:
+    case Builtins::kCEntry_Return1_SaveFPRegs_ArgvOnStack_BuiltinExit:
+    case Builtins::kCEntry_Return2_DontSaveFPRegs_ArgvOnStack_NoBuiltinExit:
+    case Builtins::kCEntry_Return2_DontSaveFPRegs_ArgvOnStack_BuiltinExit:
+    case Builtins::kCEntry_Return2_DontSaveFPRegs_ArgvInRegister_NoBuiltinExit:
+    case Builtins::kCEntry_Return2_SaveFPRegs_ArgvOnStack_NoBuiltinExit:
+    case Builtins::kCEntry_Return2_SaveFPRegs_ArgvOnStack_BuiltinExit:
+    case Builtins::kCloneFastJSArray:
+    case Builtins::kConstruct:
+    case Builtins::kConvertToLocaleString:
+    case Builtins::kCreateTypedArray:
+    case Builtins::kDirectCEntry:
+    case Builtins::kDoubleToI:
+    case Builtins::kExtractFastJSArray:
+    case Builtins::kFastNewObject:
+    case Builtins::kFindOrderedHashMapEntry:
+    case Builtins::kFlatMapIntoArray:
+    case Builtins::kFlattenIntoArray:
+    case Builtins::kGetProperty:
+    case Builtins::kHasProperty:
+    case Builtins::kMathPowInternal:
+    case Builtins::kNonNumberToNumber:
+    case Builtins::kNonPrimitiveToPrimitive_Number:
+    case Builtins::kNumberToString:
+    case Builtins::kObjectToString:
+    case Builtins::kOrderedHashTableHealIndex:
+    case Builtins::kOrdinaryToPrimitive_Number:
+    case Builtins::kOrdinaryToPrimitive_String:
+    case Builtins::kParseInt:
+    case Builtins::kProxyHasProperty:
+    case Builtins::kRecordWrite:
+    case Builtins::kStringAdd_CheckNone:
+    case Builtins::kStringEqual:
+    case Builtins::kStringIndexOf:
+    case Builtins::kStringRepeat:
+    case Builtins::kToInteger:
+    case Builtins::kToInteger_TruncateMinusZero:
+    case Builtins::kToLength:
+    case Builtins::kToName:
+    case Builtins::kToObject:
+    case Builtins::kToString:
+    case Builtins::kWeakMapLookupHashIndex:
+      return true;
+    case Builtins::kJoinStackPop:
+    case Builtins::kJoinStackPush:
+      switch (caller) {
+        case Builtins::kArrayPrototypeJoin:
+        case Builtins::kArrayPrototypeToLocaleString:
+          return true;
+        default:
+          return false;
+      }
+    case Builtins::kSetProperty:
+      switch (caller) {
+        case Builtins::kArrayPrototypeSlice:
+        case Builtins::kTypedArrayPrototypeMap:
+        case Builtins::kStringPrototypeMatchAll:
+          return true;
+        default:
+          return false;
+      }
+    default:
+      return false;
+  }
+}
+
+// static
+void DebugEvaluate::VerifyTransitiveBuiltins(Isolate* isolate) {
+  // TODO(yangguo): also check runtime calls.
   bool failed = false;
+  bool sanity_check = false;
+  for (int i = 0; i < Builtins::builtin_count; i++) {
+    Builtins::Name caller = static_cast<Builtins::Name>(i);
+    DebugInfo::SideEffectState state = BuiltinGetSideEffectState(caller);
+    if (state != DebugInfo::kHasNoSideEffect) continue;
+    Code code = isolate->builtins()->builtin(caller);
+    int mode = RelocInfo::ModeMask(RelocInfo::CODE_TARGET) |
+               RelocInfo::ModeMask(RelocInfo::RELATIVE_CODE_TARGET);
+
     for (RelocIterator it(code, mode); !it.done(); it.next()) {
       RelocInfo* rinfo = it.rinfo();
-        Address address = rinfo->target_external_reference();
-        const Runtime::Function* function = Runtime::FunctionForEntry(address);
-        if (function == nullptr) continue;
-        if (!BuiltinToIntrinsicHasNoSideEffect(
-                static_cast<Builtins::Name>(builtin_index),
-                function->function_id)) {
-          PrintF("Whitelisted builtin %s calls non-whitelisted intrinsic %s\n",
-                 Builtins::name(builtin_index), function->name);
-          failed = true;
+      DCHECK(RelocInfo::IsCodeTargetMode(rinfo->rmode()));
+      Code callee_code = isolate->heap()->GcSafeFindCodeForInnerPointer(
+          rinfo->target_address());
+      if (!callee_code->is_builtin()) continue;
+      Builtins::Name callee =
+          static_cast<Builtins::Name>(callee_code->builtin_index());
+      if (BuiltinGetSideEffectState(callee) == DebugInfo::kHasNoSideEffect) {
+        continue;
       }
-        DCHECK(!failed);
+      if (TransitivelyCalledBuiltinHasNoSideEffect(caller, callee)) {
+        sanity_check = true;
+        continue;
       }
+      PrintF("Whitelisted builtin %s calls non-whitelisted builtin %s\n",
+             Builtins::name(caller), Builtins::name(callee));
+      failed = true;
     }
-#endif  // DEBUG
-    return state;
   }
-
-  return DebugInfo::kHasSideEffects;
+  CHECK(!failed);
+  CHECK(sanity_check);
 }
+#endif  // DEBUG
 
 // static
 void DebugEvaluate::ApplySideEffectChecks(

src/debug/debug-evaluate.h

@@ -43,6 +43,10 @@ class DebugEvaluate : public AllStatic {
       Isolate* isolate, Handle<SharedFunctionInfo> info);
   static void ApplySideEffectChecks(Handle<BytecodeArray> bytecode_array);
 
+#ifdef DEBUG
+  static void VerifyTransitiveBuiltins(Isolate* isolate);
+#endif  // DEBUG
+
  private:
   // This class builds a context chain for evaluation of expressions
   // in debugger.

src/setup-isolate-full.cc

@@ -5,6 +5,7 @@
 #include "src/setup-isolate.h"
 
 #include "src/base/logging.h"
+#include "src/debug/debug-evaluate.h"
 #include "src/heap/heap-inl.h"
 #include "src/interpreter/interpreter.h"
 #include "src/isolate.h"
@@ -15,6 +16,9 @@ namespace internal {
 void SetupIsolateDelegate::SetupBuiltins(Isolate* isolate) {
   if (create_heap_objects_) {
     SetupBuiltinsInternal(isolate);
+#ifdef DEBUG
+    DebugEvaluate::VerifyTransitiveBuiltins(isolate);
+#endif  // DEBUG
   } else {
     CHECK(isolate->snapshot_available());
   }