[builtins] Add IsOffHeapSafe predicate and test

Off-heap-safety slightly differs from isolate-independence in that it
allows external references and checks instruction-size constraints.

This adds the new predicate as well as a cctest verifying it. New
DCHECKs are introduced to document assumptions and upcoming work.

Note that this breaks the --stress-off-heap-code flag. Fixes will
follow in upcoming CLs.

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

src/builtins/builtins.h

@@ -118,6 +118,17 @@ class Builtins {
   // TODO(jgruber,v8:6666): Remove once all builtins have been migrated.
   static bool IsIsolateIndependent(int index);
 
+  // This is the condition we currently use to determine whether a builtin is
+  // copied off-heap when --stress-off-heap-code is passed. Such builtins do not
+  // need to be isolate-independent, e.g. they can contain external references
+  // that point to one specific isolate. A further restrictions is that there
+  // must be enough space for the trampoline.
+  static bool IsOffHeapSafe(int index);
+
+  // The off-heap trampoline is short but requires a certain minimal instruction
+  // size. This function states whether a given builtin is too short.
+  static bool IsTooShortForOffHeapTrampoline(int index);
+
   bool is_initialized() const { return initialized_; }
 
   // Used by SetupIsolateDelegate and Deserializer.

src/builtins/x64/builtins-x64.cc

@@ -1204,6 +1204,8 @@ void Builtins::Generate_InterpreterPushArgsThenConstructImpl(
 static void Generate_InterpreterEnterBytecode(MacroAssembler* masm) {
   // Set the return address to the correct point in the interpreter entry
   // trampoline.
+  // TODO(jgruber,v8:6666): Update logic once builtin is off-heap-safe.
+  DCHECK(!Builtins::IsOffHeapSafe(Builtins::kInterpreterEntryTrampoline));
   Smi* interpreter_entry_return_pc_offset(
       masm->isolate()->heap()->interpreter_entry_return_pc_offset());
   DCHECK_NE(interpreter_entry_return_pc_offset, Smi::kZero);

src/deoptimizer.cc

@@ -957,6 +957,8 @@ void Deoptimizer::DoComputeInterpretedFrame(TranslatedFrame* translated_frame,
       (!is_topmost || (bailout_type_ == LAZY)) && !goto_catch_handler
           ? builtins->builtin(Builtins::kInterpreterEnterBytecodeAdvance)
           : builtins->builtin(Builtins::kInterpreterEnterBytecodeDispatch);
+  // TODO(jgruber,v8:6666): Update logic once builtin is off-heap-safe.
+  DCHECK(!Builtins::IsOffHeapSafe(dispatch_builtin->builtin_index()));
   output_frame->SetPc(reinterpret_cast<intptr_t>(dispatch_builtin->entry()));
 
   // Update constant pool.
@@ -1135,6 +1137,11 @@ void Deoptimizer::DoComputeConstructStubFrame(TranslatedFrame* translated_frame,
   CHECK(!is_topmost || bailout_type_ == LAZY);
   int input_index = 0;
 
+  // TODO(jgruber,v8:6666): Update logic once builtin is off-heap-safe.
+  DCHECK(!Builtins::IsOffHeapSafe(
+      Builtins::kJSConstructStubGenericRestrictedReturn));
+  DCHECK(!Builtins::IsOffHeapSafe(
+      Builtins::kJSConstructStubGenericUnrestrictedReturn));
   Builtins* builtins = isolate_->builtins();
   Code* construct_stub = builtins->builtin(
       FLAG_harmony_restrict_constructor_return
@@ -1688,6 +1695,8 @@ void Deoptimizer::DoComputeBuiltinContinuation(
                        Builtins::kContinueToCodeStubBuiltinWithResult)
                  : isolate()->builtins()->builtin(
                        Builtins::kContinueToCodeStubBuiltin));
+  // TODO(jgruber,v8:6666): Update logic once builtin is off-heap-safe.
+  DCHECK(!Builtins::IsOffHeapSafe(continue_to_builtin->builtin_index()));
   output_frame->SetPc(
       reinterpret_cast<intptr_t>(continue_to_builtin->instruction_start()));
 

src/frames.cc

@@ -170,6 +170,10 @@ bool StackTraceFrameIterator::IsValidFrame(StackFrame* frame) const {
 namespace {
 
 bool IsInterpreterFramePc(Isolate* isolate, Address pc) {
+  // TODO(jgruber,v8:6666): Update logic once builtin is off-heap-safe.
+  DCHECK(!Builtins::IsOffHeapSafe(Builtins::kInterpreterEntryTrampoline));
+  DCHECK(!Builtins::IsOffHeapSafe(Builtins::kInterpreterEnterBytecodeAdvance));
+  DCHECK(!Builtins::IsOffHeapSafe(Builtins::kInterpreterEnterBytecodeDispatch));
   Code* interpreter_entry_trampoline =
       isolate->builtins()->builtin(Builtins::kInterpreterEntryTrampoline);
   Code* interpreter_bytecode_advance =

src/isolate.cc

@@ -2862,7 +2862,7 @@ bool BuiltinAliasesOffHeapTrampolineRegister(Isolate* isolate,
 
 void ChangeToOffHeapTrampoline(Isolate* isolate, Handle<Code> code,
                                InstructionStream* stream) {
-  DCHECK(Builtins::IsIsolateIndependent(code->builtin_index()));
+  DCHECK(Builtins::IsOffHeapSafe(code->builtin_index()));
   HandleScope scope(isolate);
 
   constexpr size_t buffer_size = 256;  // Enough to fit the single jmp.
@@ -2888,6 +2888,9 @@ void ChangeToOffHeapTrampoline(Isolate* isolate, Handle<Code> code,
   code->set_relocation_info(*reloc_info);
 
   // Overwrites the original code.
+  CHECK_LE(desc.instr_size, code->instruction_size());
+  CHECK_IMPLIES(code->has_safepoint_info(),
+                desc.instr_size <= code->safepoint_table_offset());
   code->CopyFrom(desc);
 
   // TODO(jgruber): CopyFrom isn't intended to overwrite existing code, and
@@ -2924,7 +2927,7 @@ void MoveBuiltinsOffHeap(Isolate* isolate) {
 
   CodeSpaceMemoryModificationScope code_allocation(isolate->heap());
   for (int i = 0; i < Builtins::builtin_count; i++) {
-    if (!Builtins::IsIsolateIndependent(i)) continue;
+    if (!Builtins::IsOffHeapSafe(i)) continue;
     Handle<Code> code(builtins->builtin(i));
     InstructionStream* stream = new InstructionStream(*code);
     LogInstructionStream(isolate, *code, stream);

test/cctest/test-isolate-independent-builtins.cc

@@ -82,6 +82,70 @@ TEST(VerifyBuiltinsIsolateIndependence) {
 
   CHECK(!found_mismatch);
 }
+
+TEST(VerifyBuiltinsOffHeapSafety) {
+  Isolate* isolate = CcTest::i_isolate();
+  HandleScope handle_scope(isolate);
+
+  Snapshot::EnsureAllBuiltinsAreDeserialized(isolate);
+
+  constexpr int all_real_modes_mask =
+      (1 << (RelocInfo::LAST_REAL_RELOC_MODE + 1)) - 1;
+  constexpr int mode_mask =
+      all_real_modes_mask & ~RelocInfo::ModeMask(RelocInfo::COMMENT) &
+      ~RelocInfo::ModeMask(RelocInfo::INTERNAL_REFERENCE) &
+      ~RelocInfo::ModeMask(RelocInfo::INTERNAL_REFERENCE_ENCODED) &
+      ~RelocInfo::ModeMask(RelocInfo::CONST_POOL) &
+      ~RelocInfo::ModeMask(RelocInfo::VENEER_POOL) &
+      ~RelocInfo::ModeMask(RelocInfo::EXTERNAL_REFERENCE);
+  STATIC_ASSERT(RelocInfo::LAST_REAL_RELOC_MODE == RelocInfo::VENEER_POOL);
+  STATIC_ASSERT(RelocInfo::ModeMask(RelocInfo::COMMENT) ==
+                (1 << RelocInfo::COMMENT));
+  STATIC_ASSERT(
+      mode_mask ==
+      (RelocInfo::ModeMask(RelocInfo::CODE_TARGET) |
+       RelocInfo::ModeMask(RelocInfo::EMBEDDED_OBJECT) |
+       RelocInfo::ModeMask(RelocInfo::WASM_CONTEXT_REFERENCE) |
+       RelocInfo::ModeMask(RelocInfo::WASM_FUNCTION_TABLE_SIZE_REFERENCE) |
+       RelocInfo::ModeMask(RelocInfo::WASM_GLOBAL_HANDLE) |
+       RelocInfo::ModeMask(RelocInfo::WASM_CALL) |
+       RelocInfo::ModeMask(RelocInfo::JS_TO_WASM_CALL) |
+       RelocInfo::ModeMask(RelocInfo::RUNTIME_ENTRY)));
+
+  constexpr bool kVerbose = false;
+  bool found_mismatch = false;
+  for (int i = 0; i < Builtins::builtin_count; i++) {
+    Code* code = isolate->builtins()->builtin(i);
+
+    if (kVerbose) {
+      printf("%s %s\n", Builtins::KindNameOf(i), isolate->builtins()->name(i));
+    }
+
+    bool is_off_heap_safe = true;
+    for (RelocIterator it(code, mode_mask); !it.done(); it.next()) {
+      is_off_heap_safe = false;
+#ifdef ENABLE_DISASSEMBLER
+      if (kVerbose) {
+        RelocInfo::Mode mode = it.rinfo()->rmode();
+        printf("  %s\n", RelocInfo::RelocModeName(mode));
+      }
+#endif
+    }
+
+    // TODO(jgruber): Remove once we properly set up the on-heap code
+    // trampoline.
+    if (Builtins::IsTooShortForOffHeapTrampoline(i)) is_off_heap_safe = false;
+
+    const bool expected_result = Builtins::IsOffHeapSafe(i);
+    if (is_off_heap_safe != expected_result) {
+      found_mismatch = true;
+      printf("%s %s expected: %d, is: %d\n", Builtins::KindNameOf(i),
+             isolate->builtins()->name(i), expected_result, is_off_heap_safe);
+    }
+  }
+
+  CHECK(!found_mismatch);
+}
 #endif  // V8_EMBEDDED_BUILTINS
 
 // V8_CC_MSVC is true for both MSVC and clang on windows. clang can handle