[builtins] Properly handle cctest heap creation in embedded builds

Some cctests force fresh creation of heap constants, even though the
cctest binary itself is an embedded snapshot build (i.e.: a snapshot
blob exists, and a binary-embedded blob exists). This breaks a few
assumptions, for example that off-heap builtins have a single,
canonical off-heap code range.

Unfortunately this isn't that easy to fix. I see a few alternatives:

1. In builtins setup, if an embedded blob exists, regenerate the
builtins for their metadata (things like the safepoint table offset),
and then replace them by off-heap trampolines.

2. As above, but deserialize the trampolines from the snapshot blob.

3. As above, but pack required metadata into the embedded blob and
create trampolines from there.

4. Act as if the embedded blob does not exist.

Alternative 1 does not work because the generated code can be slightly
different at at runtime vs. mksnapshot-time. Alternative 2 is out
because we do not have access to the snapshot blob in TestIsolate
setup. Alternative 3 is probably the preferred option but would be a
more involved change.

This CL takes path 4. It's not an optimal solution, but it can be
replace by alternative 3 later.

TBR=ulan@chromium.org

Bug: v8:7718, v8:7751
Change-Id: I36c024cb0179615011c886ed3978bc95f0d197ac
Reviewed-on: https://chromium-review.googlesource.com/1098924Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53886}

src/builtins/builtins.cc

