From 7eff3cee052499b84ae2c903c676a744af488335 Mon Sep 17 00:00:00 2001 From: Camillo Bruni Date: Wed, 1 Feb 2023 20:13:02 +0100 Subject: [PATCH] [perf-jit] Don't use 0xFF for repeated script names This was never supported to start with and can cause invalid script names. This CL partially reverts https://crrev.com/c/3513892 Drive-by-fix: Dehandlify more code. Change-Id: I96cf4c1244d9f00dc47738cd481b440e6bed0541 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4174074 Commit-Queue: Camillo Bruni Reviewed-by: Jakob Linke Cr-Commit-Position: refs/heads/main@{#85609} --- src/codegen/source-position.cc | 4 +- src/codegen/source-position.h | 2 +- src/diagnostics/perf-jit.cc | 61 +++++++++++++------------- src/diagnostics/perf-jit.h | 4 +- src/logging/log.cc | 55 +++++++++++++---------- src/logging/log.h | 2 +- src/snapshot/serializer.h | 8 ++-- test/unittests/logging/log-unittest.cc | 2 +- 8 files changed, 74 insertions(+), 64 deletions(-) diff --git a/src/codegen/source-position.cc b/src/codegen/source-position.cc index f85c8ca3b8..073265a442 100644 --- a/src/codegen/source-position.cc +++ b/src/codegen/source-position.cc @@ -79,10 +79,10 @@ std::vector SourcePosition::InliningStack(Isolate* isolate, } SourcePositionInfo SourcePosition::FirstInfo(Isolate* isolate, - Handle code) const { + Code code) const { DisallowGarbageCollection no_gc; DeoptimizationData deopt_data = - DeoptimizationData::cast(code->deoptimization_data()); + DeoptimizationData::cast(code.deoptimization_data()); SourcePosition pos = *this; if (pos.isInlined()) { InliningPosition inl = deopt_data.InliningPositions().get(pos.InliningId()); diff --git a/src/codegen/source-position.h b/src/codegen/source-position.h index bd74dd9ff8..8794926c25 100644 --- a/src/codegen/source-position.h +++ b/src/codegen/source-position.h @@ -83,7 +83,7 @@ class SourcePosition final { Code code) const; std::vector InliningStack( OptimizedCompilationInfo* cinfo) const; - SourcePositionInfo FirstInfo(Isolate* isolate, Handle code) const; + SourcePositionInfo FirstInfo(Isolate* isolate, Code code) const; void Print(std::ostream& out, InstructionStream code) const; void PrintJson(std::ostream& out) const; diff --git a/src/diagnostics/perf-jit.cc b/src/diagnostics/perf-jit.cc index c6b88458bb..21d6878b56 100644 --- a/src/diagnostics/perf-jit.cc +++ b/src/diagnostics/perf-jit.cc @@ -117,7 +117,6 @@ const char LinuxPerfJitLogger::kFilenameFormatString[] = "./jit-%d.dump"; const int LinuxPerfJitLogger::kFilenameBufferPadding = 16; static const char kStringTerminator[] = {'\0'}; -static const char kRepeatedNameMarker[] = {'\xff', '\0'}; base::LazyRecursiveMutex LinuxPerfJitLogger::file_mutex_; // The following static variables are protected by @@ -214,11 +213,11 @@ uint64_t LinuxPerfJitLogger::GetTimestamp() { } void LinuxPerfJitLogger::LogRecordedBuffer( - Handle abstract_code, - MaybeHandle maybe_shared, const char* name, - int length) { + AbstractCode abstract_code, MaybeHandle maybe_shared, + const char* name, int length) { + DisallowGarbageCollection no_gc; if (v8_flags.perf_basic_prof_only_functions) { - CodeKind code_kind = abstract_code->kind(isolate_); + CodeKind code_kind = abstract_code.kind(isolate_); if (code_kind != CodeKind::INTERPRETED_FUNCTION && code_kind != CodeKind::TURBOFAN && code_kind != CodeKind::MAGLEV && code_kind != CodeKind::BASELINE) { @@ -231,26 +230,27 @@ void LinuxPerfJitLogger::LogRecordedBuffer( if (perf_output_handle_ == nullptr) return; // We only support non-interpreted functions. - if (!abstract_code->IsCode(isolate_)) return; - Handle code = Handle::cast(abstract_code); + if (!abstract_code.IsCode(isolate_)) return; + Code code = Code::cast(abstract_code); // Debug info has to be emitted first. Handle shared; if (v8_flags.perf_prof && maybe_shared.ToHandle(&shared)) { // TODO(herhut): This currently breaks for js2wasm/wasm2js functions. - if (code->kind() != CodeKind::JS_TO_WASM_FUNCTION && - code->kind() != CodeKind::WASM_TO_JS_FUNCTION) { + CodeKind kind = code.kind(); + if (kind != CodeKind::JS_TO_WASM_FUNCTION && + kind != CodeKind::WASM_TO_JS_FUNCTION) { LogWriteDebugInfo(code, shared); } } const char* code_name = name; - uint8_t* code_pointer = reinterpret_cast(code->InstructionStart()); + uint8_t* code_pointer = reinterpret_cast(code.InstructionStart()); // Unwinding info comes right after debug info. - if (v8_flags.perf_prof_unwinding_info) LogWriteUnwindingInfo(*code); + if (v8_flags.perf_prof_unwinding_info) LogWriteUnwindingInfo(code); - WriteJitCodeLoadEntry(code_pointer, code->InstructionSize(), code_name, + WriteJitCodeLoadEntry(code_pointer, code.InstructionSize(), code_name, length); } @@ -319,11 +319,11 @@ base::Vector GetScriptName(Object maybeScript, } // namespace -SourcePositionInfo GetSourcePositionInfo(Isolate* isolate, Handle code, +SourcePositionInfo GetSourcePositionInfo(Isolate* isolate, Code code, Handle function, SourcePosition pos) { DisallowGarbageCollection disallow; - if (code->is_turbofanned()) { + if (code.is_turbofanned()) { return pos.FirstInfo(isolate, code); } else { return SourcePositionInfo(pos, function); @@ -332,23 +332,25 @@ SourcePositionInfo GetSourcePositionInfo(Isolate* isolate, Handle code, } // namespace -void LinuxPerfJitLogger::LogWriteDebugInfo(Handle code, +void LinuxPerfJitLogger::LogWriteDebugInfo(Code code, Handle shared) { // Line ends of all scripts have been initialized prior to this. DisallowGarbageCollection no_gc; // The WasmToJS wrapper stubs have source position entries. - if (!shared->HasSourceCode()) return; + SharedFunctionInfo raw_shared = *shared; + if (!raw_shared.HasSourceCode()) return; PerfJitCodeDebugInfo debug_info; uint32_t size = sizeof(debug_info); ByteArray source_position_table = - code->SourcePositionTable(isolate_, *shared); + code.SourcePositionTable(isolate_, raw_shared); // Compute the entry count and get the names of all scripts. // Avoid additional work if the script name is repeated. Multiple script // names only occur for cross-script inlining. uint32_t entry_count = 0; Object last_script = Smi::zero(); + size_t last_script_name_size = 0; std::vector> script_names; for (SourcePositionTableIterator iterator(source_position_table); !iterator.done(); iterator.Advance()) { @@ -357,13 +359,15 @@ void LinuxPerfJitLogger::LogWriteDebugInfo(Handle code, Object current_script = *info.script; if (current_script != last_script) { std::unique_ptr name_storage; - auto name = GetScriptName(shared->script(), &name_storage, no_gc); + auto name = GetScriptName(raw_shared.script(), &name_storage, no_gc); script_names.push_back(name); // Add the size of the name after each entry. - size += name.size() + sizeof(kStringTerminator); + last_script_name_size = name.size() + sizeof(kStringTerminator); + size += last_script_name_size; last_script = current_script; } else { - size += sizeof(kRepeatedNameMarker); + DCHECK_LT(0, last_script_name_size); + size += last_script_name_size; } entry_count++; } @@ -371,7 +375,7 @@ void LinuxPerfJitLogger::LogWriteDebugInfo(Handle code, debug_info.event_ = PerfJitCodeLoad::kDebugInfo; debug_info.time_stamp_ = GetTimestamp(); - debug_info.address_ = code->InstructionStart(); + debug_info.address_ = code.InstructionStart(); debug_info.entry_count_ = entry_count; // Add the sizes of fixed parts of entries. @@ -381,7 +385,7 @@ void LinuxPerfJitLogger::LogWriteDebugInfo(Handle code, debug_info.size_ = size + padding; LogWriteBytes(reinterpret_cast(&debug_info), sizeof(debug_info)); - Address code_start = code->InstructionStart(); + Address code_start = code.InstructionStart(); last_script = Smi::zero(); int script_names_index = 0; @@ -398,16 +402,13 @@ void LinuxPerfJitLogger::LogWriteDebugInfo(Handle code, entry.column_ = info.column + 1; LogWriteBytes(reinterpret_cast(&entry), sizeof(entry)); Object current_script = *info.script; + auto name_string = script_names[script_names_index]; + LogWriteBytes(name_string.begin(), + static_cast(name_string.size())); + LogWriteBytes(kStringTerminator, sizeof(kStringTerminator)); if (current_script != last_script) { - auto name_string = script_names[script_names_index]; - LogWriteBytes(name_string.begin(), - static_cast(name_string.size())); - LogWriteBytes(kStringTerminator, sizeof(kStringTerminator)); - script_names_index++; + if (last_script != Smi::zero()) script_names_index++; last_script = current_script; - } else { - // Use the much shorter kRepeatedNameMarker for repeated names. - LogWriteBytes(kRepeatedNameMarker, sizeof(kRepeatedNameMarker)); } } char padding_bytes[8] = {0}; diff --git a/src/diagnostics/perf-jit.h b/src/diagnostics/perf-jit.h index 47f7322615..718903abba 100644 --- a/src/diagnostics/perf-jit.h +++ b/src/diagnostics/perf-jit.h @@ -58,7 +58,7 @@ class LinuxPerfJitLogger : public CodeEventLogger { void CloseMarkerFile(void* marker_address); uint64_t GetTimestamp(); - void LogRecordedBuffer(Handle code, + void LogRecordedBuffer(AbstractCode code, MaybeHandle maybe_shared, const char* name, int length) override; #if V8_ENABLE_WEBASSEMBLY @@ -79,7 +79,7 @@ class LinuxPerfJitLogger : public CodeEventLogger { void LogWriteBytes(const char* bytes, int size); void LogWriteHeader(); - void LogWriteDebugInfo(Handle code, Handle shared); + void LogWriteDebugInfo(Code code, Handle shared); #if V8_ENABLE_WEBASSEMBLY void LogWriteDebugInfo(const wasm::WasmCode* code); #endif // V8_ENABLE_WEBASSEMBLY diff --git a/src/logging/log.cc b/src/logging/log.cc index 671dd69403..9834f4df3e 100644 --- a/src/logging/log.cc +++ b/src/logging/log.cc @@ -19,6 +19,7 @@ #include "src/codegen/bailout-reason.h" #include "src/codegen/macro-assembler.h" #include "src/codegen/source-position-table.h" +#include "src/common/assert-scope.h" #include "src/deoptimizer/deoptimizer.h" #include "src/diagnostics/perf-jit.h" #include "src/execution/isolate.h" @@ -248,7 +249,8 @@ void CodeEventLogger::CodeCreateEvent(CodeTag tag, Handle code, DCHECK(is_listening_to_code_events()); name_buffer_->Init(tag); name_buffer_->AppendBytes(comment); - LogRecordedBuffer(code, MaybeHandle(), + DisallowGarbageCollection no_gc; + LogRecordedBuffer(*code, MaybeHandle(), name_buffer_->get(), name_buffer_->size()); } @@ -257,7 +259,8 @@ void CodeEventLogger::CodeCreateEvent(CodeTag tag, Handle code, DCHECK(is_listening_to_code_events()); name_buffer_->Init(tag); name_buffer_->AppendName(*name); - LogRecordedBuffer(code, MaybeHandle(), + DisallowGarbageCollection no_gc; + LogRecordedBuffer(*code, MaybeHandle(), name_buffer_->get(), name_buffer_->size()); } @@ -269,7 +272,8 @@ void CodeEventLogger::CodeCreateEvent(CodeTag tag, Handle code, name_buffer_->AppendBytes(ComputeMarker(*shared, *code)); name_buffer_->AppendByte(' '); name_buffer_->AppendName(*script_name); - LogRecordedBuffer(code, shared, name_buffer_->get(), name_buffer_->size()); + DisallowGarbageCollection no_gc; + LogRecordedBuffer(*code, shared, name_buffer_->get(), name_buffer_->size()); } void CodeEventLogger::CodeCreateEvent(CodeTag tag, Handle code, @@ -292,7 +296,8 @@ void CodeEventLogger::CodeCreateEvent(CodeTag tag, Handle code, name_buffer_->AppendInt(line); name_buffer_->AppendByte(':'); name_buffer_->AppendInt(column); - LogRecordedBuffer(code, shared, name_buffer_->get(), name_buffer_->size()); + DisallowGarbageCollection no_gc; + LogRecordedBuffer(*code, shared, name_buffer_->get(), name_buffer_->size()); } #if V8_ENABLE_WEBASSEMBLY @@ -312,6 +317,7 @@ void CodeEventLogger::CodeCreateEvent(CodeTag tag, const wasm::WasmCode* code, } name_buffer_->AppendByte('-'); name_buffer_->AppendBytes(ExecutionTierToString(code->tier())); + DisallowGarbageCollection no_gc; LogRecordedBuffer(code, name_buffer_->get(), name_buffer_->size()); } #endif // V8_ENABLE_WEBASSEMBLY @@ -321,7 +327,8 @@ void CodeEventLogger::RegExpCodeCreateEvent(Handle code, DCHECK(is_listening_to_code_events()); name_buffer_->Init(LogEventListener::CodeTag::kRegExp); name_buffer_->AppendString(*source); - LogRecordedBuffer(code, MaybeHandle(), + DisallowGarbageCollection no_gc; + LogRecordedBuffer(*code, MaybeHandle(), name_buffer_->get(), name_buffer_->size()); } @@ -338,7 +345,7 @@ class LinuxPerfBasicLogger : public CodeEventLogger { Handle shared) override {} private: - void LogRecordedBuffer(Handle code, + void LogRecordedBuffer(AbstractCode code, MaybeHandle maybe_shared, const char* name, int length) override; #if V8_ENABLE_WEBASSEMBLY @@ -414,18 +421,19 @@ void LinuxPerfBasicLogger::WriteLogRecordedBuffer(uintptr_t address, int size, size, name_length, name); } -void LinuxPerfBasicLogger::LogRecordedBuffer(Handle code, +void LinuxPerfBasicLogger::LogRecordedBuffer(AbstractCode code, MaybeHandle, const char* name, int length) { + DisallowGarbageCollection no_gc; PtrComprCageBase cage_base(isolate_); if (v8_flags.perf_basic_prof_only_functions && - CodeKindIsBuiltinOrJSFunction(code->kind(cage_base))) { + CodeKindIsBuiltinOrJSFunction(code.kind(cage_base))) { return; } WriteLogRecordedBuffer( - static_cast(code->InstructionStart(cage_base)), - code->InstructionSize(cage_base), name, length); + static_cast(code.InstructionStart(cage_base)), + code.InstructionSize(cage_base), name, length); } #if V8_ENABLE_WEBASSEMBLY @@ -637,7 +645,7 @@ class LowLevelLogger : public CodeEventLogger { void CodeMovingGCEvent() override; private: - void LogRecordedBuffer(Handle code, + void LogRecordedBuffer(AbstractCode code, MaybeHandle maybe_shared, const char* name, int length) override; #if V8_ENABLE_WEBASSEMBLY @@ -727,19 +735,19 @@ void LowLevelLogger::LogCodeInfo() { LogWriteBytes(arch, sizeof(arch)); } -void LowLevelLogger::LogRecordedBuffer(Handle code, +void LowLevelLogger::LogRecordedBuffer(AbstractCode code, MaybeHandle, const char* name, int length) { + DisallowGarbageCollection no_gc; PtrComprCageBase cage_base(isolate_); CodeCreateStruct event; event.name_size = length; - event.code_address = code->InstructionStart(cage_base); - event.code_size = code->InstructionSize(cage_base); + event.code_address = code.InstructionStart(cage_base); + event.code_size = code.InstructionSize(cage_base); LogWriteStruct(event); LogWriteBytes(name, length); - LogWriteBytes( - reinterpret_cast(code->InstructionStart(cage_base)), - code->InstructionSize(cage_base)); + LogWriteBytes(reinterpret_cast(code.InstructionStart(cage_base)), + code.InstructionSize(cage_base)); } #if V8_ENABLE_WEBASSEMBLY @@ -801,7 +809,7 @@ class JitLogger : public CodeEventLogger { JitCodeEvent::CodeType code_type); private: - void LogRecordedBuffer(Handle code, + void LogRecordedBuffer(AbstractCode code, MaybeHandle maybe_shared, const char* name, int length) override; #if V8_ENABLE_WEBASSEMBLY @@ -818,16 +826,17 @@ JitLogger::JitLogger(Isolate* isolate, JitCodeEventHandler code_event_handler) DCHECK_NOT_NULL(code_event_handler); } -void JitLogger::LogRecordedBuffer(Handle code, +void JitLogger::LogRecordedBuffer(AbstractCode code, MaybeHandle maybe_shared, const char* name, int length) { + DisallowGarbageCollection no_gc; PtrComprCageBase cage_base(isolate_); JitCodeEvent event; event.type = JitCodeEvent::CODE_ADDED; - event.code_start = reinterpret_cast(code->InstructionStart(cage_base)); - event.code_type = code->IsCode(cage_base) ? JitCodeEvent::JIT_CODE - : JitCodeEvent::BYTE_CODE; - event.code_len = code->InstructionSize(cage_base); + event.code_start = reinterpret_cast(code.InstructionStart(cage_base)); + event.code_type = + code.IsCode(cage_base) ? JitCodeEvent::JIT_CODE : JitCodeEvent::BYTE_CODE; + event.code_len = code.InstructionSize(cage_base); Handle shared; if (maybe_shared.ToHandle(&shared) && shared->script(cage_base).IsScript(cage_base)) { diff --git a/src/logging/log.h b/src/logging/log.h index 3e211388ee..d411da512b 100644 --- a/src/logging/log.h +++ b/src/logging/log.h @@ -448,7 +448,7 @@ class V8_EXPORT_PRIVATE CodeEventLogger : public LogEventListener { private: class NameBuffer; - virtual void LogRecordedBuffer(Handle code, + virtual void LogRecordedBuffer(AbstractCode code, MaybeHandle maybe_shared, const char* name, int length) = 0; #if V8_ENABLE_WEBASSEMBLY diff --git a/src/snapshot/serializer.h b/src/snapshot/serializer.h index 8063da7726..cfaf9300b5 100644 --- a/src/snapshot/serializer.h +++ b/src/snapshot/serializer.h @@ -118,10 +118,10 @@ class CodeAddressMap : public CodeEventLogger { base::HashMap impl_; }; - void LogRecordedBuffer(Handle code, - MaybeHandle, const char* name, - int length) override { - address_to_name_map_.Insert(code->address(), name, length); + void LogRecordedBuffer(AbstractCode code, MaybeHandle, + const char* name, int length) override { + DisallowGarbageCollection no_gc; + address_to_name_map_.Insert(code.address(), name, length); } #if V8_ENABLE_WEBASSEMBLY diff --git a/test/unittests/logging/log-unittest.cc b/test/unittests/logging/log-unittest.cc index ca48d0629c..fdeb11dda7 100644 --- a/test/unittests/logging/log-unittest.cc +++ b/test/unittests/logging/log-unittest.cc @@ -453,7 +453,7 @@ TEST_F(LogTest, Issue539892) { } private: - void LogRecordedBuffer(i::Handle code, + void LogRecordedBuffer(i::AbstractCode code, i::MaybeHandle maybe_shared, const char* name, int length) override {} #if V8_ENABLE_WEBASSEMBLY