Revert "Reland "[builtins] Verify Isolate compatibility with the embedded blob""

This reverts commit 1e3582b5.

Reason for revert: Still fails nosnap: https://ci.chromium.org/p/v8/builders/luci.v8.ci/V8%20Linux%20-%20nosnap%20-%20debug/22789

Original change's description:
> Reland "[builtins] Verify Isolate compatibility with the embedded blob"
> 
> This is a reland of b022e825
> 
> Original change's description:
> > [builtins] Verify Isolate compatibility with the embedded blob
> >
> > Embedded builtins (= the embedded blob) have a few dependencies on the
> > snapshot state. For instance, they require that metadata stored on
> > builtin Code objects as well as the builtins constant table remain
> > unchanged from mksnapshot-time. Embedders may violate these
> > assumptions by accident, e.g. by loading a snapshot generated with
> > different build flags, leading to seemingly unrelated failures later
> > on.
> >
> > This CL introduces an Isolate hash stored in the embedded blob which
> > hashes relevant parts of builtin Code objects and the builtins
> > constant table. It's verified in Isolate::Init in debug builds.
> >
> > Bug: v8:8723
> > Change-Id: Ifc9bdbe6f56ea67d8984f162afa73a3572cfbba8
> > Reviewed-on: https://chromium-review.googlesource.com/c/1442641
> > Commit-Queue: Jakob Gruber <jgruber@chromium.org>
> > Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#59177}
> 
> Tbr: yangguo@chromium.org,sigurds@chromium.org
> Bug: v8:8723
> Change-Id: I1dd001783f0f1fae21a9809c8639e40f55b8f663
> Reviewed-on: https://chromium-review.googlesource.com/c/1445985
> Commit-Queue: Jakob Gruber <jgruber@chromium.org>
> Reviewed-by: Jakob Gruber <jgruber@chromium.org>
> Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#59236}

TBR=yangguo@chromium.org,sigurds@chromium.org,jgruber@chromium.org

Change-Id: If6082452c739d4de44ed70d3c6355f5282684ac1
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:8723
Reviewed-on: https://chromium-review.googlesource.com/c/1448311Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59241}

src/isolate.cc

@@ -213,7 +213,7 @@ void Isolate::SetEmbeddedBlob(const uint8_t* blob, uint32_t blob_size) {
   // Verify that the contents of the embedded blob are unchanged from
   // serialization-time, just to ensure the compiler isn't messing with us.
   EmbeddedData d = EmbeddedData::FromBlob();
-  CHECK_EQ(d.EmbeddedBlobHash(), d.CreateEmbeddedBlobHash());
+  CHECK_EQ(d.Hash(), d.CreateHash());
 #endif  // DEBUG
 }
 
@@ -244,45 +244,6 @@ uint32_t Isolate::CurrentEmbeddedBlobSize() {
       std::memory_order::memory_order_relaxed);
 }
 
-size_t Isolate::HashIsolateForEmbeddedBlob() {
-  DCHECK(builtins_.is_initialized());
-  DCHECK(FLAG_embedded_builtins);
-  DCHECK(Builtins::AllBuiltinsAreIsolateIndependent());
-
-  DisallowHeapAllocation no_gc;
-
-  static constexpr size_t kSeed = 0;
-  size_t hash = kSeed;
-
-  // Hash data sections of builtin code objects.
-  for (int i = 0; i < Builtins::builtin_count; i++) {
-    Code code = heap_.builtin(i);
-
-    DCHECK(Internals::HasHeapObjectTag(code.ptr()));
-    uint8_t* const code_ptr =
-        reinterpret_cast<uint8_t*>(code.ptr() - kHeapObjectTag);
-
-    // These static asserts ensure we don't miss relevant fields. We don't hash
-    // instruction size and flags since they change when creating the off-heap
-    // trampolines. Other data fields must remain the same.
-    STATIC_ASSERT(Code::kInstructionSizeOffset == Code::kDataStart);
-    STATIC_ASSERT(Code::kFlagsOffset == Code::kInstructionSizeOffsetEnd + 1);
-    STATIC_ASSERT(Code::kSafepointTableOffsetOffset ==
-                  Code::kFlagsOffsetEnd + 1);
-    static constexpr int kStartOffset = Code::kSafepointTableOffsetOffset;
-
-    for (int j = kStartOffset; j < Code::kHeaderPaddingStart; j++) {
-      hash = base::hash_combine(hash, size_t{code_ptr[j]});
-    }
-  }
-
-  // The builtins constants table is also tightly tied to embedded builtins.
-  hash = base::hash_combine(
-      hash, static_cast<size_t>(heap_.builtins_constants_table()->length()));
-
-  return hash;
-}
-
 void ThreadLocalTop::Initialize(Isolate* isolate) {
   *this = ThreadLocalTop();
   isolate_ = isolate;
@@ -3150,13 +3111,6 @@ void CreateOffHeapTrampolines(Isolate* isolate) {
   }
 }
 