@@ -200,8 +200,10 @@ bool Builtins::IsBuiltinHandle(Handle<HeapObject> maybe_code,
 // static
 bool Builtins::IsEmbeddedBuiltin(const Code* code) {
 #ifdef V8_EMBEDDED_BUILTINS
-  return Builtins::IsBuiltinId(code->builtin_index()) &&
-         Builtins::IsIsolateIndependent(code->builtin_index());
+  const int builtin_index = code->builtin_index();
+  return Isolate::CurrentEmbeddedBlob() != nullptr &&
+         Builtins::IsBuiltinId(builtin_index) &&
+         Builtins::IsIsolateIndependent(builtin_index);
 #else
   return false;
 #endif

src/builtins/setup-builtins-internal.cc

@@ -152,12 +152,14 @@ Code* BuildWithCodeStubAssemblerCS(Isolate* isolate, int32_t builtin_index,
 }
 }  // anonymous namespace
 
+// static
 void SetupIsolateDelegate::AddBuiltin(Builtins* builtins, int index,
                                       Code* code) {
   DCHECK_EQ(index, code->builtin_index());
   builtins->set_builtin(index, code);
 }
 
+// static
 void SetupIsolateDelegate::PopulateWithPlaceholders(Isolate* isolate) {
   // Fill the builtins list with placeholders. References to these placeholder
   // builtins are eventually replaced by the actual builtins. This is to
@@ -170,6 +172,7 @@ void SetupIsolateDelegate::PopulateWithPlaceholders(Isolate* isolate) {
   }
 }
 
+// static
 void SetupIsolateDelegate::ReplacePlaceholders(Isolate* isolate) {
   // Replace references from all code objects to placeholders.
   Builtins* builtins = isolate->builtins();
@@ -209,6 +212,7 @@ void SetupIsolateDelegate::ReplacePlaceholders(Isolate* isolate) {
   }
 }
 
+// static
 void SetupIsolateDelegate::SetupBuiltinsInternal(Isolate* isolate) {
   Builtins* builtins = isolate->builtins();
   DCHECK(!builtins->initialized_);

src/disassembler.cc

@@ -134,7 +134,8 @@ static void PrintRelocInfo(StringBuilder* out, Isolate* isolate,
     out->AddFormatted("    ;; external reference (%s)", reference_name);
   } else if (RelocInfo::IsCodeTarget(rmode)) {
     out->AddFormatted("    ;; code:");
-    Code* code = Code::GetCodeFromTargetAddress(relocinfo->target_address());
+    Code* code = isolate->heap()->GcSafeFindCodeForInnerPointer(
+        relocinfo->target_address());
     Code::Kind kind = code->kind();
     if (kind == Code::STUB) {
       // Get the STUB key and extract major and minor key.

src/heap/factory.cc

@@ -2583,10 +2583,10 @@ Handle<Code> Factory::NewCodeForDeserialization(uint32_t size) {
 #ifdef V8_EMBEDDED_BUILTINS
 Handle<Code> Factory::NewOffHeapTrampolineFor(Handle<Code> code,
                                               Address off_heap_entry) {
-  DCHECK(isolate()->serializer_enabled());
-  DCHECK_NOT_NULL(isolate()->embedded_blob());
-  DCHECK_NE(0, isolate()->embedded_blob_size());
-  DCHECK(Builtins::IsEmbeddedBuiltin(*code));
+  CHECK(isolate()->serializer_enabled());
+  CHECK_NOT_NULL(isolate()->embedded_blob());
+  CHECK_NE(0, isolate()->embedded_blob_size());
+  CHECK(Builtins::IsEmbeddedBuiltin(*code));
 
   Handle<Code> result =
       Builtins::GenerateOffHeapTrampolineFor(isolate(), off_heap_entry);

src/isolate.cc

@@ -2577,6 +2577,18 @@ Isolate::Isolate()
   tracing_cpu_profiler_.reset(new TracingCpuProfilerImpl(this));
 
   init_memcopy_functions(this);
+
+#ifdef V8_EMBEDDED_BUILTINS
+#ifdef V8_MULTI_SNAPSHOTS
+  if (FLAG_untrusted_code_mitigations) {
+    SetEmbeddedBlob(DefaultEmbeddedBlob(), DefaultEmbeddedBlobSize());
+  } else {
+    SetEmbeddedBlob(TrustedEmbeddedBlob(), TrustedEmbeddedBlobSize());
+  }
+#else
+  SetEmbeddedBlob(DefaultEmbeddedBlob(), DefaultEmbeddedBlobSize());
+#endif
+#endif  // V8_EMBEDDED_BUILTINS
 }
 
 
@@ -2963,18 +2975,6 @@ bool Isolate::Init(StartupDeserializer* des) {
   compiler_dispatcher_ =
       new CompilerDispatcher(this, V8::GetCurrentPlatform(), FLAG_stack_size);
 
-#ifdef V8_EMBEDDED_BUILTINS
-#ifdef V8_MULTI_SNAPSHOTS
-  if (FLAG_untrusted_code_mitigations) {
-    SetEmbeddedBlob(DefaultEmbeddedBlob(), DefaultEmbeddedBlobSize());
-  } else {
-    SetEmbeddedBlob(TrustedEmbeddedBlob(), TrustedEmbeddedBlobSize());
-  }
-#else
-  SetEmbeddedBlob(DefaultEmbeddedBlob(), DefaultEmbeddedBlobSize());
-#endif
-#endif
-
   // Enable logging before setting up the heap
   logger_->SetUp(this);
 

src/objects/code-inl.h

@@ -530,6 +530,14 @@ Address Code::constant_pool() const {
 }
 
 Code* Code::GetCodeFromTargetAddress(Address address) {
+#ifdef V8_EMBEDDED_BUILTINS
+  // TODO(jgruber,v8:6666): Support embedded builtins here. We'd need to pass in
+  // the current isolate.
+  Address start = reinterpret_cast<Address>(Isolate::CurrentEmbeddedBlob());
+  Address end = start + Isolate::CurrentEmbeddedBlobSize();
+  CHECK(address < start || address >= end);
+#endif  // V8_EMBEDDED_BUILTINS
+
   HeapObject* code = HeapObject::FromAddress(address - Code::kHeaderSize);
   // GetCodeFromTargetAddress might be called when marking objects during mark
   // sweep. reinterpret_cast is therefore used instead of the more appropriate

src/safepoint-table.cc

@@ -71,6 +71,7 @@ SafepointEntry SafepointTable::FindEntry(Address pc) const {
   // We use kMaxUInt32 as sentinel value, so check that we don't hit that.
   DCHECK_NE(kMaxUInt32, pc_offset);
   unsigned len = length();
+  CHECK_GT(len, 0);
   // If pc == kMaxUInt32, then this entry covers all call sites in the function.
   if (len == 1 && GetPcOffset(0) == kMaxUInt32) return GetEntry(0);
   for (unsigned i = 0; i < len; i++) {

test/cctest/cctest.status

@@ -134,13 +134,6 @@
   'test-strings/StringOOM*': [PASS, ['mode == debug', SKIP]],
   'test-serialize/CustomSnapshotDataBlobImmortalImmovableRoots': [PASS, ['mode == debug', SKIP]],
   'test-parsing/ObjectRestNegativeTestSlow': [PASS, ['mode == debug', SKIP]],
-
-  # Until https://crbug.com/v8/7718 is fixed.
-  'test-serialize/PartialSerializerCustomContext': [SKIP],
-  'test-serialize/StartupSerializerOnce': [SKIP],
-  'test-serialize/StartupSerializerOnceRunScript': [SKIP],
-  'test-serialize/StartupSerializerTwice': [SKIP],
-  'test-serialize/StartupSerializerTwiceRunScript': [SKIP],
 }],  # ALWAYS
 
 ##############################################################################

test/cctest/test-serialize.cc

@@ -75,9 +75,10 @@ void DisableAlwaysOpt() {
 // TestIsolate is used for testing isolate serialization.
 class TestIsolate : public Isolate {
  public:
-  static v8::Isolate* NewInitialized(bool enable_serializer) {
-    i::Isolate* isolate = new TestIsolate(enable_serializer);
-    isolate->setup_delegate_ = new SetupIsolateDelegateForTests(true);
+  static v8::Isolate* NewInitialized() {
+    const bool kEnableSerializer = true;
+    const bool kGenerateHeap = true;
+    i::Isolate* isolate = new TestIsolate(kEnableSerializer, kGenerateHeap);
     v8::Isolate* v8_isolate = reinterpret_cast<v8::Isolate*>(isolate);
     v8::Isolate::Scope isolate_scope(v8_isolate);
     isolate->Init(nullptr);
@@ -88,23 +89,37 @@ class TestIsolate : public Isolate {
   // Allows flexibility to bootstrap with or without snapshot even when
   // the production Isolate class has one or the other behavior baked in.
   static v8::Isolate* New(const v8::Isolate::CreateParams& params) {
-    i::Isolate* isolate = new TestIsolate(false);
-    bool create_heap_objects = params.snapshot_blob == nullptr;
-    isolate->setup_delegate_ =
-        new SetupIsolateDelegateForTests(create_heap_objects);
+    const bool kEnableSerializer = false;
+    const bool kGenerateHeap = params.snapshot_blob == nullptr;
+    i::Isolate* isolate = new TestIsolate(kEnableSerializer, kGenerateHeap);
     v8::Isolate* v8_isolate = reinterpret_cast<v8::Isolate*>(isolate);
     v8::Isolate::Initialize(v8_isolate, params);
     return v8_isolate;
   }
-  explicit TestIsolate(bool with_serializer) : Isolate() {
+  explicit TestIsolate(bool with_serializer, bool generate_heap) : Isolate() {
     if (with_serializer) enable_serializer();
     set_array_buffer_allocator(CcTest::array_buffer_allocator());
+    setup_delegate_ = new SetupIsolateDelegateForTests(generate_heap);
+
+#ifdef V8_EMBEDDED_BUILTINS
+    if (generate_heap || clear_embedded_blob_) {
+      // We're generating the heap, including new builtins. Act as if we don't
+      // have an embedded blob.
+      clear_embedded_blob_ = true;
+      SetEmbeddedBlob(nullptr, 0);
     }
-  void SetDeserializeFromSnapshot() {
-    setup_delegate_ = new SetupIsolateDelegateForTests(false);
+#endif  // V8_EMBEDDED_BUILTINS
   }
+
+ private:
+  // A sticky flag that ensures the embedded blob is remains cleared after it
+  // has been cleared once. E.g.: after creating & serializing a complete heap
+  // snapshot, future isolates also expect the embedded blob to be cleared.
+  static bool clear_embedded_blob_;
 };
 
+bool TestIsolate::clear_embedded_blob_ = false;
+
 static Vector<const byte> WritePayload(const Vector<const byte>& payload) {
   int length = payload.length();
   byte* blob = NewArray<byte>(length);
@@ -256,10 +271,11 @@ v8::Isolate* InitializeFromBlob(StartupBlobs& blobs) {
     SnapshotData startup_snapshot(blobs.startup);
     BuiltinSnapshotData builtin_snapshot(blobs.builtin);
     StartupDeserializer deserializer(&startup_snapshot, &builtin_snapshot);
-    TestIsolate* isolate = new TestIsolate(false);
+    const bool kEnableSerializer = false;
+    const bool kGenerateHeap = false;
+    TestIsolate* isolate = new TestIsolate(kEnableSerializer, kGenerateHeap);
     v8_isolate = reinterpret_cast<v8::Isolate*>(isolate);
     v8::Isolate::Scope isolate_scope(v8_isolate);
-    isolate->SetDeserializeFromSnapshot();
     isolate->Init(&deserializer);
   }
   return v8_isolate;
@@ -286,7 +302,7 @@ static void SanityCheck(v8::Isolate* v8_isolate) {
 UNINITIALIZED_TEST(StartupSerializerOnce) {
   DisableLazyDeserialization();
   DisableAlwaysOpt();
-  v8::Isolate* isolate = TestIsolate::NewInitialized(true);
+  v8::Isolate* isolate = TestIsolate::NewInitialized();
   StartupBlobs blobs = Serialize(isolate);
   isolate->Dispose();
   isolate = Deserialize(blobs);
@@ -354,7 +370,7 @@ UNINITIALIZED_TEST(StartupSerializerRootMapDependencies) {
 UNINITIALIZED_TEST(StartupSerializerTwice) {
   DisableLazyDeserialization();
   DisableAlwaysOpt();
-  v8::Isolate* isolate = TestIsolate::NewInitialized(true);
+  v8::Isolate* isolate = TestIsolate::NewInitialized();
   StartupBlobs blobs1 = Serialize(isolate);
   StartupBlobs blobs2 = Serialize(isolate);
   isolate->Dispose();
@@ -376,7 +392,7 @@ UNINITIALIZED_TEST(StartupSerializerTwice) {
 UNINITIALIZED_TEST(StartupSerializerOnceRunScript) {
   DisableLazyDeserialization();
   DisableAlwaysOpt();
-  v8::Isolate* isolate = TestIsolate::NewInitialized(true);
+  v8::Isolate* isolate = TestIsolate::NewInitialized();
   StartupBlobs blobs = Serialize(isolate);
   isolate->Dispose();
   isolate = Deserialize(blobs);
@@ -402,7 +418,7 @@ UNINITIALIZED_TEST(StartupSerializerOnceRunScript) {
 UNINITIALIZED_TEST(StartupSerializerTwiceRunScript) {
   DisableLazyDeserialization();
   DisableAlwaysOpt();
-  v8::Isolate* isolate = TestIsolate::NewInitialized(true);
+  v8::Isolate* isolate = TestIsolate::NewInitialized();
   StartupBlobs blobs1 = Serialize(isolate);
   StartupBlobs blobs2 = Serialize(isolate);
   isolate->Dispose();
@@ -429,7 +445,7 @@ UNINITIALIZED_TEST(StartupSerializerTwiceRunScript) {
 static void PartiallySerializeContext(Vector<const byte>* startup_blob_out,
                                       Vector<const byte>* builtin_blob_out,
                                       Vector<const byte>* partial_blob_out) {
-  v8::Isolate* v8_isolate = TestIsolate::NewInitialized(true);
+  v8::Isolate* v8_isolate = TestIsolate::NewInitialized();
   Isolate* isolate = reinterpret_cast<Isolate*>(v8_isolate);
   Heap* heap = isolate->heap();
   {
@@ -533,7 +549,7 @@ UNINITIALIZED_TEST(PartialSerializerContext) {
 static void PartiallySerializeCustomContext(
     Vector<const byte>* startup_blob_out, Vector<const byte>* builtin_blob_out,
     Vector<const byte>* partial_blob_out) {
-  v8::Isolate* v8_isolate = TestIsolate::NewInitialized(true);
+  v8::Isolate* v8_isolate = TestIsolate::NewInitialized();
   Isolate* isolate = reinterpret_cast<Isolate*>(v8_isolate);
   {
     v8::Isolate::Scope isolate_scope(v8_isolate);