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

This reverts commit 902f48bd.

Reason for revert: Made TSAN unhappy: https://ci.chromium.org/p/v8/builders/ci/V8%20Linux64%20TSAN%20-%20isolates/9480

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}

TBR=clemensb@chromium.org,thibaudm@chromium.org

Change-Id: Ibf650e8b6143471b44f2822c1737e7de5f8bdb20
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:10359
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2194372Reviewed-by: Shu-yu Guo <syg@chromium.org>
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67720}

src/wasm/wasm-code-manager.cc

@@ -1838,12 +1838,14 @@ std::vector<std::unique_ptr<WasmCode>> NativeModule::AddCompiledCode(
   return generated_code;
 }
 
-void NativeModule::SetTieredDown() {
+bool NativeModule::SetTieredDown() {
   // Do not tier down asm.js.
-  if (module()->origin != kWasmOrigin) return;
+  if (module()->origin != kWasmOrigin) return true;
 
   base::MutexGuard lock(&allocation_mutex_);
+  if (tiering_state_ == kTieredDown) return false;
   tiering_state_ = kTieredDown;
+  return true;
 }
 
 bool NativeModule::IsTieredDown() {
@@ -1855,7 +1857,9 @@ void NativeModule::TierDown() {
   // Do not tier down asm.js.
   if (module()->origin != kWasmOrigin) return;
 
-  SetTieredDown();
+  // Set the module to tiered down state; return if it is already in that state.
+  if (!SetTieredDown()) return;
+
   RecompileNativeModule(this, kTieredDown);
 }
 

src/wasm/wasm-code-manager.h

@@ -595,11 +595,9 @@ class V8_EXPORT_PRIVATE NativeModule final {
   V8_WARN_UNUSED_RESULT std::vector<std::unique_ptr<WasmCode>> AddCompiledCode(
       Vector<WasmCompilationResult>);
 
-  // Set the module to tiered down state. Triggering recompilation (if
-  // necessary) has to be done by the caller.
-  void SetTieredDown();
-
-  // Check whether this modules is tiered down for debugging.
+  // Set to tiered down state. Returns {true} if this caused a change, {false}
+  // otherwise.
+  bool SetTieredDown();
   bool IsTieredDown();
 
   // Set the flag to keep this module tiered down, trigger recompilation of all

src/wasm/wasm-engine.cc

@@ -631,20 +631,15 @@ void WasmEngine::TierDownAllModulesPerIsolate(Isolate* isolate) {
 }
 
 void WasmEngine::TierUpAllModulesPerIsolate(Isolate* isolate) {
-  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;
+  std::vector<NativeModule*> native_modules;
+  {
+    base::MutexGuard lock(&mutex_);
+    isolates_[isolate]->keep_tiered_down = false;
+    for (auto* native_module : isolates_[isolate]->native_modules) {
+      native_modules.push_back(native_module);
     }
-    return false;
-  };
-  for (auto* native_module : isolates_[isolate]->native_modules) {
-    // Only start tier-up if no other isolate needs this modules in tiered down
-    // state.
-    if (test_keep_tiered_down(native_module)) continue;
+  }
+  for (auto* native_module : native_modules) {
     native_module->StartTierUp();
   }
 }
@@ -1006,7 +1001,6 @@ 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 tier_down_module = false;
   if (native_module) {
     base::MutexGuard guard(&mutex_);
     auto& native_module_info = native_modules_[native_module.get()];
@@ -1015,10 +1009,7 @@ std::shared_ptr<NativeModule> WasmEngine::MaybeGetNativeModule(
     }
     native_module_info->isolates.insert(isolate);
     isolates_[isolate]->native_modules.insert(native_module.get());
-    tier_down_module = isolates_[isolate]->keep_tiered_down;
   }
-  // Potentially tier down after releasing the mutex.
-  if (tier_down_module) native_module->TierDown();
   return native_module;
 }
 
@@ -1034,17 +1025,11 @@ bool WasmEngine::UpdateNativeModuleCache(
 
   if (prev == native_module->get()) return true;
 
-  bool tier_down_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());
-    tier_down_module = isolates_[isolate]->keep_tiered_down;
-  }
-  // Potentially tier down after releasing the mutex.
-  if (tier_down_module) native_module->get()->TierDown();
+  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());
   return false;
 }
 

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

@@ -23,7 +23,7 @@ function checkTieredDown(instance) {
   }
 }
 
-function waitForTieredUp(instance) {
+function checkTieredUp(instance) {
   // Busy waiting until all functions are tiered up.
   let num_liftoff_functions = 0;
   while (true) {
@@ -37,8 +37,6 @@ function waitForTieredUp(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();
@@ -46,9 +44,8 @@ checkTieredDown(instance);
 const newInstance = create_builder(num_functions*2).instantiate();
 checkTieredDown(newInstance);
 Debug.disable();
-// Eventually the instances will be completely tiered up again.
-waitForTieredUp(instance);
-waitForTieredUp(newInstance);
+checkTieredUp(instance);
+checkTieredUp(newInstance);
 
 // Async.
 async function testTierDownToLiftoffAsync() {
@@ -58,8 +55,8 @@ async function testTierDownToLiftoffAsync() {
   const newAsyncInstance = await create_builder(num_functions*3).asyncInstantiate();
   checkTieredDown(newAsyncInstance);
   Debug.disable();
-  waitForTieredUp(asyncInstance);
-  waitForTieredUp(newAsyncInstance);
+  checkTieredUp(asyncInstance);
+  checkTieredUp(newAsyncInstance);
 }
 
 assertPromiseResult(testTierDownToLiftoffAsync());

test/debugger/debugger.status

@@ -152,6 +152,16 @@
   '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.