[compiler] Force all calls to JS builtins to be lowered

Previously only Builtins declared TFJ or CPP in builtins-definitions.h
were converted to direct calls in ReduceJSCall. This allows all
builtins with JS linkage to be converted. To facilitate this, it adds
Builtins::HasJSLinkage(id) that returns true for any builtins with
JSTrampolineDescriptor as their call descriptor.

It also ensures that any JS functions installed by the bootstrapper are
also required to have JS linkage to catch early errors.

Change-Id: I2fddca41f9ab1c7c9633aa0ab4847a5c108e2bb2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1883549Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Commit-Queue: Dan Elphick <delphick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64698}

src/builtins/builtins-definitions.h

@@ -97,8 +97,8 @@ namespace internal {
   ASM(JSEntry, Dummy)                                                          \
   ASM(JSConstructEntry, Dummy)                                                 \
   ASM(JSRunMicrotasksEntry, RunMicrotasksEntry)                                \
-  ASM(JSEntryTrampoline, Dummy)                                                \
-  ASM(JSConstructEntryTrampoline, Dummy)                                       \
+  ASM(JSEntryTrampoline, JSTrampoline)                                         \
+  ASM(JSConstructEntryTrampoline, JSTrampoline)                                \
   ASM(ResumeGeneratorTrampoline, ResumeGenerator)                              \
                                                                                \
   /* String helpers */                                                         \
@@ -116,7 +116,7 @@ namespace internal {
   TFS(OrderedHashTableHealIndex, kTable, kIndex)                               \
                                                                                \
   /* Interpreter */                                                            \
-  ASM(InterpreterEntryTrampoline, Dummy)                                       \
+  ASM(InterpreterEntryTrampoline, JSTrampoline)                                \
   ASM(InterpreterPushArgsThenCall, InterpreterPushArgsThenCall)                \
   ASM(InterpreterPushUndefinedAndArgsThenCall, InterpreterPushArgsThenCall)    \
   ASM(InterpreterPushArgsThenCallWithFinalSpread, InterpreterPushArgsThenCall) \
@@ -522,12 +522,12 @@ namespace internal {
                                                                                \
   /* Function */                                                               \
   CPP(FunctionConstructor)                                                     \
-  ASM(FunctionPrototypeApply, Dummy)                                           \
+  ASM(FunctionPrototypeApply, JSTrampoline)                                    \
   CPP(FunctionPrototypeBind)                                                   \
   /* ES6 #sec-function.prototype.bind */                                       \
   TFJ(FastFunctionPrototypeBind,                                               \
       SharedFunctionInfo::kDontAdaptArgumentsSentinel)                         \
-  ASM(FunctionPrototypeCall, Dummy)                                            \
+  ASM(FunctionPrototypeCall, JSTrampoline)                                     \
   /* ES6 #sec-function.prototype-@@hasinstance */                              \
   TFJ(FunctionPrototypeHasInstance, 1, kReceiver, kV)                          \
   /* ES6 #sec-function.prototype.tostring */                                   \
@@ -796,8 +796,8 @@ namespace internal {
   TFJ(PromiseAllSettledRejectElementClosure, 1, kReceiver, kValue)             \
                                                                                \
   /* Reflect */                                                                \
-  ASM(ReflectApply, Dummy)                                                     \
-  ASM(ReflectConstruct, Dummy)                                                 \
+  ASM(ReflectApply, JSTrampoline)                                              \
+  ASM(ReflectConstruct, JSTrampoline)                                          \
   CPP(ReflectDefineProperty)                                                   \
   CPP(ReflectGetOwnPropertyDescriptor)                                         \
   CPP(ReflectOwnKeys)                                                          \

src/builtins/builtins.cc

@@ -188,6 +188,13 @@ Callable Builtins::CallableFor(Isolate* isolate, Name name) {
   return Callable{code, CallInterfaceDescriptorFor(name)};
 }
 
+// static
+bool Builtins::HasJSLinkage(int builtin_index) {
+  Name name = static_cast<Name>(builtin_index);
+  DCHECK_NE(BCH, Builtins::KindOf(name));
+  return CallInterfaceDescriptorFor(name) == JSTrampolineDescriptor{};
+}
+
 // static
 const char* Builtins::name(int index) {
   DCHECK(IsBuiltinId(index));

src/builtins/builtins.h

@@ -95,6 +95,7 @@ class Builtins {
 
   static CallInterfaceDescriptor CallInterfaceDescriptorFor(Name name);
   V8_EXPORT_PRIVATE static Callable CallableFor(Isolate* isolate, Name name);
+  static bool HasJSLinkage(int index);
 
   static int GetStackParameterCount(Name name);
 

src/codegen/interface-descriptors.h

@@ -292,6 +292,10 @@ class V8_EXPORT_PRIVATE CallInterfaceDescriptor {
 
   const char* DebugName() const;
 
+  bool operator==(const CallInterfaceDescriptor& other) const {
+    return data() == other.data();
+  }
+
  protected:
   const CallInterfaceDescriptorData* data() const { return data_; }
 

src/compiler/js-heap-broker.cc

@@ -2440,7 +2440,6 @@ void JSHeapBroker::InitializeRefsMap() {
         Builtins::kCallFunction_ReceiverIsNotNullOrUndefined,
         Builtins::kCallFunction_ReceiverIsNullOrUndefined,
         Builtins::kCloneFastJSArray,
-        Builtins::kCompileLazy,
         Builtins::kConstructFunctionForwardVarargs,
         Builtins::kForInFilter,
         Builtins::kGetProperty,
@@ -2457,8 +2456,8 @@ void JSHeapBroker::InitializeRefsMap() {
       GetOrCreateData(b->builtin_handle(id));
     }
   }
-  for (int32_t id = 0; id < Builtins::builtin_count; ++id) {
-    if (Builtins::KindOf(id) == Builtins::TFJ) {
+  for (int32_t id = 0; id < Builtins::kFirstBytecodeHandler; ++id) {
+    if (Builtins::HasJSLinkage(id)) {
       GetOrCreateData(b->builtin_handle(id));
     }
   }

src/compiler/js-typed-lowering.cc

@@ -1759,8 +1759,8 @@ Reduction JSTypedLowering::ReduceJSCall(Node* node) {
     } else if (shared.HasBuiltinId() && Builtins::IsCpp(shared.builtin_id())) {
       // Patch {node} to a direct CEntry call.
       ReduceBuiltin(jsgraph(), node, shared.builtin_id(), arity, flags);
-    } else if (shared.HasBuiltinId() &&
-               Builtins::KindOf(shared.builtin_id()) == Builtins::TFJ) {
+    } else if (shared.HasBuiltinId()) {
+      DCHECK(Builtins::HasJSLinkage(shared.builtin_id()));
       // Patch {node} to a direct code object call.
       Callable callable = Builtins::CallableFor(
           isolate(), static_cast<Builtins::Name>(shared.builtin_id()));

src/init/bootstrapper.cc

@@ -382,6 +382,8 @@ V8_NOINLINE Handle<JSFunction> CreateFunction(
     Isolate* isolate, Handle<String> name, InstanceType type, int instance_size,
     int inobject_properties, Handle<HeapObject> prototype,
     Builtins::Name builtin_id) {
+  DCHECK(Builtins::HasJSLinkage(builtin_id));
+
   Handle<JSFunction> result;
 
   NewFunctionArgs args = NewFunctionArgs::ForBuiltinWithPrototype(
@@ -412,6 +414,7 @@ V8_NOINLINE Handle<JSFunction> InstallFunction(
     Isolate* isolate, Handle<JSObject> target, Handle<String> name,
     InstanceType type, int instance_size, int inobject_properties,
     Handle<HeapObject> prototype, Builtins::Name call) {
+  DCHECK(Builtins::HasJSLinkage(call));
   Handle<JSFunction> function = CreateFunction(
       isolate, name, type, instance_size, inobject_properties, prototype, call);
   JSObject::AddProperty(isolate, target, name, function, DONT_ENUM);
@@ -431,6 +434,7 @@ V8_NOINLINE Handle<JSFunction> SimpleCreateFunction(Isolate* isolate,
                                                     Handle<String> name,
                                                     Builtins::Name call,
                                                     int len, bool adapt) {
+  DCHECK(Builtins::HasJSLinkage(call));
   NewFunctionArgs args = NewFunctionArgs::ForBuiltinWithoutPrototype(
       name, call, LanguageMode::kStrict);
   Handle<JSFunction> fun = isolate->factory()->NewFunction(args);

test/mjsunit/cross-realm-builtin-call.js

+// Copyright 2019 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// Flags: --allow-natives-syntax
+
+function check(x) { assertEquals(x, "foo"); }
+
+var r = Realm.createAllowCrossRealmAccess();
+var f = Realm.eval(r, `
+    function f(func) {
+      // The call to Function.prototype.apply is across native contexts so
+      // cannot be elided. However the compiler should be able to call the
+      // builtin directly rather than via the trampoline Code object. This isn't
+      // easy to test, but here we at least check that it doesn't crash due to
+      // calling a builtin Code object incorrectly (Function.Prototype.apply).
+      return func.apply(undefined, ["foo"]);
+    }
+
+    f;`);
+
+%PrepareFunctionForOptimization(f);
+
+f(check);
+f(check);
+
+%OptimizeFunctionOnNextCall(f);
+
+f(check);