diff --git a/common.gypi b/common.gypi index 1effebe59bd4..46ae0a4d4b93 100644 --- a/common.gypi +++ b/common.gypi @@ -42,7 +42,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.53', + 'v8_embedder_string': '-node.55', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/src/wasm/wasm-code-manager.cc b/deps/v8/src/wasm/wasm-code-manager.cc index 489a30d7aed5..249408d5c170 100644 --- a/deps/v8/src/wasm/wasm-code-manager.cc +++ b/deps/v8/src/wasm/wasm-code-manager.cc @@ -2776,6 +2776,10 @@ void NativeModule::FreeCode(base::Vector codes) { // Free the {WasmCode} objects. This will also unregister trap handler data. for (WasmCode* code : codes) { DCHECK_EQ(1, owned_code_.count(code->instruction_start())); + // TODO(407003348): Drop this check if it doesn't trigger in the wild. + CHECK_EQ(WasmCode::refcount( + code->ref_count_bitfield_.load(std::memory_order_acquire)), + 0); owned_code_.erase(code->instruction_start()); } // Remove debug side tables for all removed code objects, after releasing our @@ -2995,6 +2999,16 @@ void WasmCodeRefScope::AddRef(WasmCode* code) { code->IncRef(); } +// static +WasmCode* WasmCodeRefScope::AddRefIfNotDying(WasmCode* code) { + DCHECK_NOT_NULL(code); + WasmCodeRefScope* current_scope = current_code_refs_scope; + DCHECK_NOT_NULL(current_scope); + if (!code->IncRefIfNotDying()) return nullptr; + current_scope->code_ptrs_.push_back(code); + return code; +} + void WasmCodeLookupCache::Flush() { for (int i = 0; i < kWasmCodeLookupCacheSize; i++) cache_[i].pc.store(kNullAddress, std::memory_order_release); diff --git a/deps/v8/src/wasm/wasm-code-manager.h b/deps/v8/src/wasm/wasm-code-manager.h index 49258f516d22..38a3cc28f4e1 100644 --- a/deps/v8/src/wasm/wasm-code-manager.h +++ b/deps/v8/src/wasm/wasm-code-manager.h @@ -261,40 +261,51 @@ class V8_EXPORT_PRIVATE WasmCode final { ~WasmCode(); void IncRef() { - [[maybe_unused]] int old_val = - ref_count_.fetch_add(1, std::memory_order_acq_rel); - DCHECK_LE(1, old_val); - DCHECK_GT(kMaxInt, old_val); + [[maybe_unused]] uint32_t old_field = + ref_count_bitfield_.fetch_add(1, std::memory_order_acq_rel); + DCHECK_LE(1, refcount(old_field)); + DCHECK_GT(kMaxInt, refcount(old_field)); + } + + // Returns true if the refcount was incremented, false if {this->is_dying()}. + bool IncRefIfNotDying() { + uint32_t old_field = ref_count_bitfield_.load(std::memory_order_acquire); + while (true) { + if (is_dying(old_field)) return false; + if (ref_count_bitfield_.compare_exchange_weak( + old_field, old_field + 1, std::memory_order_acq_rel)) { + return true; + } + } } // Decrement the ref count. Returns whether this code becomes dead and needs // to be freed. V8_WARN_UNUSED_RESULT bool DecRef() { - int old_count = ref_count_.load(std::memory_order_acquire); + uint32_t old_field = ref_count_bitfield_.load(std::memory_order_acquire); while (true) { - DCHECK_LE(1, old_count); - if (V8_UNLIKELY(old_count == 1)) { - if (is_dying()) { + DCHECK_LE(1, refcount(old_field)); + if (V8_UNLIKELY(refcount(old_field) == 1)) { + if (is_dying(old_field)) { // The code was already on the path to deletion, only temporary // C++ references to it are left. Decrement the refcount, and // return true if it drops to zero. return DecRefOnDeadCode(); } // Otherwise, the code enters the path to destruction now. - mark_as_dying(); - old_count = ref_count_.load(std::memory_order_acquire); - if (V8_LIKELY(old_count == 1)) { + if (ref_count_bitfield_.compare_exchange_weak( + old_field, old_field | kIsDyingMask, + std::memory_order_acq_rel)) { // No other thread got in the way. Commit to the decision. DecRefOnPotentiallyDeadCode(); return false; } - // Another thread managed to increment the refcount again, just - // before we set the "dying" bit. So undo that, and resume the - // loop to evaluate again what needs to be done. - undo_mark_as_dying(); + // Another thread interfered. Re-evaluate what to do. + continue; } - if (ref_count_.compare_exchange_weak(old_count, old_count - 1, - std::memory_order_acq_rel)) { + DCHECK_LT(1, refcount(old_field)); + if (ref_count_bitfield_.compare_exchange_weak( + old_field, old_field - 1, std::memory_order_acq_rel)) { return false; } } @@ -303,16 +314,18 @@ class V8_EXPORT_PRIVATE WasmCode final { // Decrement the ref count on code that is known to be in use (i.e. the ref // count cannot drop to zero here). void DecRefOnLiveCode() { - [[maybe_unused]] int old_count = - ref_count_.fetch_sub(1, std::memory_order_acq_rel); - DCHECK_LE(2, old_count); + [[maybe_unused]] uint32_t old_bitfield_value = + ref_count_bitfield_.fetch_sub(1, std::memory_order_acq_rel); + DCHECK_LE(2, refcount(old_bitfield_value)); } // Decrement the ref count on code that is known to be dead, even though there // might still be C++ references. Returns whether this drops the last // reference and the code needs to be freed. V8_WARN_UNUSED_RESULT bool DecRefOnDeadCode() { - return ref_count_.fetch_sub(1, std::memory_order_acq_rel) == 1; + uint32_t old_bitfield_value = + ref_count_bitfield_.fetch_sub(1, std::memory_order_acq_rel); + return refcount(old_bitfield_value) == 1; } // Decrement the ref count on a set of {WasmCode} objects, potentially @@ -321,9 +334,9 @@ class V8_EXPORT_PRIVATE WasmCode final { // Called by the WasmEngine when it shuts down for code it thinks is // probably dead (i.e. is in the "potentially_dead_code_" set). Wrapped - // in a method only because {ref_count_} is private. + // in a method only because {ref_count_bitfield_} is private. void DcheckRefCountIsOne() { - DCHECK_EQ(1, ref_count_.load(std::memory_order_acquire)); + DCHECK_EQ(1, refcount(ref_count_bitfield_.load(std::memory_order_acquire))); } // Returns the last source position before {offset}. @@ -340,7 +353,15 @@ class V8_EXPORT_PRIVATE WasmCode final { return ForDebuggingField::decode(flags_); } - bool is_dying() const { return dying_.load(std::memory_order_acquire); } + bool is_dying() const { + return is_dying(ref_count_bitfield_.load(std::memory_order_acquire)); + } + static bool is_dying(uint32_t bit_field_value) { + return (bit_field_value & kIsDyingMask) != 0; + } + static uint32_t refcount(uint32_t bit_field_value) { + return bit_field_value & ~kIsDyingMask; + } // Returns {true} for Liftoff code that sets up a feedback vector slot in its // stack frame. @@ -427,11 +448,6 @@ class V8_EXPORT_PRIVATE WasmCode final { // for consideration in the next Code GC cycle. V8_NOINLINE void DecRefOnPotentiallyDeadCode(); - void mark_as_dying() { dying_.store(true, std::memory_order_release); } - // This is rarely necessary to mitigate a race condition. See the comment - // at its (only) call site. - void undo_mark_as_dying() { dying_.store(false, std::memory_order_release); } - NativeModule* const native_module_ = nullptr; uint8_t* const instructions_; const uint64_t signature_hash_; @@ -476,10 +492,6 @@ class V8_EXPORT_PRIVATE WasmCode final { using ForDebuggingField = ExecutionTierField::Next; using FrameHasFeedbackSlotField = ForDebuggingField::Next; - // Will be set to {true} the first time this code object is considered - // "potentially dead" (to be confirmed by the next Wasm Code GC cycle). - std::atomic dying_{false}; - // WasmCode is ref counted. Counters are held by: // 1) The jump table / code table. // 2) {WasmCodeRefScope}s. @@ -490,7 +502,11 @@ class V8_EXPORT_PRIVATE WasmCode final { // it's being used. Once the ref count drops to zero (i.e. after being removed // from (3) and all (2)), the code object is deleted and the memory for the // machine code is freed. - std::atomic ref_count_{1}; + // The topmost bit is used to indicate that the code is in (3). It is stored + // in this same field to avoid race conditions between atomic updates to + // that state and the refcount. + static constexpr uint32_t kIsDyingMask = 0x8000'0000u; + std::atomic ref_count_bitfield_{1}; }; WasmCode::Kind GetCodeKind(const WasmCompilationResult& result); @@ -1237,6 +1253,10 @@ class V8_EXPORT_PRIVATE V8_NODISCARD WasmCodeRefScope { // Register a {WasmCode} reference in the current {WasmCodeRefScope}. Fails if // there is no current scope. static void AddRef(WasmCode*); + // Same, but conditional: + // - if the {code} is marked as dying, do nothing, return nullptr. + // - otherwise add a ref and return {code}. + static WasmCode* AddRefIfNotDying(WasmCode* code); private: WasmCodeRefScope* const previous_scope_; diff --git a/deps/v8/src/wasm/wasm-engine.cc b/deps/v8/src/wasm/wasm-engine.cc index 7724979eb01b..f7a0e392a59d 100644 --- a/deps/v8/src/wasm/wasm-engine.cc +++ b/deps/v8/src/wasm/wasm-engine.cc @@ -1890,17 +1890,11 @@ void WasmEngine::FreeDeadCodeLocked(const DeadCodeMap& dead_code, const std::vector& code_vec = dead_code_entry.second; TRACE_CODE_GC("Freeing %zu code object%s of module %p.\n", code_vec.size(), code_vec.size() == 1 ? "" : "s", native_module); -#if DEBUG - for (WasmCode* code : code_vec) DCHECK(code->is_dying()); -#endif // DEBUG native_module->FreeCode(base::VectorOf(code_vec)); } if (dead_wrappers.size()) { TRACE_CODE_GC("Freeing %zu wrapper%s.\n", dead_wrappers.size(), dead_wrappers.size() == 1 ? "" : "s"); -#if DEBUG - for (WasmCode* code : dead_wrappers) DCHECK(code->is_dying()); -#endif // DEBUG GetWasmImportWrapperCache()->Free(dead_wrappers); } } diff --git a/deps/v8/src/wasm/wasm-import-wrapper-cache.cc b/deps/v8/src/wasm/wasm-import-wrapper-cache.cc index 447195d1a616..8d6c439380f2 100644 --- a/deps/v8/src/wasm/wasm-import-wrapper-cache.cc +++ b/deps/v8/src/wasm/wasm-import-wrapper-cache.cc @@ -146,9 +146,7 @@ WasmCode* WasmImportWrapperCache::FindWrapper(WasmCodePointer call_target) { GetProcessWideWasmCodePointerTable()->GetEntrypointWithoutSignatureCheck( call_target)); if (iter == codes_.end()) return nullptr; - WasmCodeRefScope::AddRef(iter->second); - if (iter->second->is_dying()) return nullptr; - return iter->second; + return WasmCodeRefScope::AddRefIfNotDying(iter->second); } WasmCode* WasmImportWrapperCache::CompileWasmImportCallWrapper( @@ -165,8 +163,8 @@ WasmCode* WasmImportWrapperCache::CompileWasmImportCallWrapper( // again whether another thread has just created the wrapper. wasm_code = cache_scope[key]; if (wasm_code) { - WasmCodeRefScope::AddRef(wasm_code); - if (!wasm_code->is_dying()) return wasm_code; + wasm_code = WasmCodeRefScope::AddRefIfNotDying(wasm_code); + if (wasm_code) return wasm_code; } wasm_code = cache_scope.AddWrapper(key, std::move(result), @@ -218,6 +216,9 @@ void WasmImportWrapperCache::Free(std::vector& wrappers) { } code_allocator_->FreeCode(base::VectorOf(wrappers)); for (WasmCode* wrapper : wrappers) { + // TODO(407003348): Drop this check if it doesn't trigger in the wild. + CHECK_EQ(wrapper->ref_count_bitfield_.load(std::memory_order_acquire), + WasmCode::kIsDyingMask); delete wrapper; } // Make sure nobody tries to access stale pointers. @@ -232,9 +233,7 @@ WasmCode* WasmImportWrapperCache::MaybeGet(ImportCallKind kind, auto it = entry_map_.find({kind, type_index, expected_arity, suspend}); if (it == entry_map_.end()) return nullptr; - WasmCodeRefScope::AddRef(it->second); - if (it->second->is_dying()) return nullptr; - return it->second; + return WasmCodeRefScope::AddRefIfNotDying(it->second); } WasmCode* WasmImportWrapperCache::Lookup(Address pc) const {