From 4ac65c1b112ab3e4d178b2c7c1b423e35b9f4cfe Mon Sep 17 00:00:00 2001 From: Jakob Gruber Date: Wed, 27 Apr 2022 11:30:51 +0200 Subject: [PATCH] [osr] Fix DeoptExitIsInsideOsrLoop in presence of inlining This logic was confused in the presence of inlined frames; the deopt exit offset would point inside the innermost inlined frame while we incorrectly assumed it points at the outermost frame. Fix this by always referring to the bytecode offset of the outermost frame. Bug: v8:12161 Fixed: chromium:1320094 Change-Id: I2eb28498639432c5344859f64a9388d93ee23bde Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3608630 Auto-Submit: Jakob Linke Reviewed-by: Toon Verwaest Commit-Queue: Jakob Linke Cr-Commit-Position: refs/heads/main@{#80212} --- src/deoptimizer/deoptimizer.cc | 16 ++++++++------ src/deoptimizer/deoptimizer.h | 12 +++++++---- src/interpreter/bytecode-array-iterator.cc | 10 +++++++++ src/interpreter/bytecode-array-iterator.h | 3 +++ src/runtime/runtime-compiler.cc | 11 ++++++---- test/mjsunit/regress/regress-1320094.js | 25 ++++++++++++++++++++++ 6 files changed, 63 insertions(+), 14 deletions(-) create mode 100644 test/mjsunit/regress/regress-1320094.js diff --git a/src/deoptimizer/deoptimizer.cc b/src/deoptimizer/deoptimizer.cc index 4adbce7a1e..68382da025 100644 --- a/src/deoptimizer/deoptimizer.cc +++ b/src/deoptimizer/deoptimizer.cc @@ -533,7 +533,6 @@ Deoptimizer::Deoptimizer(Isolate* isolate, JSFunction function, DCHECK_EQ(0, offset % kLazyDeoptExitSize); deopt_exit_index_ = eager_deopt_count + (offset / kLazyDeoptExitSize); } - deopt_exit_bytecode_offset_ = deopt_data.GetBytecodeOffset(deopt_exit_index_); } Code Deoptimizer::FindOptimizedCode() { @@ -642,7 +641,8 @@ int LookupCatchHandler(Isolate* isolate, TranslatedFrame* translated_frame, } // namespace -void Deoptimizer::TraceDeoptBegin(int optimization_id) { +void Deoptimizer::TraceDeoptBegin(int optimization_id, + BytecodeOffset bytecode_offset) { DCHECK(tracing_enabled()); FILE* file = trace_scope()->file(); Deoptimizer::DeoptInfo info = @@ -666,9 +666,8 @@ void Deoptimizer::TraceDeoptBegin(int optimization_id) { #ifdef DEBUG info.node_id, #endif // DEBUG - deopt_exit_bytecode_offset_.ToInt(), deopt_exit_index_, - fp_to_sp_delta_, caller_frame_top_, - PointerAuthentication::StripPAC(from_)); + bytecode_offset.ToInt(), deopt_exit_index_, fp_to_sp_delta_, + caller_frame_top_, PointerAuthentication::StripPAC(from_)); if (verbose_tracing_enabled() && deopt_kind_ != DeoptimizeKind::kLazy) { PrintF(file, " ;;; deoptimize at "); OFStream outstr(file); @@ -796,13 +795,15 @@ void Deoptimizer::DoComputeOutputFrames() { CHECK_GT(static_cast(caller_frame_top_), stack_guard->real_jslimit()); + BytecodeOffset bytecode_offset = + input_data.GetBytecodeOffset(deopt_exit_index_); ByteArray translations = input_data.TranslationByteArray(); unsigned translation_index = input_data.TranslationIndex(deopt_exit_index_).value(); if (tracing_enabled()) { timer.Start(); - TraceDeoptBegin(input_data.OptimizationId().value()); + TraceDeoptBegin(input_data.OptimizationId().value(), bytecode_offset); } FILE* trace_file = @@ -817,6 +818,9 @@ void Deoptimizer::DoComputeOutputFrames() { : 0, actual_argument_count_ - kJSArgcReceiverSlots); + bytecode_offset_in_outermost_frame_ = + translated_state_.frames()[0].bytecode_offset(); + // Do the input frame to output frame(s) translation. size_t count = translated_state_.frames().size(); // If we are supposed to go to the catch handler, find the catching frame diff --git a/src/deoptimizer/deoptimizer.h b/src/deoptimizer/deoptimizer.h index a39cc3579e..ff7ad1ef02 100644 --- a/src/deoptimizer/deoptimizer.h +++ b/src/deoptimizer/deoptimizer.h @@ -54,8 +54,12 @@ class Deoptimizer : public Malloced { Handle function() const; Handle compiled_code() const; DeoptimizeKind deopt_kind() const { return deopt_kind_; } - BytecodeOffset deopt_exit_bytecode_offset() const { - return deopt_exit_bytecode_offset_; + + // Where the deopt exit occurred *in the outermost frame*, i.e in the + // function we generated OSR'd code for. If the deopt occurred in an inlined + // function, this would point at the corresponding outermost Call bytecode. + BytecodeOffset bytecode_offset_in_outermost_frame() const { + return bytecode_offset_in_outermost_frame_; } static Deoptimizer* New(Address raw_function, DeoptimizeKind kind, @@ -183,7 +187,7 @@ class Deoptimizer : public Malloced { CodeTracer::Scope* verbose_trace_scope() const { return FLAG_trace_deopt_verbose ? trace_scope() : nullptr; } - void TraceDeoptBegin(int optimization_id); + void TraceDeoptBegin(int optimization_id, BytecodeOffset bytecode_offset); void TraceDeoptEnd(double deopt_duration); #ifdef DEBUG static void TraceFoundActivation(Isolate* isolate, JSFunction function); @@ -195,7 +199,7 @@ class Deoptimizer : public Malloced { JSFunction function_; Code compiled_code_; unsigned deopt_exit_index_; - BytecodeOffset deopt_exit_bytecode_offset_ = BytecodeOffset::None(); + BytecodeOffset bytecode_offset_in_outermost_frame_ = BytecodeOffset::None(); DeoptimizeKind deopt_kind_; Address from_; int fp_to_sp_delta_; diff --git a/src/interpreter/bytecode-array-iterator.cc b/src/interpreter/bytecode-array-iterator.cc index 9e99f9cc57..7732e78b0f 100644 --- a/src/interpreter/bytecode-array-iterator.cc +++ b/src/interpreter/bytecode-array-iterator.cc @@ -41,6 +41,16 @@ void BytecodeArrayIterator::SetOffset(int offset) { UpdateOperandScale(); } +// static +bool BytecodeArrayIterator::IsValidOffset(Handle bytecode_array, + int offset) { + for (BytecodeArrayIterator it(bytecode_array); !it.done(); it.Advance()) { + if (it.current_offset() == offset) return true; + if (it.current_offset() > offset) break; + } + return false; +} + void BytecodeArrayIterator::ApplyDebugBreak() { // Get the raw bytecode from the bytecode array. This may give us a // scaling prefix, which we can patch with the matching debug-break diff --git a/src/interpreter/bytecode-array-iterator.h b/src/interpreter/bytecode-array-iterator.h index 145f85b6ef..e1bebe1b5e 100644 --- a/src/interpreter/bytecode-array-iterator.h +++ b/src/interpreter/bytecode-array-iterator.h @@ -83,6 +83,9 @@ class V8_EXPORT_PRIVATE BytecodeArrayIterator { void SetOffset(int offset); void Reset() { SetOffset(0); } + // Whether the given offset is reachable in this bytecode array. + static bool IsValidOffset(Handle bytecode_array, int offset); + void ApplyDebugBreak(); inline Bytecode current_bytecode() const { diff --git a/src/runtime/runtime-compiler.cc b/src/runtime/runtime-compiler.cc index 623e1a95c8..3dc710b66b 100644 --- a/src/runtime/runtime-compiler.cc +++ b/src/runtime/runtime-compiler.cc @@ -188,9 +188,12 @@ bool DeoptExitIsInsideOsrLoop(Isolate* isolate, JSFunction function, DCHECK(!deopt_exit_offset.IsNone()); DCHECK(!osr_offset.IsNone()); - interpreter::BytecodeArrayIterator it( - handle(function.shared().GetBytecodeArray(isolate), isolate), - osr_offset.ToInt()); + Handle bytecode_array( + function.shared().GetBytecodeArray(isolate), isolate); + DCHECK(interpreter::BytecodeArrayIterator::IsValidOffset( + bytecode_array, deopt_exit_offset.ToInt())); + + interpreter::BytecodeArrayIterator it(bytecode_array, osr_offset.ToInt()); DCHECK_EQ(it.current_bytecode(), interpreter::Bytecode::kJumpLoop); for (; !it.done(); it.Advance()) { @@ -240,7 +243,7 @@ RUNTIME_FUNCTION(Runtime_NotifyDeoptimized) { // Make sure to materialize objects before causing any allocation. deoptimizer->MaterializeHeapObjects(); const BytecodeOffset deopt_exit_offset = - deoptimizer->deopt_exit_bytecode_offset(); + deoptimizer->bytecode_offset_in_outermost_frame(); delete deoptimizer; // Ensure the context register is updated for materialized objects. diff --git a/test/mjsunit/regress/regress-1320094.js b/test/mjsunit/regress/regress-1320094.js new file mode 100644 index 0000000000..e21df37d08 --- /dev/null +++ b/test/mjsunit/regress/regress-1320094.js @@ -0,0 +1,25 @@ +// Copyright 2022 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. +// +// Flags: --no-use-ic + +function getRandomProperty(v) { + var properties = Object.getOwnPropertyNames(v); + var proto = Object.v; + if (proto) { properties = properties.Object.getOwnPropertyNames(); } + if (properties.includes() && v.constructor.hasOwnProperty()) { properties = properties.Object.v.constructor.__proto__; } + if (properties.length == 0) { return "0"; } + return properties[rand % properties.length]; +} +var __v_12 = {}; +function __f_7() { + for (var __v_8 = 99; __v_8 < 100; __v_8++) { + for (var __v_10 = 0; __v_10 < 1000; __v_10++) { + var __v_13 = __v_12 + 3; + __v_13.__p_702586125 = __v_13[getRandomProperty( 702586125)]; + } + if (true) break; + } +} + __f_7();