Reland "[wasm][debug] Fix tier down for multiple isolates"

This is a reland of 902f48bd, fixed
to avoid lock inversion problems detected by TSan.

Original change's description:
> [wasm][debug] Fix tier down for multiple isolates
>
> If multiple isolates are using the same module, we need to keep it
> tiered down as long as any isolate still has a debugger open.
> Also, we cannot short-cut the {NativeModule::TierDown} method, since the
> previously triggered tier down might not have finished yet.
> For now, each isolate starts an independent tier down (i.e. a full
> recompilation). We could optimize this later by skipping functions that
> are already tiered down, or are already scheduled for tier down, but we
> still need to wait for tier-down to finish on each isolate.
>
> R=thibaudm@chromium.org
>
> Bug: v8:10359
> Change-Id: I7ea6a6f5d3977e48718ac5bc94f9831541f6173f
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2190758
> Commit-Queue: Clemens Backes <clemensb@chromium.org>
> Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#67716}

Bug: v8:10359
Cq-Include-Trybots: luci.v8.try:v8_linux64_tsan_rel
Cq-Include-Trybots: luci.v8.try:v8_linux64_tsan_isolates_rel_ng
Change-Id: Ie98cf073fc79e5c6991df6d4466de7b560274070
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2194451
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67754}

src/runtime/runtime-test.cc

@@ -1383,7 +1383,8 @@ RUNTIME_FUNCTION(Runtime_WasmTierDownModule) {
   DCHECK_EQ(1, args.length());
   CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 0);
   auto* native_module = instance->module_object().native_module();
-  native_module->TierDown();
+  native_module->SetTieringState(wasm::kTieredDown);
+  native_module->TriggerRecompilation();
   CHECK(!native_module->compilation_state()->failed());
   return ReadOnlyRoots(isolate).undefined_value();
 }
@@ -1393,7 +1394,8 @@ RUNTIME_FUNCTION(Runtime_WasmTierUpModule) {
   DCHECK_EQ(1, args.length());
   CONVERT_ARG_HANDLE_CHECKED(WasmInstanceObject, instance, 0);
   auto* native_module = instance->module_object().native_module();
-  native_module->StartTierUp();
+  native_module->SetTieringState(wasm::kTieredUp);
+  native_module->TriggerRecompilation();
   CHECK(!native_module->compilation_state()->failed());
   return ReadOnlyRoots(isolate).undefined_value();
 }

src/wasm/wasm-code-manager.cc

@@ -1838,14 +1838,12 @@ std::vector<std::unique_ptr<WasmCode>> NativeModule::AddCompiledCode(
   return generated_code;
 }
 
