Revert "[wasm] Store compile errors in CompilationState"

This reverts commit bf3d7b9a.

Reason for revert: Breaks TSAN build, see
https://ci.chromium.org/p/v8/builders/luci.v8.ci/V8%20Linux64%20TSAN/23248

Original change's description:
> [wasm] Store compile errors in CompilationState
> 
> We are currently storing compilation errors in the individual
> compilation units and pass it to the ErrorThrower during finishing.
> This CL changes that to store errors on the CompilationState directly.
> From there, it is propagated to the ErrorThrower in the compilation
> state callback.
> This removes more work from the finisher task and slims down the
> WasmCompilationUnits.
> 
> R=​mstarzinger@chromium.org
> 
> Bug: v8:8343, v8:7921
> Change-Id: Id332add43d4219d2a30fee653ed4e53a9b2698d9
> Reviewed-on: https://chromium-review.googlesource.com/c/1303720
> Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
> Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#57091}

TBR=mstarzinger@chromium.org,clemensh@chromium.org

Change-Id: Id32c7337494a4749485adbcfcaae7b2331afea66
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:8343, v8:7921
Reviewed-on: https://chromium-review.googlesource.com/c/1304544Reviewed-by: Maya Lekova <mslekova@chromium.org>
Commit-Queue: Maya Lekova <mslekova@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57094}

src/compiler/wasm-compiler.cc