-#ifdef DEBUG
-bool IsolateIsCompatibleWithEmbeddedBlob(Isolate* isolate) {
-  EmbeddedData d = EmbeddedData::FromBlob(isolate);
-  return (d.IsolateHash() == isolate->HashIsolateForEmbeddedBlob());
-}
-#endif  // DEBUG
-
 }  // namespace
 
 void Isolate::InitializeDefaultEmbeddedBlob() {
@@ -3384,12 +3338,6 @@ bool Isolate::Init(StartupDeserializer* des) {
   // Initialize the builtin entry table.
   Builtins::UpdateBuiltinEntryTable(this);
 
-  // Verify that the current heap state (usually deserialized from the snapshot)
-  // is compatible with the embedded blob. If this DCHECK fails, we've likely
-  // loaded a snapshot generated by a different V8 version or build-time
-  // configuration.
-  DCHECK(IsolateIsCompatibleWithEmbeddedBlob(this));
-
 #ifndef V8_TARGET_ARCH_ARM
   // The IET for profiling should always be a full on-heap Code object.
   DCHECK(!Code::cast(heap_.interpreter_entry_trampoline_for_profiling())

src/isolate.h

@@ -1470,11 +1470,6 @@ class Isolate final : private HiddenFactory {
     return builtins_constants_table_builder_;
   }
 
-  // Hashes bits of the Isolate that are relevant for embedded builtins. In
-  // particular, the embedded blob requires builtin Code object layout and the
-  // builtins constants table to remain unchanged from build-time.
-  size_t HashIsolateForEmbeddedBlob();
-
   static const uint8_t* CurrentEmbeddedBlob();
   static uint32_t CurrentEmbeddedBlobSize();
   static bool CurrentEmbeddedBlobIsBinaryEmbedded();

src/snapshot/embedded-data.cc

@@ -232,13 +232,6 @@ EmbeddedData EmbeddedData::FromIsolate(Isolate* isolate) {
   // between two builtins with int3's (on x64/ia32).
   ZapCode(reinterpret_cast<Address>(blob), blob_size);
 
-  // Hash relevant parts of the Isolate's heap and store the result.
-  {
-    STATIC_ASSERT(IsolateHashSize() == kSizetSize);
-    const size_t hash = isolate->HashIsolateForEmbeddedBlob();
-    std::memcpy(blob + IsolateHashOffset(), &hash, IsolateHashSize());
-  }
-
   // Write the metadata tables.
   DCHECK_EQ(MetadataSize(), sizeof(metadata[0]) * metadata.size());
   std::memcpy(blob + MetadataOffset(), metadata.data(), MetadataSize());
@@ -261,14 +254,12 @@ EmbeddedData EmbeddedData::FromIsolate(Isolate* isolate) {
   FinalizeEmbeddedCodeTargets(isolate, &d);
 
   // Hash the blob and store the result.
-  {
-    STATIC_ASSERT(EmbeddedBlobHashSize() == kSizetSize);
-    const size_t hash = d.CreateEmbeddedBlobHash();
-    std::memcpy(blob + EmbeddedBlobHashOffset(), &hash, EmbeddedBlobHashSize());
+  STATIC_ASSERT(HashSize() == kSizetSize);
+  const size_t hash = d.CreateHash();
+  std::memcpy(blob + HashOffset(), &hash, HashSize());
 
-    DCHECK_EQ(hash, d.CreateEmbeddedBlobHash());
-    DCHECK_EQ(hash, d.EmbeddedBlobHash());
-  }
+  DCHECK_EQ(hash, d.CreateHash());
+  DCHECK_EQ(hash, d.Hash());
 
   if (FLAG_serialization_statistics) d.PrintStatistics();
 
@@ -290,10 +281,10 @@ uint32_t EmbeddedData::InstructionSizeOfBuiltin(int i) const {
   return metadata[i].instructions_length;
 }
 
-size_t EmbeddedData::CreateEmbeddedBlobHash() const {
-  STATIC_ASSERT(EmbeddedBlobHashOffset() == 0);
-  STATIC_ASSERT(EmbeddedBlobHashSize() == kSizetSize);
-  return base::hash_range(data_ + EmbeddedBlobHashSize(), data_ + size_);
+size_t EmbeddedData::CreateHash() const {
+  STATIC_ASSERT(HashOffset() == 0);
+  STATIC_ASSERT(HashSize() == kSizetSize);
+  return base::hash_range(data_ + HashSize(), data_ + size_);
 }
 
 void EmbeddedData::PrintStatistics() const {
@@ -320,8 +311,7 @@ void EmbeddedData::PrintStatistics() const {
   const int k90th = embedded_count * 0.90;
   const int k99th = embedded_count * 0.99;
 
-  const int metadata_size = static_cast<int>(
-      EmbeddedBlobHashSize() + IsolateHashSize() + MetadataSize());
+  const int metadata_size = static_cast<int>(HashSize() + MetadataSize());
 
   PrintF("EmbeddedData:\n");
   PrintF("  Total size:                         %d\n",

src/snapshot/embedded-data.h

@@ -71,13 +71,9 @@ class EmbeddedData final {
     return (size == 0) ? 0 : PadAndAlign(size);
   }
 
-  size_t CreateEmbeddedBlobHash() const;
-  size_t EmbeddedBlobHash() const {
-    return *reinterpret_cast<const size_t*>(data_ + EmbeddedBlobHashOffset());
-  }
-
-  size_t IsolateHash() const {
-    return *reinterpret_cast<const size_t*>(data_ + IsolateHashOffset());
+  size_t CreateHash() const;
+  size_t Hash() const {
+    return *reinterpret_cast<const size_t*>(data_ + HashOffset());
   }
 
   struct Metadata {
@@ -92,20 +88,15 @@ class EmbeddedData final {
   // The layout of the blob is as follows:
   //
   // [0] hash of the remaining blob
-  // [1] hash of embedded-blob-relevant heap objects
-  // [2] metadata of instruction stream 0
+  // [1] metadata of instruction stream 0
   // ... metadata
   // ... instruction streams
 
   static constexpr uint32_t kTableSize = Builtins::builtin_count;
-  static constexpr uint32_t EmbeddedBlobHashOffset() { return 0; }
-  static constexpr uint32_t EmbeddedBlobHashSize() { return kSizetSize; }
-  static constexpr uint32_t IsolateHashOffset() {
-    return EmbeddedBlobHashOffset() + EmbeddedBlobHashSize();
-  }
-  static constexpr uint32_t IsolateHashSize() { return kSizetSize; }
+  static constexpr uint32_t HashOffset() { return 0; }
+  static constexpr uint32_t HashSize() { return kSizetSize; }
   static constexpr uint32_t MetadataOffset() {
-    return IsolateHashOffset() + IsolateHashSize();
+    return HashOffset() + HashSize();
   }
   static constexpr uint32_t MetadataSize() {
     return sizeof(struct Metadata) * kTableSize;

test/cctest/test-serialize.cc

@@ -1337,7 +1337,6 @@ UNINITIALIZED_TEST(CustomSnapshotDataBlobOutdatedContextWithOverflow) {
 
 UNINITIALIZED_TEST(CustomSnapshotDataBlobWithLocker) {
   DisableAlwaysOpt();
-  DisableEmbeddedBlobRefcounting();
   v8::Isolate::CreateParams create_params;
   create_params.array_buffer_allocator = CcTest::array_buffer_allocator();
   DisableEmbeddedBlobRefcounting();
@@ -2576,9 +2575,8 @@ TEST(Regress503552) {
   delete cache_data;
 }
 
-UNINITIALIZED_TEST(SnapshotCreatorMultipleContexts) {
+TEST(SnapshotCreatorMultipleContexts) {
   DisableAlwaysOpt();
-  DisableEmbeddedBlobRefcounting();
   v8::StartupData blob;
   {
     v8::SnapshotCreator creator;
@@ -2637,7 +2635,6 @@ UNINITIALIZED_TEST(SnapshotCreatorMultipleContexts) {
 
   isolate->Dispose();
   delete[] blob.data;
-  FreeCurrentEmbeddedBlob();
 }
 
 static int serialized_static_field = 314;
@@ -2700,9 +2697,8 @@ intptr_t replaced_external_references[] = {
 intptr_t short_external_references[] = {
     reinterpret_cast<intptr_t>(SerializedCallbackReplacement), 0};
 
-UNINITIALIZED_TEST(SnapshotCreatorExternalReferences) {
+TEST(SnapshotCreatorExternalReferences) {
   DisableAlwaysOpt();
-  DisableEmbeddedBlobRefcounting();
   v8::StartupData blob;
   {
     v8::SnapshotCreator creator(original_external_references);
@@ -2789,12 +2785,10 @@ UNINITIALIZED_TEST(SnapshotCreatorExternalReferences) {
   CHECK_EQ(3, serializable_two_byte_resource.dispose_count());
 
   delete[] blob.data;
-  FreeCurrentEmbeddedBlob();
 }
 
-UNINITIALIZED_TEST(SnapshotCreatorShortExternalReferences) {
+TEST(SnapshotCreatorShortExternalReferences) {
   DisableAlwaysOpt();
-  DisableEmbeddedBlobRefcounting();
   v8::StartupData blob;
   {
     v8::SnapshotCreator creator(original_external_references);
@@ -2833,7 +2827,6 @@ UNINITIALIZED_TEST(SnapshotCreatorShortExternalReferences) {
     isolate->Dispose();
   }
   delete[] blob.data;
-  FreeCurrentEmbeddedBlob();
 }
 
 v8::StartupData CreateSnapshotWithDefaultAndCustom() {
@@ -2870,9 +2863,8 @@ v8::StartupData CreateSnapshotWithDefaultAndCustom() {
   return creator.CreateBlob(v8::SnapshotCreator::FunctionCodeHandling::kClear);
 }
 
-UNINITIALIZED_TEST(SnapshotCreatorNoExternalReferencesDefault) {
+TEST(SnapshotCreatorNoExternalReferencesDefault) {
   DisableAlwaysOpt();
-  DisableEmbeddedBlobRefcounting();
   v8::StartupData blob = CreateSnapshotWithDefaultAndCustom();
 
   // Deserialize with an incomplete list of external references.
@@ -2893,7 +2885,6 @@ UNINITIALIZED_TEST(SnapshotCreatorNoExternalReferencesDefault) {
     isolate->Dispose();
   }
   delete[] blob.data;
-  FreeCurrentEmbeddedBlob();
 }
 
 v8::StartupData CreateCustomSnapshotWithPreparseDataAndNoOuterScope() {
@@ -2919,9 +2910,8 @@ v8::StartupData CreateCustomSnapshotWithPreparseDataAndNoOuterScope() {
   return creator.CreateBlob(v8::SnapshotCreator::FunctionCodeHandling::kClear);
 }
 
-UNINITIALIZED_TEST(SnapshotCreatorPreparseDataAndNoOuterScope) {
+TEST(SnapshotCreatorPreparseDataAndNoOuterScope) {
   DisableAlwaysOpt();
-  DisableEmbeddedBlobRefcounting();
   v8::StartupData blob = CreateCustomSnapshotWithPreparseDataAndNoOuterScope();
 
   // Deserialize with an incomplete list of external references.
@@ -2940,7 +2930,6 @@ UNINITIALIZED_TEST(SnapshotCreatorPreparseDataAndNoOuterScope) {
     isolate->Dispose();
   }
   delete[] blob.data;
-  FreeCurrentEmbeddedBlob();
 }
 
 v8::StartupData CreateCustomSnapshotArrayJoinWithKeep() {
@@ -2961,9 +2950,8 @@ v8::StartupData CreateCustomSnapshotArrayJoinWithKeep() {
   return creator.CreateBlob(v8::SnapshotCreator::FunctionCodeHandling::kKeep);
 }
 
-UNINITIALIZED_TEST(SnapshotCreatorArrayJoinWithKeep) {
+TEST(SnapshotCreatorArrayJoinWithKeep) {
   DisableAlwaysOpt();
-  DisableEmbeddedBlobRefcounting();
   v8::StartupData blob = CreateCustomSnapshotArrayJoinWithKeep();
 
   // Deserialize with an incomplete list of external references.
@@ -2983,7 +2971,6 @@ UNINITIALIZED_TEST(SnapshotCreatorArrayJoinWithKeep) {
     isolate->Dispose();
   }
   delete[] blob.data;
-  FreeCurrentEmbeddedBlob();
 }
 
 TEST(SnapshotCreatorNoExternalReferencesCustomFail1) {
@@ -3036,9 +3023,8 @@ TEST(SnapshotCreatorNoExternalReferencesCustomFail2) {
   delete[] blob.data;
 }
 
-UNINITIALIZED_TEST(SnapshotCreatorUnknownExternalReferences) {
+TEST(SnapshotCreatorUnknownExternalReferences) {
   DisableAlwaysOpt();
-  DisableEmbeddedBlobRefcounting();
   v8::SnapshotCreator creator;
   v8::Isolate* isolate = creator.GetIsolate();
   {
@@ -3059,12 +3045,10 @@ UNINITIALIZED_TEST(SnapshotCreatorUnknownExternalReferences) {
       creator.CreateBlob(v8::SnapshotCreator::FunctionCodeHandling::kClear);
 
   delete[] blob.data;
-  FreeCurrentEmbeddedBlob();
 }
 
-UNINITIALIZED_TEST(SnapshotCreatorTemplates) {
+TEST(SnapshotCreatorTemplates) {
   DisableAlwaysOpt();
-  DisableEmbeddedBlobRefcounting();
   v8::StartupData blob;
 
   {
@@ -3228,12 +3212,10 @@ UNINITIALIZED_TEST(SnapshotCreatorTemplates) {
     isolate->Dispose();
   }
   delete[] blob.data;
-  FreeCurrentEmbeddedBlob();
 }
 
-UNINITIALIZED_TEST(SnapshotCreatorAddData) {
+TEST(SnapshotCreatorAddData) {
   DisableAlwaysOpt();
-  DisableEmbeddedBlobRefcounting();
   v8::StartupData blob;
 
   {
@@ -3430,7 +3412,6 @@ UNINITIALIZED_TEST(SnapshotCreatorAddData) {
     isolate->Dispose();
   }
   delete[] blob.data;
-  FreeCurrentEmbeddedBlob();
 }
 
 TEST(SnapshotCreatorUnknownHandles) {
@@ -3458,9 +3439,8 @@ TEST(SnapshotCreatorUnknownHandles) {
   delete[] blob.data;
 }
 
-UNINITIALIZED_TEST(SnapshotCreatorIncludeGlobalProxy) {
+TEST(SnapshotCreatorIncludeGlobalProxy) {
   DisableAlwaysOpt();
-  DisableEmbeddedBlobRefcounting();
   v8::StartupData blob;
 
   {
@@ -3648,7 +3628,6 @@ UNINITIALIZED_TEST(SnapshotCreatorIncludeGlobalProxy) {
     isolate->Dispose();
   }
   delete[] blob.data;
-  FreeCurrentEmbeddedBlob();
 }
 
 UNINITIALIZED_TEST(ReinitializeHashSeedNotRehashable) {
@@ -3770,7 +3749,7 @@ UNINITIALIZED_TEST(ReinitializeHashSeedRehashable) {
   FreeCurrentEmbeddedBlob();
 }
 
-UNINITIALIZED_TEST(SerializationStats) {
+TEST(SerializationStats) {
   FLAG_profile_deserialization = true;
   FLAG_always_opt = false;
   v8::StartupData blob = CreateSnapshotDataBlob();
@@ -3785,8 +3764,6 @@ UNINITIALIZED_TEST(SerializationStats) {
     }
     PrintF("Embedded blob is %d bytes\n", embedded_blob_size);
   }
-
-  FreeCurrentEmbeddedBlob();
 }
 
 void CheckSFIsAreWeak(WeakFixedArray sfis, Isolate* isolate) {
@@ -3805,11 +3782,10 @@ void CheckSFIsAreWeak(WeakFixedArray sfis, Isolate* isolate) {
   CHECK_GT(no_of_weak, 0);
 }
 
-UNINITIALIZED_TEST(WeakArraySerializationInSnapshot) {
+TEST(WeakArraySerializizationInSnapshot) {
   const char* code = "var my_func = function() { }";
 
   DisableAlwaysOpt();
-  DisableEmbeddedBlobRefcounting();
   i::FLAG_allow_natives_syntax = true;
   v8::StartupData blob;
   {
@@ -3851,11 +3827,10 @@ UNINITIALIZED_TEST(WeakArraySerializationInSnapshot) {
     // Verify that the pointers in shared_function_infos are weak.
     WeakFixedArray sfis =
         Script::cast(function->shared()->script())->shared_function_infos();
-    CheckSFIsAreWeak(sfis, reinterpret_cast<i::Isolate*>(isolate));
+    CheckSFIsAreWeak(sfis, CcTest::i_isolate());
   }
   isolate->Dispose();
   delete[] blob.data;
-  FreeCurrentEmbeddedBlob();
 }
 
 TEST(WeakArraySerializationInCodeCache) {