-bool NativeModule::SetTieredDown() {
-  // Do not tier down asm.js.
-  if (module()->origin != kWasmOrigin) return true;
+void NativeModule::SetTieringState(TieringState new_tiering_state) {
+  // Do not tier down asm.js (just never change the tiering state).
+  if (module()->origin != kWasmOrigin) return;
 
   base::MutexGuard lock(&allocation_mutex_);
-  if (tiering_state_ == kTieredDown) return false;
-  tiering_state_ = kTieredDown;
-  return true;
+  tiering_state_ = new_tiering_state;
 }
 
 bool NativeModule::IsTieredDown() {
@@ -1853,27 +1851,16 @@ bool NativeModule::IsTieredDown() {
   return tiering_state_ == kTieredDown;
 }
 
-void NativeModule::TierDown() {
-  // Do not tier down asm.js.
-  if (module()->origin != kWasmOrigin) return;
-
-  // Set the module to tiered down state; return if it is already in that state.
-  if (!SetTieredDown()) return;
-
-  RecompileNativeModule(this, kTieredDown);
-}
-
-void NativeModule::StartTierUp() {
-  // Do not tier up asm.js.
-  if (module()->origin != kWasmOrigin) return;
-
+void NativeModule::TriggerRecompilation() {
+  // Read the tiering state under the lock, then trigger recompilation after
+  // releasing the lock. If the tiering state was changed when the triggered
+  // compilation units finish, code installation will handle that correctly.
+  TieringState current_state;
   {
     base::MutexGuard lock(&allocation_mutex_);
-    if (tiering_state_ == kTieredUp) return;
-    tiering_state_ = kTieredUp;
+    current_state = tiering_state_;
   }
-
-  RecompileNativeModule(this, kTieredUp);
+  RecompileNativeModule(this, current_state);
 }
 
 void NativeModule::FreeCode(Vector<WasmCode* const> codes) {

src/wasm/wasm-code-manager.h

@@ -595,18 +595,19 @@ class V8_EXPORT_PRIVATE NativeModule final {
   V8_WARN_UNUSED_RESULT std::vector<std::unique_ptr<WasmCode>> AddCompiledCode(
       Vector<WasmCompilationResult>);
 
-  // Set to tiered down state. Returns {true} if this caused a change, {false}
-  // otherwise.
-  bool SetTieredDown();
-  bool IsTieredDown();
+  // Set a new tiering state, but don't trigger any recompilation yet; use
+  // {TriggerRecompilation} for that. The two steps are split because In some
+  // scenarios we need to drop locks before triggering recompilation.
+  void SetTieringState(TieringState);
 
-  // Set the flag to keep this module tiered down, trigger recompilation of all
-  // functions, and wait for recompilation to complete.
-  void TierDown();
+  // Check whether this modules is tiered down for debugging.
+  bool IsTieredDown();
 
-  // Clear the flag to keep this module tiered down and trigger recompilation
-  // of all functions. Does not wait for completion of recompilation.
-  void StartTierUp();
+  // Trigger a full recompilation of this module, in the tier set previously via
+  // {SetTieringState}. When tiering down, the calling thread contributes to
+  // compilation and only returns once recompilation is done. Tiering up happens
+  // concurrently, so this method might return before it is complete.
+  void TriggerRecompilation();
 
   // Free a set of functions of this module. Uncommits whole pages if possible.
   // The given vector must be ordered by the instruction start address, and all

src/wasm/wasm-engine.cc

@@ -623,24 +623,40 @@ void WasmEngine::TierDownAllModulesPerIsolate(Isolate* isolate) {
     isolates_[isolate]->keep_tiered_down = true;
     for (auto* native_module : isolates_[isolate]->native_modules) {
       native_modules.push_back(native_module);
+      native_module->SetTieringState(kTieredDown);
     }
   }
   for (auto* native_module : native_modules) {
-    native_module->TierDown();
+    native_module->TriggerRecompilation();
   }
 }
 
 void WasmEngine::TierUpAllModulesPerIsolate(Isolate* isolate) {
-  std::vector<NativeModule*> native_modules;
+  // Only trigger recompilation after releasing the mutex, otherwise we risk
+  // deadlocks because of lock inversion.
+  std::vector<NativeModule*> native_modules_to_recompile;
   {
     base::MutexGuard lock(&mutex_);
     isolates_[isolate]->keep_tiered_down = false;
+    auto test_keep_tiered_down = [this](NativeModule* native_module) {
+      DCHECK_EQ(1, native_modules_.count(native_module));
+      for (auto* isolate : native_modules_[native_module]->isolates) {
+        DCHECK_EQ(1, isolates_.count(isolate));
+        if (isolates_[isolate]->keep_tiered_down) return true;
+      }
+      return false;
+    };
     for (auto* native_module : isolates_[isolate]->native_modules) {
-      native_modules.push_back(native_module);
+      if (!native_module->IsTieredDown()) continue;
+      // Only start tier-up if no other isolate needs this modules in tiered
+      // down state.
+      if (test_keep_tiered_down(native_module)) continue;
+      native_module->SetTieringState(kTieredUp);
+      native_modules_to_recompile.push_back(native_module);
     }
   }
-  for (auto* native_module : native_modules) {
-    native_module->StartTierUp();
+  for (auto* native_module : native_modules_to_recompile) {
+    native_module->TriggerRecompilation();
   }
 }
 
@@ -988,7 +1004,7 @@ std::shared_ptr<NativeModule> WasmEngine::NewNativeModule(
   auto& modules_per_isolate = isolates_[isolate]->native_modules;
   modules_per_isolate.insert(native_module.get());
   if (isolates_[isolate]->keep_tiered_down) {
-    native_module->SetTieredDown();
+    native_module->SetTieringState(kTieredDown);
   }
   isolate->counters()->wasm_modules_per_isolate()->AddSample(
       static_cast<int>(modules_per_isolate.size()));
@@ -1001,6 +1017,7 @@ std::shared_ptr<NativeModule> WasmEngine::MaybeGetNativeModule(
     ModuleOrigin origin, Vector<const uint8_t> wire_bytes, Isolate* isolate) {
   std::shared_ptr<NativeModule> native_module =
       native_module_cache_.MaybeGetNativeModule(origin, wire_bytes);
+  bool recompile_module = false;
   if (native_module) {
     base::MutexGuard guard(&mutex_);
     auto& native_module_info = native_modules_[native_module.get()];
@@ -1009,7 +1026,13 @@ std::shared_ptr<NativeModule> WasmEngine::MaybeGetNativeModule(
     }
     native_module_info->isolates.insert(isolate);
     isolates_[isolate]->native_modules.insert(native_module.get());
+    if (isolates_[isolate]->keep_tiered_down) {
+      native_module->SetTieringState(kTieredDown);
+      recompile_module = true;
     }
+  }
+  // Potentially recompile the module for tier down, after releasing the mutex.
+  if (recompile_module) native_module->TriggerRecompilation();
   return native_module;
 }
 
@@ -1025,11 +1048,20 @@ bool WasmEngine::UpdateNativeModuleCache(
 
   if (prev == native_module->get()) return true;
 
+  bool recompile_module = false;
+  {
     base::MutexGuard guard(&mutex_);
     DCHECK_EQ(1, native_modules_.count(native_module->get()));
     native_modules_[native_module->get()]->isolates.insert(isolate);
     DCHECK_EQ(1, isolates_.count(isolate));
     isolates_[isolate]->native_modules.insert(native_module->get());
+    if (isolates_[isolate]->keep_tiered_down) {
+      native_module->get()->SetTieringState(kTieredDown);
+      recompile_module = true;
+    }
+  }
+  // Potentially recompile the module for tier down, after releasing the mutex.
+  if (recompile_module) native_module->get()->TriggerRecompilation();
   return false;
 }
 

test/cctest/wasm/wasm-run-utils.h

@@ -221,7 +221,8 @@ class TestingModuleBuilder {
   void SetExecutable() { native_module_->SetExecutable(true); }
 
   void TierDown() {
-    native_module_->TierDown();
+    native_module_->SetTieringState(kTieredDown);
+    native_module_->TriggerRecompilation();
     execution_tier_ = ExecutionTier::kLiftoff;
   }
 

test/debugger/debug/wasm/debug-enabled-tier-down-wasm.js

@@ -23,7 +23,7 @@ function checkTieredDown(instance) {
   }
 }
 
-function checkTieredUp(instance) {
+function waitForTieredUp(instance) {
   // Busy waiting until all functions are tiered up.
   let num_liftoff_functions = 0;
   while (true) {
@@ -37,6 +37,8 @@ function checkTieredUp(instance) {
   }
 }
 
+// In the 'isolates' test, this test runs in parallel to itself on two isolates.
+// All checks below should still hold.
 const instance = create_builder().instantiate();
 const Debug = new DebugWrapper();
 Debug.enable();
@@ -44,8 +46,9 @@ checkTieredDown(instance);
 const newInstance = create_builder(num_functions*2).instantiate();
 checkTieredDown(newInstance);
 Debug.disable();
-checkTieredUp(instance);
-checkTieredUp(newInstance);
+// Eventually the instances will be completely tiered up again.
+waitForTieredUp(instance);
+waitForTieredUp(newInstance);
 
 // Async.
 async function testTierDownToLiftoffAsync() {
@@ -55,8 +58,8 @@ async function testTierDownToLiftoffAsync() {
   const newAsyncInstance = await create_builder(num_functions*3).asyncInstantiate();
   checkTieredDown(newAsyncInstance);
   Debug.disable();
-  checkTieredUp(asyncInstance);
-  checkTieredUp(newAsyncInstance);
+  waitForTieredUp(asyncInstance);
+  waitForTieredUp(newAsyncInstance);
 }
 
 assertPromiseResult(testTierDownToLiftoffAsync());

test/debugger/debugger.status

@@ -152,16 +152,6 @@
   'debug/wasm/frame-inspection': [SKIP],
 }],
 
-##############################################################################
-['isolates', {
-  # WebAssembly debugging does not work reliably when multiple isolates are
-  # involved (https://crbug.com/v8/10359).
-  # (this list might need to be extended by more debugging tests as they
-  # start flaking)
-  'debug/wasm/*': [SKIP],
-  'regress/regress-crbug-1032042': [SKIP],
-}],  # 'isolates'
-
 ################################################################################
 ['variant == stress_snapshot', {
   '*': [SKIP],  # only relevant for mjsunit tests.