[builtins] Remove canonicalization during serialization

Now that we generate the embedded blob and off-heap trampolines
directly after builtin generation, the heap should not contain any
remaining references to full on-heap builtin Code objects.

The one exception is the interpreter entry trampoline copy for
profiling. This mechanism was actually broken by canonicalization; we
intended to store a full copy of the IET on the root list, but
serialization replaced it with the canonicalized builtin. This CL
fixes that as a side-effect.

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

src/deoptimizer.h

@@ -841,6 +841,15 @@ class DeoptimizerData {
   explicit DeoptimizerData(Heap* heap);
   ~DeoptimizerData();
 
+#ifdef DEBUG
+  bool IsDeoptEntryCode(Code code) const {
+    for (int i = 0; i < kLastDeoptimizeKind + 1; i++) {
+      if (code == deopt_entry_code_[i]) return true;
+    }
+    return false;
+  }
+#endif  // DEBUG
+
  private:
   Heap* heap_;
   static const int kLastDeoptimizeKind =

src/isolate.cc

@@ -3100,12 +3100,8 @@ void CreateOffHeapTrampolines(Isolate* isolate) {
     Handle<Code> trampoline = isolate->factory()->NewOffHeapTrampolineFor(
         builtins->builtin_handle(i), instruction_start);
 
-    // Note that references to the old, on-heap code objects may still exist on
-    // the heap. This is fine for the sake of serialization, as serialization
-    // will canonicalize all builtins in MaybeCanonicalizeBuiltin().
-    //
-    // From this point onwards, some builtin code objects may be unreachable and
-    // thus collected by the GC.
+    // From this point onwards, the old builtin code object is unreachable and
+    // will be collected by the next GC.
     builtins->set_builtin(i, *trampoline);
 
     if (isolate->logger()->is_listening_to_code_events() ||
@@ -3287,18 +3283,16 @@ bool Isolate::Init(StartupDeserializer* des) {
   setup_delegate_->SetupBuiltins(this);
 #ifndef V8_TARGET_ARCH_ARM
   if (create_heap_objects) {
-    // Create a copy of the the interpreter entry trampoline and store it
-    // on the root list. It is used as a template for further copies that
-    // may later be created to help profile interpreted code.
+    // Store the interpreter entry trampoline on the root list. It is used as a
+    // template for further copies that may later be created to help profile
+    // interpreted code.
     // We currently cannot do this on arm due to RELATIVE_CODE_TARGETs
     // assuming that all possible Code targets may be addressed with an int24
     // offset, effectively limiting code space size to 32MB. We can guarantee
     // this at mksnapshot-time, but not at runtime.
     // See also: https://crbug.com/v8/8713.
-    HandleScope handle_scope(this);
-    Handle<Code> code =
-        factory()->CopyCode(BUILTIN_CODE(this, InterpreterEntryTrampoline));
-    heap_.SetInterpreterEntryTrampolineForProfiling(*code);
+    heap_.SetInterpreterEntryTrampolineForProfiling(
+        heap_.builtin(Builtins::kInterpreterEntryTrampoline));
   }
 #endif
   if (FLAG_embedded_builtins && create_heap_objects) {
@@ -3344,6 +3338,12 @@ bool Isolate::Init(StartupDeserializer* des) {
   // Initialize the builtin entry table.
   Builtins::UpdateBuiltinEntryTable(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())
+              ->is_off_heap_trampoline());
+#endif  // V8_TARGET_ARCH_ARM
+
   if (FLAG_print_builtin_code) builtins()->PrintBuiltinCode();
   if (FLAG_print_builtin_size) builtins()->PrintBuiltinSize();
 

src/snapshot/startup-serializer.cc

@@ -7,6 +7,7 @@
 #include "src/api.h"
 #include "src/code-tracer.h"
 #include "src/contexts.h"
+#include "src/deoptimizer.h"
 #include "src/global-handles.h"
 #include "src/objects-inl.h"
 #include "src/objects/foreign-inl.h"
@@ -30,31 +31,47 @@ StartupSerializer::~StartupSerializer() {
   OutputStatistics("StartupSerializer");
 }
 
+#ifdef DEBUG
 namespace {
 
-// Due to how we currently create the embedded blob, we may encounter both
-// off-heap trampolines and old, outdated full Code objects during
-// serialization. This ensures that we only serialize the canonical version of
-// each builtin.
-// See also CreateOffHeapTrampolines().
-HeapObject MaybeCanonicalizeBuiltin(Isolate* isolate, HeapObject obj) {
-  if (!obj->IsCode()) return obj;
+bool IsUnexpectedCodeObject(Isolate* isolate, HeapObject obj) {
+  if (!obj->IsCode()) return false;
 
-  const int builtin_index = Code::cast(obj)->builtin_index();
-  if (!Builtins::IsBuiltinId(builtin_index)) return obj;
+  Code code = Code::cast(obj);
 
-  return isolate->builtins()->builtin(builtin_index);
+  // TODO(v8:8768): Deopt entry code should not be serialized.
+  if (code->kind() == Code::STUB && isolate->deoptimizer_data() != nullptr) {
+    if (isolate->deoptimizer_data()->IsDeoptEntryCode(code)) return false;
+  }
+
+  if (code->kind() == Code::REGEXP) return false;
+  if (!code->is_builtin()) return true;
+  if (code->is_off_heap_trampoline()) return false;
+
+  // An on-heap builtin. We only expect this for the interpreter entry
+  // trampoline copy stored on the root list and transitively called builtins.
+  // See Heap::interpreter_entry_trampoline_for_profiling.
+
+  switch (code->builtin_index()) {
+    case Builtins::kAbort:
+    case Builtins::kCEntry_Return1_DontSaveFPRegs_ArgvOnStack_NoBuiltinExit:
+    case Builtins::kInterpreterEntryTrampoline:
+    case Builtins::kRecordWrite:
+      return false;
+    default:
+      return true;
+  }
+
+  UNREACHABLE();
 }
 
 }  // namespace
+#endif  // DEBUG
 
 void StartupSerializer::SerializeObject(HeapObject obj, HowToCode how_to_code,
                                         WhereToPoint where_to_point, int skip) {
   DCHECK(!obj->IsJSFunction());
-
-  // TODO(jgruber): Remove canonicalization once off-heap trampoline creation
-  // moves to Isolate::Init().
-  obj = MaybeCanonicalizeBuiltin(isolate(), obj);
+  DCHECK(!IsUnexpectedCodeObject(isolate(), obj));
 
   if (SerializeHotObject(obj, how_to_code, where_to_point, skip)) return;
   if (IsRootAndHasBeenSerialized(obj) &&