[wasm][debug] Store "for debugging" flag on compilation unit

Before the "debug" flag was stored on the {CompilationEnv}. But each
background compilation task only gets the {CompilationEnv} once when
starting compilation, so by the time it picks up the "Liftoff for
debugging" compilation jobs, it might still compile them without the
debug flag being set. This leads to flakes in the "debug-step-into-wasm"
test, because we won't stop in the function prologue when stepping in
(because the function prologue does not check the "hook on function
call" flag if debug mode was not enabled).

This CL does not increase the size of a compilation unit, since both the
tier and the debug flag only need a single byte each.

As a nice side effect, this change allows us to remove the lock in
{CreateCompilationEnv}, because no modifyable flag is read any more.

R=thibaudm@chromium.org

Bug: v8:10410
Change-Id: Ic296ea0c4dd1d4dedde119f0536e87e5d301b5a1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2144116Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67115}

src/wasm/baseline/liftoff-compiler.cc

@@ -306,13 +306,14 @@ class LiftoffCompiler {
                   CompilationEnv* env, Zone* compilation_zone,
                   std::unique_ptr<AssemblerBuffer> buffer,
                   DebugSideTableBuilder* debug_sidetable_builder,
-                  Vector<int> breakpoints = {},
+                  ForDebugging for_debugging, Vector<int> breakpoints = {},
                   Vector<int> extra_source_pos = {})
       : asm_(std::move(buffer)),
         descriptor_(
             GetLoweredCallDescriptor(compilation_zone, call_descriptor)),
         env_(env),
         debug_sidetable_builder_(debug_sidetable_builder),
+        for_debugging_(for_debugging),
         compilation_zone_(compilation_zone),
         safepoint_table_builder_(compilation_zone_),
         next_breakpoint_ptr_(breakpoints.begin()),
