From ee0c7f459d71c6373848a2045c94a3b1fb76e68f Mon Sep 17 00:00:00 2001 From: Jakob Linke Date: Wed, 25 Jan 2023 14:16:45 +0100 Subject: [PATCH] Fix CodeMoveEvent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Dominik Inführ Reviewed-by: Camillo Bruni Cr-Commit-Position: refs/heads/main@{#85474} --- src/diagnostics/perf-jit.cc | 6 -- src/diagnostics/perf-jit.h | 5 +- src/heap/mark-compact.cc | 13 +-- src/logging/code-events.h | 11 ++- src/logging/log.cc | 111 +++++++++++++++++-------- src/logging/log.h | 6 +- src/profiler/profiler-listener.cc | 17 +++- src/profiler/profiler-listener.h | 3 +- src/runtime/runtime-test.cc | 3 +- src/snapshot/serializer.h | 5 +- test/cctest/test-cpu-profiler.cc | 10 ++- test/unittests/logging/log-unittest.cc | 5 +- 12 files changed, 134 insertions(+), 61 deletions(-) diff --git a/src/diagnostics/perf-jit.cc b/src/diagnostics/perf-jit.cc index 0feab10bf6..c6b88458bb 100644 --- a/src/diagnostics/perf-jit.cc +++ b/src/diagnostics/perf-jit.cc @@ -518,12 +518,6 @@ void LinuxPerfJitLogger::LogWriteUnwindingInfo(Code code) { LogWriteBytes(padding_bytes, static_cast(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) == rv); diff --git a/src/diagnostics/perf-jit.h b/src/diagnostics/perf-jit.h index 0211b1baf6..47f7322615 100644 --- a/src/diagnostics/perf-jit.h +++ b/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 code, Handle shared) override {} diff --git a/src/heap/mark-compact.cc b/src/heap/mark-compact.cc index 327e56c526..f3fb1a5e9b 100644 --- a/src/heap/mark-compact.cc +++ b/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); } diff --git a/src/logging/code-events.h b/src/logging/code-events.h index 997bb91866..562eaff77a 100644 --- a/src/logging/code-events.h +++ b/src/logging/code-events.h @@ -88,7 +88,8 @@ class LogEventListener { virtual void RegExpCodeCreateEvent(Handle code, Handle 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_) { diff --git a/src/logging/log.cc b/src/logging/log.cc index 9432a91134..37ccf02487 100644 --- a/src/logging/log.cc +++ b/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 code, Handle shared) override {} @@ -587,22 +588,38 @@ void ExternalLogEventListener::RegExpCodeCreateEvent(Handle code, code_event_handler_->Handle(reinterpret_cast(&code_event)); } -void ExternalLogEventListener::CodeMoveEvent(AbstractCode from, - AbstractCode to) { - PtrComprCageBase cage_base(isolate_); - CodeEvent code_event; - code_event.previous_code_start_address = - static_cast(from.InstructionStart(cage_base)); - code_event.code_start_address = - static_cast(to.InstructionStart(cage_base)); - code_event.code_size = static_cast(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(previous_code_start_address); + event->code_start_address = static_cast(code_start_address); + event->code_size = static_cast(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(&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(&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 code, Handle 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 code, Handle 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(from.InstructionStart(cage_base)); - event.code_len = from.InstructionSize(cage_base); - event.new_code_start = - reinterpret_cast(to.InstructionStart(cage_base)); + event.code_type = JitCodeEvent::JIT_CODE; + event.code_start = reinterpret_cast(from.InstructionStart()); + event.code_len = from.InstructionSize(); + event.new_code_start = reinterpret_cast(to.InstructionStart()); + event.isolate = reinterpret_cast(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(from.GetFirstBytecodeAddress()); + event.code_len = from.length(); + event.new_code_start = reinterpret_cast(to.GetFirstBytecodeAddress()); event.isolate = reinterpret_cast(isolate_); code_event_handler_(&event); @@ -1540,11 +1576,16 @@ void V8FileLogger::RegExpCodeCreateEvent(Handle 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) { diff --git a/src/logging/log.h b/src/logging/log.h index 2e243e39f8..3e211388ee 100644 --- a/src/logging/log.h +++ b/src/logging/log.h @@ -190,7 +190,8 @@ class V8FileLogger : public LogEventListener { void SetterCallbackEvent(Handle name, Address entry_point) override; void RegExpCodeCreateEvent(Handle code, Handle 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, 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 code, Handle shared) override {} void CodeMovingGCEvent() override {} diff --git a/src/profiler/profiler-listener.cc b/src/profiler/profiler-listener.cc index 277d480bb7..cb36bdb6d5 100644 --- a/src/profiler/profiler-listener.cc +++ b/src/profiler/profiler-listener.cc @@ -297,13 +297,22 @@ void ProfilerListener::RegExpCodeCreateEvent(Handle 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); } diff --git a/src/profiler/profiler-listener.h b/src/profiler/profiler-listener.h index 57ec80267d..3517ab56b5 100644 --- a/src/profiler/profiler-listener.h +++ b/src/profiler/profiler-listener.h @@ -56,7 +56,8 @@ class V8_EXPORT_PRIVATE ProfilerListener : public LogEventListener, void SetterCallbackEvent(Handle name, Address entry_point) override; void RegExpCodeCreateEvent(Handle code, Handle 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 {} diff --git a/src/runtime/runtime-test.cc b/src/runtime/runtime-test.cc index 8ea23f27da..5560d24052 100644 --- a/src/runtime/runtime-test.cc +++ b/src/runtime/runtime-test.cc @@ -1661,7 +1661,8 @@ RUNTIME_FUNCTION(Runtime_EnableCodeLoggingForTesting) { void SetterCallbackEvent(Handle name, Address entry_point) final {} void RegExpCodeCreateEvent(Handle code, Handle 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 {} diff --git a/src/snapshot/serializer.h b/src/snapshot/serializer.h index a0672b2578..8063da7726 100644 --- a/src/snapshot/serializer.h +++ b/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()); } diff --git a/test/cctest/test-cpu-profiler.cc b/test/cctest/test-cpu-profiler.cc index 5b3d08d79b..fd67eac71c 100644 --- a/test/cctest/test-cpu-profiler.cc +++ b/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)); diff --git a/test/unittests/logging/log-unittest.cc b/test/unittests/logging/log-unittest.cc index 6994b2b020..ca48d0629c 100644 --- a/test/unittests/logging/log-unittest.cc +++ b/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 code, i::Handle shared) override { }