@@ -5200,18 +5200,19 @@ TurbofanWasmCompilationUnit::TurbofanWasmCompilationUnit(
 // Clears unique_ptrs, but (part of) the type is forward declared in the header.
 TurbofanWasmCompilationUnit::~TurbofanWasmCompilationUnit() = default;
 
-bool TurbofanWasmCompilationUnit::BuildGraphForWasmFunction(
+SourcePositionTable* TurbofanWasmCompilationUnit::BuildGraphForWasmFunction(
     wasm::CompilationEnv* env, wasm::WasmFeatures* detected, double* decode_ms,
-    MachineGraph* mcgraph, NodeOriginTable* node_origins,
-    SourcePositionTable* source_positions) {
+    MachineGraph* mcgraph, NodeOriginTable* node_origins) {
   base::ElapsedTimer decode_timer;
   if (FLAG_trace_wasm_decode_time) {
     decode_timer.Start();
   }
 
   // Create a TF graph during decoding.
+  SourcePositionTable* source_position_table =
+      new (mcgraph->zone()) SourcePositionTable(mcgraph->graph());
   WasmGraphBuilder builder(env, mcgraph->zone(), mcgraph,
-                           wasm_unit_->func_body_.sig, source_positions);
+                           wasm_unit_->func_body_.sig, source_position_table);
   wasm::VoidResult graph_construction_result = wasm::BuildTFGraph(
       wasm_unit_->wasm_engine_->allocator(),
       wasm_unit_->native_module_->enabled_features(), env->module, &builder,
@@ -5221,9 +5222,9 @@ bool TurbofanWasmCompilationUnit::BuildGraphForWasmFunction(
       StdoutStream{} << "Compilation failed: "
                      << graph_construction_result.error_msg() << std::endl;
     }
-    wasm_unit_->native_module()->compilation_state()->SetError(
-        wasm_unit_->func_index_, std::move(graph_construction_result));
-    return false;
+    wasm_unit_->result_ = wasm::Result<wasm::WasmCode*>::ErrorFrom(
+        std::move(graph_construction_result));
+    return nullptr;
   }
 
   builder.LowerInt64();
@@ -5244,7 +5245,7 @@ bool TurbofanWasmCompilationUnit::BuildGraphForWasmFunction(
   if (FLAG_trace_wasm_decode_time) {
     *decode_ms = decode_timer.Elapsed().InMillisecondsF();
   }
-  return true;
+  return source_position_table;
 }
 
 namespace {
@@ -5293,13 +5294,10 @@ void TurbofanWasmCompilationUnit::ExecuteCompilation(
                                       ? new (&zone)
                                             NodeOriginTable(mcgraph->graph())
                                       : nullptr;
-  SourcePositionTable* source_positions =
-      new (mcgraph->zone()) SourcePositionTable(mcgraph->graph());
-  if (!BuildGraphForWasmFunction(env, detected, &decode_ms, mcgraph,
-                                 node_origins, source_positions)) {
-    // Compilation failed.
-    return;
-  }
+  SourcePositionTable* source_positions = BuildGraphForWasmFunction(
+      env, detected, &decode_ms, mcgraph, node_origins);
+
+  if (wasm_unit_->failed()) return;
 
   if (node_origins) {
     node_origins->AddDecorator();

src/compiler/wasm-compiler.h

@@ -50,11 +50,11 @@ class TurbofanWasmCompilationUnit {
   explicit TurbofanWasmCompilationUnit(wasm::WasmCompilationUnit* wasm_unit);
   ~TurbofanWasmCompilationUnit();
 
-  bool BuildGraphForWasmFunction(wasm::CompilationEnv* env,
-                                 wasm::WasmFeatures* detected,
-                                 double* decode_ms, MachineGraph* mcgraph,
-                                 NodeOriginTable* node_origins,
-                                 SourcePositionTable* source_positions);
+  SourcePositionTable* BuildGraphForWasmFunction(wasm::CompilationEnv* env,
+                                                 wasm::WasmFeatures* detected,
+                                                 double* decode_ms,
+                                                 MachineGraph* mcgraph,
+                                                 NodeOriginTable* node_origins);
 
   void ExecuteCompilation(wasm::CompilationEnv*, Counters*,
                           wasm::WasmFeatures* detected);

src/wasm/compilation-environment.h

@@ -13,7 +13,6 @@ namespace internal {
 namespace wasm {
 
 class NativeModule;
-class ResultBase;
 
 enum RuntimeExceptionSupport : bool {
   kRuntimeExceptionSupport = true,
@@ -72,8 +71,6 @@ class CompilationState {
 
   void CancelAndWait();
 
-  void SetError(uint32_t func_index, const ResultBase& error_result);
-
  private:
   friend class NativeModule;
   CompilationState() = delete;

src/wasm/function-compiler.cc

@@ -93,6 +93,25 @@ void WasmCompilationUnit::ExecuteCompilation(CompilationEnv* env,
   }
 }
 
+void WasmCompilationUnit::ReportError(ErrorThrower* thrower) const {
+  DCHECK(result_.failed());
+  // Add the function as another context for the exception. This is
+  // user-visible, so use official format.
+  EmbeddedVector<char, 128> message;
+  wasm::ModuleWireBytes wire_bytes(native_module()->wire_bytes());
+  wasm::WireBytesRef name_ref =
+      native_module()->module()->LookupFunctionName(wire_bytes, func_index_);
+  if (name_ref.is_set()) {
+    wasm::WasmName name = wire_bytes.GetNameOrNull(name_ref);
+    SNPrintF(message, "Compiling wasm function \"%.*s\" failed", name.length(),
+             name.start());
+  } else {
+    SNPrintF(message, "Compiling wasm function \"wasm-function[%d]\" failed",
+             func_index_);
+  }
+  thrower->CompileFailed(message.start(), result_);
+}
+
 void WasmCompilationUnit::SwitchMode(ExecutionTier new_mode) {
   // This method is being called in the constructor, where neither
   // {liftoff_unit_} nor {turbofan_unit_} are set, or to switch mode from
@@ -116,11 +135,9 @@ void WasmCompilationUnit::SwitchMode(ExecutionTier new_mode) {
 }
 
 // static
-bool WasmCompilationUnit::CompileWasmFunction(Isolate* isolate,
-                                              NativeModule* native_module,
-                                              WasmFeatures* detected,
-                                              const WasmFunction* function,
-                                              ExecutionTier mode) {
+bool WasmCompilationUnit::CompileWasmFunction(
+    Isolate* isolate, NativeModule* native_module, WasmFeatures* detected,
+    ErrorThrower* thrower, const WasmFunction* function, ExecutionTier mode) {
   ModuleWireBytes wire_bytes(native_module->wire_bytes());
   FunctionBody function_body{function->sig, function->code.offset(),
                              wire_bytes.start() + function->code.offset(),
@@ -130,12 +147,17 @@ bool WasmCompilationUnit::CompileWasmFunction(Isolate* isolate,
                            function->func_index, mode);
   CompilationEnv env = native_module->CreateCompilationEnv();
   unit.ExecuteCompilation(&env, isolate->counters(), detected);
-  return !unit.failed();
+  if (unit.failed()) {
+    unit.ReportError(thrower);
+    return false;
+  }
+  return true;
 }
 
 void WasmCompilationUnit::SetResult(WasmCode* code, Counters* counters) {
-  DCHECK_NULL(result_);
-  result_ = code;
+  DCHECK(!result_.failed());
+  DCHECK_NULL(result_.value());
+  result_ = Result<WasmCode*>(code);
   native_module()->PublishCode(code);
 
   counters->wasm_generated_code_size()->Increment(

src/wasm/function-compiler.h

@@ -46,11 +46,17 @@ class WasmCompilationUnit final {
 
   NativeModule* native_module() const { return native_module_; }
   ExecutionTier mode() const { return mode_; }
-  bool failed() const { return result_ == nullptr; }  // TODO(clemensh): Remove.
-  WasmCode* result() const { return result_; }
+  bool failed() const { return result_.failed(); }
+  WasmCode* result() const {
+    DCHECK(!failed());
+    DCHECK_NOT_NULL(result_.value());
+    return result_.value();
+  }
+
+  void ReportError(ErrorThrower* thrower) const;
 
   static bool CompileWasmFunction(Isolate* isolate, NativeModule* native_module,
-                                  WasmFeatures* detected,
+                                  WasmFeatures* detected, ErrorThrower* thrower,
                                   const WasmFunction* function,
                                   ExecutionTier = GetDefaultExecutionTier());
 
@@ -63,7 +69,7 @@ class WasmCompilationUnit final {
   int func_index_;
   NativeModule* native_module_;
   ExecutionTier mode_;
-  WasmCode* result_ = nullptr;
+  wasm::Result<WasmCode*> result_;
 
   // LiftoffCompilationUnit, set if {mode_ == kLiftoff}.
   std::unique_ptr<LiftoffCompilationUnit> liftoff_unit_;

src/wasm/module-compiler.cc

@@ -83,7 +83,7 @@ class CompilationStateImpl {
   // Set the callback function to be called on compilation events. Needs to be
   // set before {AddCompilationUnits} is run.
   void SetCallback(
-      std::function<void(CompilationEvent, const VoidResult*)> callback);
+      std::function<void(CompilationEvent, ErrorThrower*)> callback);
 
   // Inserts new functions to compile and kicks off compilation.
   void AddCompilationUnits(
@@ -94,7 +94,7 @@ class CompilationStateImpl {
 
   bool HasCompilationUnitToFinish();
 
-  void OnError(const VoidResult& error_result);
+  void OnError(ErrorThrower* thrower);
   void OnFinishedUnit();
   void ScheduleUnitForFinishing(std::unique_ptr<WasmCompilationUnit> unit,
                                 ExecutionTier mode);
@@ -109,8 +109,6 @@ class CompilationStateImpl {
 
   void Abort();
 
-  void SetError(uint32_t func_index, const ResultBase& error_result);
-
   Isolate* isolate() const { return isolate_; }
 
   bool failed() const {
@@ -127,13 +125,8 @@ class CompilationStateImpl {
   CompileMode compile_mode() const { return compile_mode_; }
   WasmFeatures* detected_features() { return &detected_features_; }
 
-  const VoidResult& compile_error() const {
-    DCHECK_NOT_NULL(compile_error_);
-    return *compile_error_;
-  }
-
  private:
-  void NotifyOnEvent(CompilationEvent event, const VoidResult* error_result);
+  void NotifyOnEvent(CompilationEvent event, ErrorThrower* thrower);
 
   std::vector<std::unique_ptr<WasmCompilationUnit>>& finish_units() {
     return baseline_compilation_finished_ ? tiering_finish_units_
@@ -158,9 +151,8 @@ class CompilationStateImpl {
   std::vector<std::unique_ptr<WasmCompilationUnit>> tiering_compilation_units_;
 
   bool finisher_is_running_ = false;
-  bool failed_ = false;  // TODO(clemensh): Remove; derive from compile_error_.
+  bool failed_ = false;
   size_t num_background_tasks_ = 0;
-  std::unique_ptr<VoidResult> compile_error_;
 
   std::vector<std::unique_ptr<WasmCompilationUnit>> baseline_finish_units_;
   std::vector<std::unique_ptr<WasmCompilationUnit>> tiering_finish_units_;
@@ -173,7 +165,7 @@ class CompilationStateImpl {
   //////////////////////////////////////////////////////////////////////////////
 
   // Callback function to be called on compilation events.
-  std::function<void(CompilationEvent, const VoidResult*)> callback_;
+  std::function<void(CompilationEvent, ErrorThrower*)> callback_;
 
   CancelableTaskManager background_task_manager_;
   CancelableTaskManager foreground_task_manager_;
@@ -347,11 +339,6 @@ CompilationStateImpl* Impl(CompilationState* compilation_state) {
 
 void CompilationState::CancelAndWait() { Impl(this)->CancelAndWait(); }
 
-void CompilationState::SetError(uint32_t func_index,
-                                const ResultBase& error_result) {
-  Impl(this)->SetError(func_index, error_result);
-}
-
 CompilationState::~CompilationState() { Impl(this)->~CompilationStateImpl(); }
 
 // static
@@ -574,14 +561,17 @@ void InitializeCompilationUnits(NativeModule* native_module,
   builder.Commit();
 }
 
-void FinishCompilationUnits(CompilationStateImpl* compilation_state) {
+void FinishCompilationUnits(CompilationStateImpl* compilation_state,
+                            ErrorThrower* thrower) {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.wasm"), "FinishCompilationUnits");
-  while (!compilation_state->failed()) {
+  while (true) {
+    if (compilation_state->failed()) break;
     std::unique_ptr<WasmCompilationUnit> unit =
         compilation_state->GetNextExecutedUnit();
     if (unit == nullptr) break;
 
     if (unit->failed()) {
+      unit->ReportError(thrower);
       compilation_state->Abort();
       break;
     }
@@ -592,7 +582,8 @@ void FinishCompilationUnits(CompilationStateImpl* compilation_state) {
 }
 
 void CompileInParallel(Isolate* isolate, NativeModule* native_module,
-                       Handle<WasmModuleObject> module_object) {
+                       Handle<WasmModuleObject> module_object,
+                       ErrorThrower* thrower) {
   // Data structures for the parallel compilation.
 
   //-----------------------------------------------------------------------
@@ -652,7 +643,7 @@ void CompileInParallel(Isolate* isolate, NativeModule* native_module,
     //      thread dequeues it and finishes the compilation unit. Compilation
     //      units are finished concurrently to the background threads to save
     //      memory.
-    FinishCompilationUnits(compilation_state);
+    FinishCompilationUnits(compilation_state, thrower);
 
     if (compilation_state->failed()) break;
   }
@@ -663,7 +654,7 @@ void CompileInParallel(Isolate* isolate, NativeModule* native_module,
     //    baseline compilation units are left to be processed. If compilation
     //    already failed, all background tasks have already been canceled
     //    in {FinishCompilationUnits}, and there are no units to finish.
-    FinishCompilationUnits(compilation_state);
+    FinishCompilationUnits(compilation_state, thrower);
 
     if (compilation_state->baseline_compilation_finished()) break;
   }
@@ -693,10 +684,11 @@ void CompileSequentially(Isolate* isolate, NativeModule* native_module,
 
     // Compile the function.
     bool success = WasmCompilationUnit::CompileWasmFunction(
-        isolate, native_module, &detected, &func);
+        isolate, native_module, &detected, thrower, &func);
     if (!success) {
-      thrower->CompileFailed(
-          Impl(native_module->compilation_state())->compile_error());
+      TruncatedUserString<> name(wire_bytes.GetNameOrNull(&func, module));
+      thrower->CompileError("Compilation of #%d:%.*s failed.", i, name.length(),
+                            name.start());
       break;
     }
   }
@@ -766,14 +758,11 @@ void CompileNativeModule(Isolate* isolate, ErrorThrower* thrower,
         V8::GetCurrentPlatform()->NumberOfWorkerThreads() > 0;
 
     if (compile_parallel) {
-      CompileInParallel(isolate, native_module, module_object);
+      CompileInParallel(isolate, native_module, module_object, thrower);
     } else {
       CompileSequentially(isolate, native_module, thrower);
     }
-    auto* compilation_state = Impl(native_module->compilation_state());
-    if (compilation_state->failed()) {
-      thrower->CompileFailed(compilation_state->compile_error());
-    }
+    if (thrower->error()) return;
   }
 }
 
@@ -818,8 +807,11 @@ class FinishCompileTask : public CancelableTask {
       }
 
       if (unit->failed()) {
-        compilation_state_->OnError(compilation_state_->compile_error());
+        ErrorThrower thrower(compilation_state_->isolate(), "AsyncCompile");
+        unit->ReportError(&thrower);
+        compilation_state_->OnError(&thrower);
         compilation_state_->SetFinisherIsRunning(false);
+        thrower.Reset();
         break;
       }
 
@@ -2568,7 +2560,7 @@ class AsyncCompileJob::PrepareAndStartCompile : public CompileStep {
       // on the current step we are in.
       AsyncCompileJob* job = job_;
       compilation_state->SetCallback(
-          [job](CompilationEvent event, const VoidResult* error_result) {
+          [job](CompilationEvent event, ErrorThrower* thrower) {
             // Callback is called from a foreground thread.
             switch (event) {
               case CompilationEvent::kFinishedBaselineCompilation:
@@ -2591,7 +2583,6 @@ class AsyncCompileJob::PrepareAndStartCompile : public CompileStep {
                 }
                 return;
               case CompilationEvent::kFailedCompilation: {
-                DCHECK_NOT_NULL(error_result);
                 // Tier-up compilation should not fail if baseline compilation
                 // did not fail.
                 DCHECK(!Impl(job->native_module_->compilation_state())
@@ -2599,9 +2590,7 @@ class AsyncCompileJob::PrepareAndStartCompile : public CompileStep {
 
                 SaveContext saved_context(job->isolate());
                 job->isolate()->set_context(*job->native_context_);
-                ErrorThrower thrower(job->isolate(), "AsyncCompilation");
-                thrower.CompileFailed(*error_result);
-                Handle<Object> error = thrower.Reify();
+                Handle<Object> error = thrower->Reify();
 
                 DeferredHandleScope deferred(job->isolate());
                 error = handle(*error, job->isolate());
@@ -2917,7 +2906,7 @@ void CompilationStateImpl::SetNumberOfFunctionsToCompile(size_t num_functions) {
 }
 
 void CompilationStateImpl::SetCallback(
-    std::function<void(CompilationEvent, const VoidResult*)> callback) {
+    std::function<void(CompilationEvent, ErrorThrower*)> callback) {
   DCHECK_NULL(callback_);
   callback_ = std::move(callback);
 }
@@ -2980,10 +2969,10 @@ bool CompilationStateImpl::HasCompilationUnitToFinish() {
   return !finish_units().empty();
 }
 
-void CompilationStateImpl::OnError(const VoidResult& error_result) {
-  DCHECK(error_result.failed());
+void CompilationStateImpl::OnError(ErrorThrower* thrower) {
   Abort();
-  NotifyOnEvent(CompilationEvent::kFailedCompilation, &error_result);
+  DCHECK(thrower->error());
+  NotifyOnEvent(CompilationEvent::kFailedCompilation, thrower);
 }
 
 void CompilationStateImpl::OnFinishedUnit() {
@@ -3101,31 +3090,9 @@ void CompilationStateImpl::Abort() {
   background_task_manager_.CancelAndWait();
 }
 
-void CompilationStateImpl::SetError(uint32_t func_index,
-                                    const ResultBase& error_result) {
-  DCHECK(error_result.failed());
-  base::MutexGuard guard(&mutex_);
-  // Ignore all but the first error.
-  if (compile_error_) return;
-  std::ostringstream error;
-  error << "Compiling wasm function \"";
-  wasm::ModuleWireBytes wire_bytes(native_module_->wire_bytes());
-  wasm::WireBytesRef name_ref =
-      native_module_->module()->LookupFunctionName(wire_bytes, func_index);
-  if (name_ref.is_set()) {
-    wasm::WasmName name = wire_bytes.GetNameOrNull(name_ref);
-    error.write(name.start(), name.length());
-  } else {
-    error << "wasm-function[" << func_index << "]";
-  }
-  error << "\" failed: " << error_result.error_msg();
-  compile_error_ = base::make_unique<VoidResult>(
-      VoidResult::Error(error_result.error_offset(), error.str()));
-}
-
 void CompilationStateImpl::NotifyOnEvent(CompilationEvent event,
-                                         const VoidResult* error_result) {
-  if (callback_) callback_(event, error_result);
+                                         ErrorThrower* thrower) {
+  if (callback_) callback_(event, thrower);
 }
 
 void CompileJsToWasmWrappers(Isolate* isolate,

src/wasm/wasm-engine.cc

@@ -173,10 +173,11 @@ std::shared_ptr<StreamingDecoder> WasmEngine::StartStreamingCompilation(
 
 bool WasmEngine::CompileFunction(Isolate* isolate, NativeModule* native_module,
                                  uint32_t function_index, ExecutionTier tier) {
+  ErrorThrower thrower(isolate, "Manually requested tier up");
   // Note we assume that "one-off" compilations can discard detected features.
   WasmFeatures detected = kNoWasmFeatures;
   return WasmCompilationUnit::CompileWasmFunction(
-      isolate, native_module, &detected,
+      isolate, native_module, &detected, &thrower,
       &native_module->module()->functions[function_index], tier);
 }
 

src/wasm/wasm-engine.h

@@ -95,8 +95,8 @@ class V8_EXPORT_PRIVATE WasmEngine {
       std::shared_ptr<CompilationResultResolver> resolver);
 
   // Compiles the function with the given index at a specific compilation tier
-  // and returns true on success, false otherwise. This is mostly used for
-  // testing to force a function into a specific tier.
+  // and returns true on success, false (and pending exception) otherwise. This
+  // is mostly used for testing to force a function into a specific tier.
   bool CompileFunction(Isolate* isolate, NativeModule* native_module,
                        uint32_t function_index, ExecutionTier tier);
 

src/wasm/wasm-result.h

@@ -125,17 +125,13 @@ class V8_EXPORT_PRIVATE ErrorThrower {
   PRINTF_FORMAT(2, 3) void LinkError(const char* fmt, ...);
   PRINTF_FORMAT(2, 3) void RuntimeError(const char* fmt, ...);
 
-  void CompileFailed(const char* error, const ResultBase& result) {
+  template <typename T>
+  void CompileFailed(const char* error, const Result<T>& result) {
     DCHECK(result.failed());
     CompileError("%s: %s @+%u", error, result.error_msg().c_str(),
                  result.error_offset());
   }
 
-  void CompileFailed(const ResultBase& result) {
-    DCHECK(result.failed());
-    CompileError("%s @+%u", result.error_msg().c_str(), result.error_offset());
-  }
-
   // Create and return exception object.
   V8_WARN_UNUSED_RESULT Handle<Object> Reify();
 

test/cctest/wasm/test-wasm-shared-engine.cc

@@ -350,9 +350,10 @@ TEST(SharedEngineRunThreadedTierUp) {
   threads.emplace_back(&engine, [module](SharedEngineIsolate& isolate) {
     HandleScope scope(isolate.isolate());
     Handle<WasmInstanceObject> instance = isolate.ImportInstance(module);
+    ErrorThrower thrower(isolate.isolate(), "Forced Tier Up");
     WasmFeatures detected = kNoWasmFeatures;
     WasmCompilationUnit::CompileWasmFunction(
-        isolate.isolate(), module.get(), &detected,
+        isolate.isolate(), module.get(), &detected, &thrower,
         &module->module()->functions[0], ExecutionTier::kOptimized);
     CHECK_EQ(23, isolate.Run(instance));
   });