@@ -610,7 +611,7 @@ class LiftoffCompiler {
 
     // If we are generating debug code, do check the "hook on function call"
     // flag. If set, trigger a break.
-    if (V8_UNLIKELY(env_->debug)) {
+    if (V8_UNLIKELY(for_debugging_)) {
       // If there is a breakpoint set on the first instruction (== start of the
       // function), then skip the check for "hook on function call", since we
       // will unconditionally break there anyway.
@@ -741,7 +742,7 @@ class LiftoffCompiler {
 
   void EmitBreakpoint(FullDecoder* decoder) {
     DEBUG_CODE_COMMENT("breakpoint");
-    DCHECK(env_->debug);
+    DCHECK(for_debugging_);
     source_position_table_builder_.AddPosition(
         __ pc_offset(), SourcePosition(decoder->position()), false);
     __ CallRuntimeStub(WasmCode::kWasmDebugBreak);
@@ -3208,6 +3209,7 @@ class LiftoffCompiler {
   compiler::CallDescriptor* const descriptor_;
   CompilationEnv* const env_;
   DebugSideTableBuilder* const debug_sidetable_builder_;
+  const ForDebugging for_debugging_;
   LiftoffBailoutReason bailout_reason_ = kSuccess;
   std::vector<OutOfLineCode> out_of_line_code_;
   SourcePositionTableBuilder source_position_table_builder_;
@@ -3256,8 +3258,8 @@ class LiftoffCompiler {
 
 WasmCompilationResult ExecuteLiftoffCompilation(
     AccountingAllocator* allocator, CompilationEnv* env,
-    const FunctionBody& func_body, int func_index, Counters* counters,
-    WasmFeatures* detected, Vector<int> breakpoints,
+    const FunctionBody& func_body, int func_index, ForDebugging for_debugging,
+    Counters* counters, WasmFeatures* detected, Vector<int> breakpoints,
     std::unique_ptr<DebugSideTable>* debug_sidetable,
     Vector<int> extra_source_pos) {
   int func_body_size = static_cast<int>(func_body.end - func_body.start);
@@ -3286,7 +3288,8 @@ WasmCompilationResult ExecuteLiftoffCompilation(
   WasmFullDecoder<Decoder::kValidate, LiftoffCompiler> decoder(
       &zone, env->module, env->enabled_features, detected, func_body,
       call_descriptor, env, &zone, instruction_buffer->CreateView(),
-      debug_sidetable_builder.get(), breakpoints, extra_source_pos);
+      debug_sidetable_builder.get(), for_debugging, breakpoints,
+      extra_source_pos);
   decoder.Decode();
   liftoff_compile_time_scope.reset();
   LiftoffCompiler* compiler = &decoder.interface();
@@ -3339,7 +3342,7 @@ std::unique_ptr<DebugSideTable> GenerateLiftoffDebugSideTable(
       &zone, env->module, env->enabled_features, &detected, func_body,
       call_descriptor, env, &zone,
       NewAssemblerBuffer(AssemblerBase::kDefaultBufferSize),
-      &debug_sidetable_builder);
+      &debug_sidetable_builder, kForDebugging);
   decoder.Decode();
   DCHECK(decoder.ok());
   DCHECK(!decoder.interface().did_bailout());

src/wasm/baseline/liftoff-compiler.h

@@ -54,8 +54,8 @@ enum LiftoffBailoutReason : int8_t {
 
 V8_EXPORT_PRIVATE WasmCompilationResult ExecuteLiftoffCompilation(
     AccountingAllocator*, CompilationEnv*, const FunctionBody&, int func_index,
-    Counters*, WasmFeatures* detected_features, Vector<int> breakpoints = {},
-    std::unique_ptr<DebugSideTable>* = nullptr,
+    ForDebugging, Counters*, WasmFeatures* detected_features,
+    Vector<int> breakpoints = {}, std::unique_ptr<DebugSideTable>* = nullptr,
     Vector<int> extra_source_pos = {});
 
 V8_EXPORT_PRIVATE std::unique_ptr<DebugSideTable> GenerateLiftoffDebugSideTable(

src/wasm/compilation-environment.h

@@ -60,15 +60,11 @@ struct CompilationEnv {
 
   const LowerSimd lower_simd;
 
-  // Whether the debugger is active.
-  const bool debug;
-
   constexpr CompilationEnv(const WasmModule* module,
                            UseTrapHandler use_trap_handler,
                            RuntimeExceptionSupport runtime_exception_support,
                            const WasmFeatures& enabled_features,
-                           LowerSimd lower_simd = kNoLowerSimd,
-                           bool debug = false)
+                           LowerSimd lower_simd = kNoLowerSimd)
       : module(module),
         use_trap_handler(use_trap_handler),
         runtime_exception_support(runtime_exception_support),
@@ -79,8 +75,7 @@ struct CompilationEnv {
                              : max_initial_mem_pages()) *
                         uint64_t{kWasmPageSize}),
         enabled_features(enabled_features),
-        lower_simd(lower_simd),
-        debug(debug) {}
+        lower_simd(lower_simd) {}
 };
 
 // The wire bytes are either owned by the StreamingDecoder, or (after streaming)

src/wasm/function-compiler.cc

@@ -188,9 +188,9 @@ WasmCompilationResult WasmCompilationUnit::ExecuteFunctionCompilation(
       if (V8_LIKELY(FLAG_wasm_tier_mask_for_testing == 0) ||
           func_index_ >= 32 ||
           ((FLAG_wasm_tier_mask_for_testing & (1 << func_index_)) == 0)) {
-        result =
-            ExecuteLiftoffCompilation(wasm_engine->allocator(), env, func_body,
-                                      func_index_, counters, detected);
+        result = ExecuteLiftoffCompilation(wasm_engine->allocator(), env,
+                                           func_body, func_index_,
+                                           for_debugging_, counters, detected);
         if (result.succeeded()) break;
       }
 
@@ -250,7 +250,7 @@ void WasmCompilationUnit::CompileWasmFunction(Isolate* isolate,
 
   DCHECK_LE(native_module->num_imported_functions(), function->func_index);
   DCHECK_LT(function->func_index, native_module->num_functions());
-  WasmCompilationUnit unit(function->func_index, tier);
+  WasmCompilationUnit unit(function->func_index, tier, kNoDebugging);
   CompilationEnv env = native_module->CreateCompilationEnv();
   WasmCompilationResult result = unit.ExecuteCompilation(
       isolate->wasm_engine(), &env,

src/wasm/function-compiler.h

@@ -78,8 +78,8 @@ class V8_EXPORT_PRIVATE WasmCompilationUnit final {
  public:
   static ExecutionTier GetBaselineExecutionTier(const WasmModule*);
 
-  WasmCompilationUnit(int index, ExecutionTier tier)
-      : func_index_(index), tier_(tier) {}
+  WasmCompilationUnit(int index, ExecutionTier tier, ForDebugging for_debugging)
+      : func_index_(index), tier_(tier), for_debugging_(for_debugging) {}
 
   WasmCompilationResult ExecuteCompilation(
       WasmEngine*, CompilationEnv*, const std::shared_ptr<WireBytesStorage>&,
@@ -103,6 +103,7 @@ class V8_EXPORT_PRIVATE WasmCompilationUnit final {
 
   int func_index_;
   ExecutionTier tier_;
+  ForDebugging for_debugging_;
 };
 
 // {WasmCompilationUnit} should be trivially copyable and small enough so we can

src/wasm/module-compiler.cc

@@ -739,15 +739,16 @@ class CompilationUnitBuilder {
 
   void AddUnits(uint32_t func_index) {
     if (func_index < native_module_->module()->num_imported_functions) {
-      baseline_units_.emplace_back(func_index, ExecutionTier::kNone);
+      baseline_units_.emplace_back(func_index, ExecutionTier::kNone,
+                                   kNoDebugging);
       return;
     }
     ExecutionTierPair tiers = GetRequestedExecutionTiers(
         native_module_->module(), compilation_state()->compile_mode(),
         native_module_->enabled_features(), func_index);
-    baseline_units_.emplace_back(func_index, tiers.baseline_tier);
+    baseline_units_.emplace_back(func_index, tiers.baseline_tier, kNoDebugging);
     if (tiers.baseline_tier != tiers.top_tier) {
-      tiering_units_.emplace_back(func_index, tiers.top_tier);
+      tiering_units_.emplace_back(func_index, tiers.top_tier, kNoDebugging);
     }
   }
 
@@ -770,12 +771,14 @@ class CompilationUnitBuilder {
               GetCompileStrategy(module, native_module_->enabled_features(),
                                  func_index, lazy_module));
 #endif
-    tiering_units_.emplace_back(func_index, tiers.top_tier);
+    tiering_units_.emplace_back(func_index, tiers.top_tier, kNoDebugging);
   }
 
   void AddRecompilationUnit(int func_index, ExecutionTier tier) {
     // For recompilation, just treat all units like baseline units.
-    baseline_units_.emplace_back(func_index, tier);
+    baseline_units_.emplace_back(
+        func_index, tier,
+        tier == ExecutionTier::kLiftoff ? kForDebugging : kNoDebugging);
   }
 
   bool Commit() {
@@ -898,7 +901,8 @@ bool CompileLazy(Isolate* isolate, NativeModule* native_module,
 
   DCHECK_LE(native_module->num_imported_functions(), func_index);
   DCHECK_LT(func_index, native_module->num_functions());
-  WasmCompilationUnit baseline_unit(func_index, tiers.baseline_tier);
+  WasmCompilationUnit baseline_unit{func_index, tiers.baseline_tier,
+                                    kNoDebugging};
   CompilationEnv env = native_module->CreateCompilationEnv();
   WasmCompilationResult result = baseline_unit.ExecuteCompilation(
       isolate->wasm_engine(), &env, compilation_state->GetWireBytesStorage(),
@@ -935,7 +939,7 @@ bool CompileLazy(Isolate* isolate, NativeModule* native_module,
   if (GetCompileStrategy(module, enabled_features, func_index, lazy_module) ==
           CompileStrategy::kLazy &&
       tiers.baseline_tier < tiers.top_tier) {
-    WasmCompilationUnit tiering_unit{func_index, tiers.top_tier};
+    WasmCompilationUnit tiering_unit{func_index, tiers.top_tier, kNoDebugging};
     compilation_state->AddTopTierCompilationUnit(tiering_unit);
   }
 

src/wasm/wasm-code-manager.cc

@@ -812,10 +812,8 @@ void NativeModule::LogWasmCodes(Isolate* isolate) {
 }
 
 CompilationEnv NativeModule::CreateCompilationEnv() const {
-  // Protect concurrent accesses to {tier_down_}.
-  base::MutexGuard guard(&allocation_mutex_);
   return {module(), use_trap_handler_, kRuntimeExceptionSupport,
-          enabled_features_, kNoLowerSimd,      tier_down_};
+          enabled_features_, kNoLowerSimd};
 }
 
 WasmCode* NativeModule::AddCodeForTesting(Handle<Code> code) {

src/wasm/wasm-debug.cc

@@ -669,8 +669,9 @@ class DebugInfoImpl {
         StackFramePositions(func_index, current_isolate);
 
     WasmCompilationResult result = ExecuteLiftoffCompilation(
-        native_module_->engine()->allocator(), &env, body, func_index, nullptr,
-        nullptr, offsets, &debug_sidetable, VectorOf(stack_frame_positions));
+        native_module_->engine()->allocator(), &env, body, func_index,
+        kForDebugging, nullptr, nullptr, offsets, &debug_sidetable,
+        VectorOf(stack_frame_positions));
     // Liftoff compilation failure is a FATAL error. We rely on complete Liftoff
     // support for debugging.
     if (!result.succeeded()) FATAL("Liftoff compilation failed");

src/wasm/wasm-tier.h

@@ -32,6 +32,8 @@ inline const char* ExecutionTierToString(ExecutionTier tier) {
   }
 }
 
+enum ForDebugging : bool { kForDebugging = true, kNoDebugging = false };
+
 }  // namespace wasm
 }  // namespace internal
 }  // namespace v8

test/cctest/wasm/test-liftoff-inspection.cc

@@ -39,16 +39,17 @@ class LiftoffCompileEnvironment {
     auto test_func = AddFunction(return_types, param_types, raw_function_bytes);
 
     // Now compile the function with Liftoff two times.
-    CompilationEnv env =
-        module_builder_.CreateCompilationEnv(TestingModuleBuilder::kDebug);
+    CompilationEnv env = module_builder_.CreateCompilationEnv();
     WasmFeatures detected1;
     WasmFeatures detected2;
-    WasmCompilationResult result1 = ExecuteLiftoffCompilation(
-        isolate_->allocator(), &env, test_func.body,
-        test_func.function->func_index, isolate_->counters(), &detected1);
-    WasmCompilationResult result2 = ExecuteLiftoffCompilation(
-        isolate_->allocator(), &env, test_func.body,
-        test_func.function->func_index, isolate_->counters(), &detected2);
+    WasmCompilationResult result1 =
+        ExecuteLiftoffCompilation(isolate_->allocator(), &env, test_func.body,
+                                  test_func.function->func_index, kNoDebugging,
+                                  isolate_->counters(), &detected1);
+    WasmCompilationResult result2 =
+        ExecuteLiftoffCompilation(isolate_->allocator(), &env, test_func.body,
+                                  test_func.function->func_index, kNoDebugging,
+                                  isolate_->counters(), &detected2);
 
     CHECK(result1.succeeded());
     CHECK(result2.succeeded());
@@ -69,13 +70,13 @@ class LiftoffCompileEnvironment {
       std::vector<int> breakpoints = {}) {
     auto test_func = AddFunction(return_types, param_types, raw_function_bytes);
 
-    CompilationEnv env =
-        module_builder_.CreateCompilationEnv(TestingModuleBuilder::kDebug);
+    CompilationEnv env = module_builder_.CreateCompilationEnv();
     WasmFeatures detected;
     std::unique_ptr<DebugSideTable> debug_side_table_via_compilation;
-    ExecuteLiftoffCompilation(
-        CcTest::i_isolate()->allocator(), &env, test_func.body, 0, nullptr,
-        &detected, VectorOf(breakpoints), &debug_side_table_via_compilation);
+    ExecuteLiftoffCompilation(CcTest::i_isolate()->allocator(), &env,
+                              test_func.body, 0, kForDebugging, nullptr,
+                              &detected, VectorOf(breakpoints),
+                              &debug_side_table_via_compilation);
 
     // If there are no breakpoint, then {ExecuteLiftoffCompilation} should
     // provide the same debug side table.

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

@@ -338,18 +338,14 @@ uint32_t TestingModuleBuilder::AddPassiveElementSegment(
   return index;
 }
 
-CompilationEnv TestingModuleBuilder::CreateCompilationEnv(
-    AssumeDebugging debug) {
+CompilationEnv TestingModuleBuilder::CreateCompilationEnv() {
   // This is a hack so we don't need to call
   // trap_handler::IsTrapHandlerEnabled().
   const bool is_trap_handler_enabled =
       V8_TRAP_HANDLER_SUPPORTED && i::FLAG_wasm_trap_handler;
   return {test_module_ptr_,
           is_trap_handler_enabled ? kUseTrapHandler : kNoTrapHandler,
-          runtime_exception_support_,
-          enabled_features_,
-          lower_simd(),
-          debug};
+          runtime_exception_support_, enabled_features_, lower_simd()};
 }
 
 const WasmGlobal* TestingModuleBuilder::AddGlobal(ValueType type) {
@@ -578,7 +574,8 @@ void WasmFunctionCompiler::Build(const byte* start, const byte* end) {
                          func_wire_bytes.begin(), func_wire_bytes.end()};
   NativeModule* native_module =
       builder_->instance_object()->module_object().native_module();
-  WasmCompilationUnit unit(function_->func_index, builder_->execution_tier());
+  WasmCompilationUnit unit(function_->func_index, builder_->execution_tier(),
+                           kNoDebugging);
   WasmFeatures unused_detected_features;
   WasmCompilationResult result = unit.ExecuteCompilation(
       isolate()->wasm_engine(), &env,

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

@@ -260,8 +260,7 @@ class TestingModuleBuilder {
 
   void TierDown() { native_module_->TierDown(isolate_); }
 
-  enum AssumeDebugging : bool { kDebug = true, kNoDebug = false };
-  CompilationEnv CreateCompilationEnv(AssumeDebugging = kNoDebug);
+  CompilationEnv CreateCompilationEnv();
 
   ExecutionTier execution_tier() const { return execution_tier_; }