Fix CodeMoveEvent

The invariants in this method are fairly strict since it is called
during object evacution and thus a) objects may be in transitory states
and b) multiple threads are working on evacuation objects concurrently.

Previously, this method ensured valid object accesses because only the
object currently being observed by ProfilingMigrationObserver was
accessed. This changed with crrev.com/c/4178821, where we (incorrectly)
also accessed another object (InstructionStream::code), leading to data
races and incorrect behavior.

This CL fixes that problem by changing LogEventListener API as follows:

 void CodeMoveEvent(InstructionStream from, InstructionStream to);
 void BytecodeMoveEvent(BytecodeArray from, BytecodeArray to);

With this change we again correctly observe invariants, and also remove
one use of AbstractCode.

Bug: v8:13654
Change-Id: Ida022e8c7f14d821e1139f025edc71c20fa386c0
Fixed: chromium:1409786
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4194192
Commit-Queue: Jakob Linke <jgruber@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#85474}

src/diagnostics/perf-jit.cc

@@ -518,12 +518,6 @@ void LinuxPerfJitLogger::LogWriteUnwindingInfo(Code code) {
   LogWriteBytes(padding_bytes, static_cast<int>(padding_size));
 }
 
-void LinuxPerfJitLogger::CodeMoveEvent(AbstractCode from, AbstractCode to) {
-  // We may receive a CodeMove event if a BytecodeArray object moves. Otherwise
-  // code relocation is not supported.
-  CHECK(from.IsBytecodeArray(isolate_));
-}
-
 void LinuxPerfJitLogger::LogWriteBytes(const char* bytes, int size) {
   size_t rv = fwrite(bytes, 1, size, perf_output_handle_);
   DCHECK(static_cast<size_t>(size) == rv);

src/diagnostics/perf-jit.h

@@ -44,7 +44,10 @@ class LinuxPerfJitLogger : public CodeEventLogger {
   explicit LinuxPerfJitLogger(Isolate* isolate);
   ~LinuxPerfJitLogger() override;
 
-  void CodeMoveEvent(AbstractCode from, AbstractCode to) override;
+  void CodeMoveEvent(InstructionStream from, InstructionStream to) override {
+    UNREACHABLE();  // Unsupported.
+  }
+  void BytecodeMoveEvent(BytecodeArray from, BytecodeArray to) override {}
   void CodeDisableOptEvent(Handle<AbstractCode> code,
                            Handle<SharedFunctionInfo> shared) override {}
 

src/heap/mark-compact.cc

@@ -1649,14 +1649,15 @@ class ProfilingMigrationObserver final : public MigrationObserver {
 
   inline void Move(AllocationSpace dest, HeapObject src, HeapObject dst,
                    int size) final {
+    // Note this method is called in a concurrent setting. The current object
+    // (src and dst) is somewhat safe to access without precautions, but other
+    // objects may be subject to concurrent modification.
     if (dest == CODE_SPACE) {
-      Code src_code = InstructionStream::cast(src).GcSafeCode(kAcquireLoad);
-      Code dst_code = InstructionStream::cast(dst).GcSafeCode(kAcquireLoad);
-      PROFILE(heap_->isolate(), CodeMoveEvent(AbstractCode::cast(src_code),
-                                              AbstractCode::cast(dst_code)));
+      PROFILE(heap_->isolate(), CodeMoveEvent(InstructionStream::cast(src),
+                                              InstructionStream::cast(dst)));
     } else if (dest == OLD_SPACE && dst.IsBytecodeArray()) {
-      PROFILE(heap_->isolate(),
-              CodeMoveEvent(AbstractCode::cast(src), AbstractCode::cast(dst)));
+      PROFILE(heap_->isolate(), BytecodeMoveEvent(BytecodeArray::cast(src),
+                                                  BytecodeArray::cast(dst)));
     }
     heap_->OnMoveEvent(src, dst, size);
   }

src/logging/code-events.h

@@ -88,7 +88,8 @@ class LogEventListener {
   virtual void RegExpCodeCreateEvent(Handle<AbstractCode> code,
                                      Handle<String> source) = 0;
   // Not handlified as this happens during GC. No allocation allowed.
-  virtual void CodeMoveEvent(AbstractCode from, AbstractCode to) = 0;
+  virtual void CodeMoveEvent(InstructionStream from, InstructionStream to) = 0;
+  virtual void BytecodeMoveEvent(BytecodeArray from, BytecodeArray to) = 0;
   virtual void SharedFunctionInfoMoveEvent(Address from, Address to) = 0;
   virtual void NativeContextMoveEvent(Address from, Address to) = 0;
   virtual void CodeMovingGCEvent() = 0;
@@ -205,12 +206,18 @@ class Logger {
       listener->RegExpCodeCreateEvent(code, source);
     }
   }
-  void CodeMoveEvent(AbstractCode from, AbstractCode to) {
+  void CodeMoveEvent(InstructionStream from, InstructionStream to) {
     base::MutexGuard guard(&mutex_);
     for (auto listener : listeners_) {
       listener->CodeMoveEvent(from, to);
     }
   }
+  void BytecodeMoveEvent(BytecodeArray from, BytecodeArray to) {
+    base::MutexGuard guard(&mutex_);
+    for (auto listener : listeners_) {
+      listener->BytecodeMoveEvent(from, to);
+    }
+  }
   void SharedFunctionInfoMoveEvent(Address from, Address to) {
     base::MutexGuard guard(&mutex_);
     for (auto listener : listeners_) {

src/logging/log.cc

@@ -332,7 +332,8 @@ class LinuxPerfBasicLogger : public CodeEventLogger {
   explicit LinuxPerfBasicLogger(Isolate* isolate);
   ~LinuxPerfBasicLogger() override;
 
-  void CodeMoveEvent(AbstractCode from, AbstractCode to) override {}
+  void CodeMoveEvent(InstructionStream from, InstructionStream to) override {}
+  void BytecodeMoveEvent(BytecodeArray from, BytecodeArray to) override {}
   void CodeDisableOptEvent(Handle<AbstractCode> code,
                            Handle<SharedFunctionInfo> shared) override {}
 
@@ -587,22 +588,38 @@ void ExternalLogEventListener::RegExpCodeCreateEvent(Handle<AbstractCode> code,
   code_event_handler_->Handle(reinterpret_cast<v8::CodeEvent*>(&code_event));
 }
 
-void ExternalLogEventListener::CodeMoveEvent(AbstractCode from,
-                                             AbstractCode to) {
-  PtrComprCageBase cage_base(isolate_);
-  CodeEvent code_event;
-  code_event.previous_code_start_address =
-      static_cast<uintptr_t>(from.InstructionStart(cage_base));
-  code_event.code_start_address =
-      static_cast<uintptr_t>(to.InstructionStart(cage_base));
-  code_event.code_size = static_cast<size_t>(to.InstructionSize(cage_base));
-  code_event.function_name = isolate_->factory()->empty_string();
-  code_event.script_name = isolate_->factory()->empty_string();
-  code_event.script_line = 0;
-  code_event.script_column = 0;
-  code_event.code_type = v8::CodeEventType::kRelocationType;
-  code_event.comment = "";
+namespace {
 
+void InitializeCodeEvent(Isolate* isolate, CodeEvent* event,
+                         Address previous_code_start_address,
+                         Address code_start_address, int code_size) {
+  event->previous_code_start_address =
+      static_cast<uintptr_t>(previous_code_start_address);
+  event->code_start_address = static_cast<uintptr_t>(code_start_address);
+  event->code_size = static_cast<size_t>(code_size);
+  event->function_name = isolate->factory()->empty_string();
+  event->script_name = isolate->factory()->empty_string();
+  event->script_line = 0;
+  event->script_column = 0;
+  event->code_type = v8::CodeEventType::kRelocationType;
+  event->comment = "";
+}
+
+}  // namespace
+
+void ExternalLogEventListener::CodeMoveEvent(InstructionStream from,
+                                             InstructionStream to) {
+  CodeEvent code_event;
+  InitializeCodeEvent(isolate_, &code_event, from.InstructionStart(),
+                      to.InstructionStart(), to.InstructionSize());
+  code_event_handler_->Handle(reinterpret_cast<v8::CodeEvent*>(&code_event));
+}
+
+void ExternalLogEventListener::BytecodeMoveEvent(BytecodeArray from,
+                                                 BytecodeArray to) {
+  CodeEvent code_event;
+  InitializeCodeEvent(isolate_, &code_event, from.GetFirstBytecodeAddress(),
+                      to.GetFirstBytecodeAddress(), to.length());
   code_event_handler_->Handle(reinterpret_cast<v8::CodeEvent*>(&code_event));
 }
 
@@ -612,7 +629,8 @@ class LowLevelLogger : public CodeEventLogger {
   LowLevelLogger(Isolate* isolate, const char* file_name);
   ~LowLevelLogger() override;
 
-  void CodeMoveEvent(AbstractCode from, AbstractCode to) override;
+  void CodeMoveEvent(InstructionStream from, InstructionStream to) override;
+  void BytecodeMoveEvent(BytecodeArray from, BytecodeArray to) override;
   void CodeDisableOptEvent(Handle<AbstractCode> code,
                            Handle<SharedFunctionInfo> shared) override {}
   void SnapshotPositionEvent(HeapObject obj, int pos);
@@ -738,11 +756,18 @@ void LowLevelLogger::LogRecordedBuffer(const wasm::WasmCode* code,
 }
 #endif  // V8_ENABLE_WEBASSEMBLY
 
-void LowLevelLogger::CodeMoveEvent(AbstractCode from, AbstractCode to) {
-  PtrComprCageBase cage_base(isolate_);
+void LowLevelLogger::CodeMoveEvent(InstructionStream from,
+                                   InstructionStream to) {
   CodeMoveStruct event;
-  event.from_address = from.InstructionStart(cage_base);
-  event.to_address = to.InstructionStart(cage_base);
+  event.from_address = from.InstructionStart();
+  event.to_address = to.InstructionStart();
+  LogWriteStruct(event);
+}
+
+void LowLevelLogger::BytecodeMoveEvent(BytecodeArray from, BytecodeArray to) {
+  CodeMoveStruct event;
+  event.from_address = from.GetFirstBytecodeAddress();
+  event.to_address = to.GetFirstBytecodeAddress();
   LogWriteStruct(event);
 }
 
@@ -762,7 +787,8 @@ class JitLogger : public CodeEventLogger {
  public:
   JitLogger(Isolate* isolate, JitCodeEventHandler code_event_handler);
 
-  void CodeMoveEvent(AbstractCode from, AbstractCode to) override;
+  void CodeMoveEvent(InstructionStream from, InstructionStream to) override;
+  void BytecodeMoveEvent(BytecodeArray from, BytecodeArray to) override;
   void CodeDisableOptEvent(Handle<AbstractCode> code,
                            Handle<SharedFunctionInfo> shared) override {}
   void AddCodeLinePosInfoEvent(void* jit_handler_data, int pc_offset,
@@ -867,19 +893,29 @@ void JitLogger::LogRecordedBuffer(const wasm::WasmCode* code, const char* name,
 }
 #endif  // V8_ENABLE_WEBASSEMBLY
 
-void JitLogger::CodeMoveEvent(AbstractCode from, AbstractCode to) {
+void JitLogger::CodeMoveEvent(InstructionStream from, InstructionStream to) {
   base::MutexGuard guard(&logger_mutex_);
 
-  PtrComprCageBase cage_base(isolate_);
   JitCodeEvent event;
   event.type = JitCodeEvent::CODE_MOVED;
-  event.code_type = from.IsInstructionStream(cage_base)
-                        ? JitCodeEvent::JIT_CODE
-                        : JitCodeEvent::BYTE_CODE;
-  event.code_start = reinterpret_cast<void*>(from.InstructionStart(cage_base));
-  event.code_len = from.InstructionSize(cage_base);
-  event.new_code_start =
-      reinterpret_cast<void*>(to.InstructionStart(cage_base));
+  event.code_type = JitCodeEvent::JIT_CODE;
+  event.code_start = reinterpret_cast<void*>(from.InstructionStart());
+  event.code_len = from.InstructionSize();
+  event.new_code_start = reinterpret_cast<void*>(to.InstructionStart());
+  event.isolate = reinterpret_cast<v8::Isolate*>(isolate_);
+
+  code_event_handler_(&event);
+}
+
+void JitLogger::BytecodeMoveEvent(BytecodeArray from, BytecodeArray to) {
+  base::MutexGuard guard(&logger_mutex_);
+
+  JitCodeEvent event;
+  event.type = JitCodeEvent::CODE_MOVED;
+  event.code_type = JitCodeEvent::BYTE_CODE;
+  event.code_start = reinterpret_cast<void*>(from.GetFirstBytecodeAddress());
+  event.code_len = from.length();
+  event.new_code_start = reinterpret_cast<void*>(to.GetFirstBytecodeAddress());
   event.isolate = reinterpret_cast<v8::Isolate*>(isolate_);
 
   code_event_handler_(&event);
@@ -1540,11 +1576,16 @@ void V8FileLogger::RegExpCodeCreateEvent(Handle<AbstractCode> code,
   msg.WriteToLogFile();
 }
 
-void V8FileLogger::CodeMoveEvent(AbstractCode from, AbstractCode to) {
+void V8FileLogger::CodeMoveEvent(InstructionStream from, InstructionStream to) {
   if (!is_listening_to_code_events()) return;
-  PtrComprCageBase cage_base(isolate_);
-  MoveEventInternal(Event::kCodeMove, from.InstructionStart(cage_base),
-                    to.InstructionStart(cage_base));
+  MoveEventInternal(Event::kCodeMove, from.InstructionStart(),
+                    to.InstructionStart());
+}
+
+void V8FileLogger::BytecodeMoveEvent(BytecodeArray from, BytecodeArray to) {
+  if (!is_listening_to_code_events()) return;
+  MoveEventInternal(Event::kCodeMove, from.GetFirstBytecodeAddress(),
+                    to.GetFirstBytecodeAddress());
 }
 
 void V8FileLogger::SharedFunctionInfoMoveEvent(Address from, Address to) {

src/logging/log.h

@@ -190,7 +190,8 @@ class V8FileLogger : public LogEventListener {
   void SetterCallbackEvent(Handle<Name> name, Address entry_point) override;
   void RegExpCodeCreateEvent(Handle<AbstractCode> code,
                              Handle<String> source) override;
-  void CodeMoveEvent(AbstractCode from, AbstractCode to) override;
+  void CodeMoveEvent(InstructionStream from, InstructionStream to) override;
+  void BytecodeMoveEvent(BytecodeArray from, BytecodeArray to) override;
   void SharedFunctionInfoMoveEvent(Address from, Address to) override;
   void NativeContextMoveEvent(Address from, Address to) override {}
   void CodeMovingGCEvent() override;
@@ -499,7 +500,8 @@ class ExternalLogEventListener : public LogEventListener {
   void SetterCallbackEvent(Handle<Name> name, Address entry_point) override {}
   void SharedFunctionInfoMoveEvent(Address from, Address to) override {}
   void NativeContextMoveEvent(Address from, Address to) override {}
-  void CodeMoveEvent(AbstractCode from, AbstractCode to) override;
+  void CodeMoveEvent(InstructionStream from, InstructionStream to) override;
+  void BytecodeMoveEvent(BytecodeArray from, BytecodeArray to) override;
   void CodeDisableOptEvent(Handle<AbstractCode> code,
                            Handle<SharedFunctionInfo> shared) override {}
   void CodeMovingGCEvent() override {}

src/profiler/profiler-listener.cc

@@ -297,13 +297,22 @@ void ProfilerListener::RegExpCodeCreateEvent(Handle<AbstractCode> code,
   DispatchCodeEvent(evt_rec);
 }
 
-void ProfilerListener::CodeMoveEvent(AbstractCode from, AbstractCode to) {
+void ProfilerListener::CodeMoveEvent(InstructionStream from,
+                                     InstructionStream to) {
   DisallowGarbageCollection no_gc;
   CodeEventsContainer evt_rec(CodeEventRecord::Type::kCodeMove);
   CodeMoveEventRecord* rec = &evt_rec.CodeMoveEventRecord_;
-  PtrComprCageBase cage_base(isolate_);
-  rec->from_instruction_start = from.InstructionStart(cage_base);
-  rec->to_instruction_start = to.InstructionStart(cage_base);
+  rec->from_instruction_start = from.InstructionStart();
+  rec->to_instruction_start = to.InstructionStart();
+  DispatchCodeEvent(evt_rec);
+}
+
+void ProfilerListener::BytecodeMoveEvent(BytecodeArray from, BytecodeArray to) {
+  DisallowGarbageCollection no_gc;
+  CodeEventsContainer evt_rec(CodeEventRecord::Type::kCodeMove);
+  CodeMoveEventRecord* rec = &evt_rec.CodeMoveEventRecord_;
+  rec->from_instruction_start = from.GetFirstBytecodeAddress();
+  rec->to_instruction_start = to.GetFirstBytecodeAddress();
   DispatchCodeEvent(evt_rec);
 }
 

src/profiler/profiler-listener.h

@@ -56,7 +56,8 @@ class V8_EXPORT_PRIVATE ProfilerListener : public LogEventListener,
   void SetterCallbackEvent(Handle<Name> name, Address entry_point) override;
   void RegExpCodeCreateEvent(Handle<AbstractCode> code,
                              Handle<String> source) override;
-  void CodeMoveEvent(AbstractCode from, AbstractCode to) override;
+  void CodeMoveEvent(InstructionStream from, InstructionStream to) override;
+  void BytecodeMoveEvent(BytecodeArray from, BytecodeArray to) override;
   void SharedFunctionInfoMoveEvent(Address from, Address to) override {}
   void NativeContextMoveEvent(Address from, Address to) override;
   void CodeMovingGCEvent() override {}

src/runtime/runtime-test.cc

@@ -1661,7 +1661,8 @@ RUNTIME_FUNCTION(Runtime_EnableCodeLoggingForTesting) {
     void SetterCallbackEvent(Handle<Name> name, Address entry_point) final {}
     void RegExpCodeCreateEvent(Handle<AbstractCode> code,
                                Handle<String> source) final {}
-    void CodeMoveEvent(AbstractCode from, AbstractCode to) final {}
+    void CodeMoveEvent(InstructionStream from, InstructionStream to) final {}
+    void BytecodeMoveEvent(BytecodeArray from, BytecodeArray to) final {}
     void SharedFunctionInfoMoveEvent(Address from, Address to) final {}
     void NativeContextMoveEvent(Address from, Address to) final {}
     void CodeMovingGCEvent() final {}

src/snapshot/serializer.h

@@ -29,7 +29,10 @@ class CodeAddressMap : public CodeEventLogger {
     isolate_->v8_file_logger()->RemoveLogEventListener(this);
   }
 
-  void CodeMoveEvent(AbstractCode from, AbstractCode to) override {
+  void CodeMoveEvent(InstructionStream from, InstructionStream to) override {
+    address_to_name_map_.Move(from.address(), to.address());
+  }
+  void BytecodeMoveEvent(BytecodeArray from, BytecodeArray to) override {
     address_to_name_map_.Move(from.address(), to.address());
   }
 

test/cctest/test-cpu-profiler.cc

@@ -198,9 +198,17 @@ TEST(CodeEvents) {
                                     comment_code, "comment");
   profiler_listener.CodeCreateEvent(i::LogEventListener::CodeTag::kBuiltin,
                                     comment2_code, "comment2");
-  profiler_listener.CodeMoveEvent(*comment2_code, *moved_code);
 
   PtrComprCageBase cage_base(isolate);
+  if (comment2_code->IsBytecodeArray(cage_base)) {
+    profiler_listener.BytecodeMoveEvent(comment2_code->GetBytecodeArray(),
+                                        moved_code->GetBytecodeArray());
+  } else {
+    profiler_listener.CodeMoveEvent(
+        comment2_code->GetCode().instruction_stream(),
+        moved_code->GetCode().instruction_stream());
+  }
+
   // Enqueue a tick event to enable code events processing.
   EnqueueTickSampleEvent(processor, aaa_code->InstructionStart(cage_base));
 

test/unittests/logging/log-unittest.cc

@@ -444,7 +444,10 @@ TEST_F(LogTest, Issue539892) {
     explicit FakeCodeEventLogger(i::Isolate* isolate)
         : CodeEventLogger(isolate) {}
 
-    void CodeMoveEvent(i::AbstractCode from, i::AbstractCode to) override {}
+    void CodeMoveEvent(i::InstructionStream from,
+                       i::InstructionStream to) override {}
+    void BytecodeMoveEvent(i::BytecodeArray from,
+                           i::BytecodeArray to) override {}
     void CodeDisableOptEvent(i::Handle<i::AbstractCode> code,
                              i::Handle<i::SharedFunctionInfo> shared) override {
     }