From 0cad8a53c8aa7d072419a42c3d26745386210ef9 Mon Sep 17 00:00:00 2001 From: Clemens Backes Date: Thu, 10 Dec 2020 14:01:36 +0100 Subject: [PATCH] [wasm] Move OperationsBarrier::Token to NativeModule The NativeModule should not die before the WasmEngine, since state owned by the engine will still be accessed in the destructor of the NativeModule. This CL ensures that by moving the OperationsBarrier from the CompilationStateImpl to the NativeModule. R=thibaudm@chromium.org, etiennep@chromium.org Bug: v8:11250, v8:11243 Change-Id: Ic4d69222e9e6076578c35986b0051817dbd8dbef Cq-Include-Trybots: luci.v8.try:v8_linux64_tsan_rel_ng Cq-Include-Trybots: luci.v8.try:v8_linux64_tsan_isolates_rel_ng Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2581959 Commit-Queue: Clemens Backes Reviewed-by: Thibaud Michaud Cr-Commit-Position: refs/heads/master@{#71696} --- src/wasm/module-compiler.cc | 10 +--------- src/wasm/wasm-code-manager.cc | 6 ++++-- src/wasm/wasm-code-manager.h | 12 +++++++++++- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/wasm/module-compiler.cc b/src/wasm/module-compiler.cc index 495cade0f6..4884e72fdb 100644 --- a/src/wasm/module-compiler.cc +++ b/src/wasm/module-compiler.cc @@ -20,7 +20,6 @@ #include "src/logging/counters.h" #include "src/logging/metrics.h" #include "src/objects/property-descriptor.h" -#include "src/tasks/operations-barrier.h" #include "src/tasks/task-utils.h" #include "src/tracing/trace-event.h" #include "src/trap-handler/trap-handler.h" @@ -658,8 +657,6 @@ class CompilationStateImpl { std::weak_ptr const native_module_weak_; const CompileMode compile_mode_; const std::shared_ptr async_counters_; - // Keeps engine alive as long as this is alive. - OperationsBarrier::Token engine_scope_; // Compilation error, atomically updated. This flag can be updated and read // using relaxed semantics. @@ -2751,12 +2748,7 @@ CompilationStateImpl::CompilationStateImpl( ? CompileMode::kTiering : CompileMode::kRegular), async_counters_(std::move(async_counters)), - engine_scope_(native_module_->engine() - ->GetBarrierForBackgroundCompile() - ->TryLock()), - compilation_unit_queues_(native_module->num_functions()) { - DCHECK(engine_scope_); -} + compilation_unit_queues_(native_module->num_functions()) {} void CompilationStateImpl::CancelCompilation() { // std::memory_order_relaxed is sufficient because no other state is diff --git a/src/wasm/wasm-code-manager.cc b/src/wasm/wasm-code-manager.cc index b262ccc247..c091abc828 100644 --- a/src/wasm/wasm-code-manager.cc +++ b/src/wasm/wasm-code-manager.cc @@ -801,15 +801,17 @@ NativeModule::NativeModule(WasmEngine* engine, const WasmFeatures& enabled, std::shared_ptr module, std::shared_ptr async_counters, std::shared_ptr* shared_this) - : code_allocator_(engine->code_manager(), std::move(code_space), + : engine_(engine), + engine_scope_(engine->GetBarrierForBackgroundCompile()->TryLock()), + code_allocator_(engine->code_manager(), std::move(code_space), async_counters), enabled_features_(enabled), module_(std::move(module)), import_wrapper_cache_(std::unique_ptr( new WasmImportWrapperCache())), - engine_(engine), use_trap_handler_(trap_handler::IsTrapHandlerEnabled() ? kUseTrapHandler : kNoTrapHandler) { + DCHECK(engine_scope_); // We receive a pointer to an empty {std::shared_ptr}, and install ourselve // there. DCHECK_NOT_NULL(shared_this); diff --git a/src/wasm/wasm-code-manager.h b/src/wasm/wasm-code-manager.h index dcc32d4fb4..464416d786 100644 --- a/src/wasm/wasm-code-manager.h +++ b/src/wasm/wasm-code-manager.h @@ -19,6 +19,7 @@ #include "src/base/optional.h" #include "src/builtins/builtins-definitions.h" #include "src/handles/handles.h" +#include "src/tasks/operations-barrier.h" #include "src/trap-handler/trap-handler.h" #include "src/utils/vector.h" #include "src/wasm/compilation-environment.h" @@ -718,6 +719,16 @@ class V8_EXPORT_PRIVATE NativeModule final { // Hold the {allocation_mutex_} when calling {PublishCodeLocked}. WasmCode* PublishCodeLocked(std::unique_ptr); + // -- Fields of {NativeModule} start here. + + WasmEngine* const engine_; + // Keep the engine alive as long as this NativeModule is alive. In its + // destructor, the NativeModule still communicates with the WasmCodeManager, + // owned by the engine. This fields comes before other fields which also still + // access the engine (like the code allocator), so that it's destructor runs + // last. + OperationsBarrier::Token engine_scope_; + // {WasmCodeAllocator} manages all code reservations and allocations for this // {NativeModule}. WasmCodeAllocator code_allocator_; @@ -790,7 +801,6 @@ class V8_EXPORT_PRIVATE NativeModule final { // End of fields protected by {allocation_mutex_}. ////////////////////////////////////////////////////////////////////////////// - WasmEngine* const engine_; int modification_scope_depth_ = 0; UseTrapHandler use_trap_handler_ = kNoTrapHandler; bool lazy_compile_frozen_ = false;