[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/1303720Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57091}

src/compiler/wasm-compiler.cc

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

src/wasm/compilation-environment.h

@@ -13,6 +13,7 @@ namespace internal {
 namespace wasm {
 
 class NativeModule;
+class ResultBase;
 
 enum RuntimeExceptionSupport : bool {
   kRuntimeExceptionSupport = true,
@@ -71,6 +72,8 @@ 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,25 +93,6 @@ 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
@@ -135,9 +116,11 @@ void WasmCompilationUnit::SwitchMode(ExecutionTier new_mode) {
 }
 
 // static
-bool WasmCompilationUnit::CompileWasmFunction(
-    Isolate* isolate, NativeModule* native_module, WasmFeatures* detected,
-    ErrorThrower* thrower, const WasmFunction* function, ExecutionTier mode) {
+bool WasmCompilationUnit::CompileWasmFunction(Isolate* isolate,
+                                              NativeModule* native_module,
+                                              WasmFeatures* detected,
+                                              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(),
@@ -147,17 +130,12 @@ bool WasmCompilationUnit::CompileWasmFunction(
                            function->func_index, mode);
   CompilationEnv env = native_module->CreateCompilationEnv();
   unit.ExecuteCompilation(&env, isolate->counters(), detected);
-  if (unit.failed()) {
-    unit.ReportError(thrower);
-    return false;
-  }
-  return true;
+  return !unit.failed();
 }
 
 void WasmCompilationUnit::SetResult(WasmCode* code, Counters* counters) {
-  DCHECK(!result_.failed());
-  DCHECK_NULL(result_.value());
-  result_ = Result<WasmCode*>(code);
+  DCHECK_NULL(result_);
+  result_ = code;
   native_module()->PublishCode(code);
 
   counters->wasm_generated_code_size()->Increment(

src/wasm/function-compiler.h

@@ -46,17 +46,11 @@ class WasmCompilationUnit final {
 
   NativeModule* native_module() const { return native_module_; }
   ExecutionTier mode() const { return mode_; }
-  bool failed() const { return result_.failed(); }
-  WasmCode* result() const {
-    DCHECK(!failed());
-    DCHECK_NOT_NULL(result_.value());
-    return result_.value();
-  }
-
-  void ReportError(ErrorThrower* thrower) const;
+  bool failed() const { return result_ == nullptr; }  // TODO(clemensh): Remove.
+  WasmCode* result() const { return result_; }
 
   static bool CompileWasmFunction(Isolate* isolate, NativeModule* native_module,
-                                  WasmFeatures* detected, ErrorThrower* thrower,
+                                  WasmFeatures* detected,
                                   const WasmFunction* function,
                                   ExecutionTier = GetDefaultExecutionTier());
 
@@ -69,7 +63,7 @@ class WasmCompilationUnit final {
   int func_index_;
   NativeModule* native_module_;
   ExecutionTier mode_;
-  wasm::Result<WasmCode*> result_;
+  WasmCode* result_ = nullptr;
 
   // 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, ErrorThrower*)> callback);
+      std::function<void(CompilationEvent, const VoidResult*)> callback);
 
   // Inserts new functions to compile and kicks off compilation.
   void AddCompilationUnits(
@@ -94,7 +94,7 @@ class CompilationStateImpl {
 
   bool HasCompilationUnitToFinish();
 
-  void OnError(ErrorThrower* thrower);
+  void OnError(const VoidResult& error_result);
   void OnFinishedUnit();
   void ScheduleUnitForFinishing(std::unique_ptr<WasmCompilationUnit> unit,
                                 ExecutionTier mode);
@@ -109,6 +109,8 @@ class CompilationStateImpl {
 
   void Abort();
 
+  void SetError(uint32_t func_index, const ResultBase& error_result);
+
   Isolate* isolate() const { return isolate_; }
 
   bool failed() const {
@@ -125,8 +127,13 @@ 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, ErrorThrower* thrower);
+  void NotifyOnEvent(CompilationEvent event, const VoidResult* error_result);
 
   std::vector<std::unique_ptr<WasmCompilationUnit>>& finish_units() {
     return baseline_compilation_finished_ ? tiering_finish_units_
@@ -151,8 +158,9 @@ class CompilationStateImpl {
   std::vector<std::unique_ptr<WasmCompilationUnit>> tiering_compilation_units_;
 
   bool finisher_is_running_ = false;
-  bool failed_ = false;
+  bool failed_ = false;  // TODO(clemensh): Remove; derive from compile_error_.
   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_;
@@ -165,7 +173,7 @@ class CompilationStateImpl {
   //////////////////////////////////////////////////////////////////////////////
 
   // Callback function to be called on compilation events.
-  std::function<void(CompilationEvent, ErrorThrower*)> callback_;
+  std::function<void(CompilationEvent, const VoidResult*)> callback_;
 
   CancelableTaskManager background_task_manager_;
   CancelableTaskManager foreground_task_manager_;
@@ -339,6 +347,11 @@ 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
@@ -561,17 +574,14 @@ void InitializeCompilationUnits(NativeModule* native_module,
   builder.Commit();
 }
 
-void FinishCompilationUnits(CompilationStateImpl* compilation_state,
-                            ErrorThrower* thrower) {
+void FinishCompilationUnits(CompilationStateImpl* compilation_state) {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.wasm"), "FinishCompilationUnits");
-  while (true) {
-    if (compilation_state->failed()) break;
+  while (!compilation_state->failed()) {
     std::unique_ptr<WasmCompilationUnit> unit =
         compilation_state->GetNextExecutedUnit();
     if (unit == nullptr) break;
 
     if (unit->failed()) {
-      unit->ReportError(thrower);
       compilation_state->Abort();
       break;
     }
@@ -582,8 +592,7 @@ void FinishCompilationUnits(CompilationStateImpl* compilation_state,
 }
 
 void CompileInParallel(Isolate* isolate, NativeModule* native_module,
-                       Handle<WasmModuleObject> module_object,
-                       ErrorThrower* thrower) {
+                       Handle<WasmModuleObject> module_object) {
   // Data structures for the parallel compilation.
 
   //-----------------------------------------------------------------------
@@ -643,7 +652,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, thrower);
+    FinishCompilationUnits(compilation_state);
 
     if (compilation_state->failed()) break;
   }
@@ -654,7 +663,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, thrower);
+    FinishCompilationUnits(compilation_state);
 
     if (compilation_state->baseline_compilation_finished()) break;
   }
@@ -684,11 +693,10 @@ void CompileSequentially(Isolate* isolate, NativeModule* native_module,
 
     // Compile the function.
     bool success = WasmCompilationUnit::CompileWasmFunction(
-        isolate, native_module, &detected, thrower, &func);
+        isolate, native_module, &detected, &func);
     if (!success) {
-      TruncatedUserString<> name(wire_bytes.GetNameOrNull(&func, module));
-      thrower->CompileError("Compilation of #%d:%.*s failed.", i, name.length(),
-                            name.start());
+      thrower->CompileFailed(
+          Impl(native_module->compilation_state())->compile_error());
       break;
     }
   }
@@ -758,11 +766,14 @@ void CompileNativeModule(Isolate* isolate, ErrorThrower* thrower,
         V8::GetCurrentPlatform()->NumberOfWorkerThreads() > 0;
 
     if (compile_parallel) {
-      CompileInParallel(isolate, native_module, module_object, thrower);
+      CompileInParallel(isolate, native_module, module_object);
     } else {
       CompileSequentially(isolate, native_module, thrower);
     }
-    if (thrower->error()) return;
+    auto* compilation_state = Impl(native_module->compilation_state());
+    if (compilation_state->failed()) {
+      thrower->CompileFailed(compilation_state->compile_error());
+    }
   }
 }
 
@@ -807,11 +818,8 @@ class FinishCompileTask : public CancelableTask {
       }
 
       if (unit->failed()) {
-        ErrorThrower thrower(compilation_state_->isolate(), "AsyncCompile");
-        unit->ReportError(&thrower);
-        compilation_state_->OnError(&thrower);
+        compilation_state_->OnError(compilation_state_->compile_error());
         compilation_state_->SetFinisherIsRunning(false);
-        thrower.Reset();
         break;
       }
 
@@ -2560,7 +2568,7 @@ class AsyncCompileJob::PrepareAndStartCompile : public CompileStep {
       // on the current step we are in.
       AsyncCompileJob* job = job_;
       compilation_state->SetCallback(
-          [job](CompilationEvent event, ErrorThrower* thrower) {
+          [job](CompilationEvent event, const VoidResult* error_result) {
             // Callback is called from a foreground thread.
             switch (event) {
               case CompilationEvent::kFinishedBaselineCompilation:
@@ -2583,6 +2591,7 @@ 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())
@@ -2590,7 +2599,9 @@ class AsyncCompileJob::PrepareAndStartCompile : public CompileStep {
 
                 SaveContext saved_context(job->isolate());
                 job->isolate()->set_context(*job->native_context_);
-                Handle<Object> error = thrower->Reify();
+                ErrorThrower thrower(job->isolate(), "AsyncCompilation");
+                thrower.CompileFailed(*error_result);
+                Handle<Object> error = thrower.Reify();
 
                 DeferredHandleScope deferred(job->isolate());
                 error = handle(*error, job->isolate());
@@ -2906,7 +2917,7 @@ void CompilationStateImpl::SetNumberOfFunctionsToCompile(size_t num_functions) {
 }
 
 void CompilationStateImpl::SetCallback(
-    std::function<void(CompilationEvent, ErrorThrower*)> callback) {
+    std::function<void(CompilationEvent, const VoidResult*)> callback) {
   DCHECK_NULL(callback_);
   callback_ = std::move(callback);
 }
@@ -2969,10 +2980,10 @@ bool CompilationStateImpl::HasCompilationUnitToFinish() {
   return !finish_units().empty();
 }
 
-void CompilationStateImpl::OnError(ErrorThrower* thrower) {
+void CompilationStateImpl::OnError(const VoidResult& error_result) {
+  DCHECK(error_result.failed());
   Abort();
-  DCHECK(thrower->error());
-  NotifyOnEvent(CompilationEvent::kFailedCompilation, thrower);
+  NotifyOnEvent(CompilationEvent::kFailedCompilation, &error_result);
 }
 
 void CompilationStateImpl::OnFinishedUnit() {
@@ -3090,9 +3101,31 @@ 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,
-                                         ErrorThrower* thrower) {
-  if (callback_) callback_(event, thrower);
+                                         const VoidResult* error_result) {
+  if (callback_) callback_(event, error_result);
 }
 
 void CompileJsToWasmWrappers(Isolate* isolate,

src/wasm/wasm-engine.cc

@@ -173,11 +173,10 @@ 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, &thrower,
+      isolate, native_module, &detected,
       &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 (and pending exception) otherwise. This
-  // is mostly used for testing to force a function into a specific tier.
+  // and returns true on success, false 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,13 +125,17 @@ class V8_EXPORT_PRIVATE ErrorThrower {
   PRINTF_FORMAT(2, 3) void LinkError(const char* fmt, ...);
   PRINTF_FORMAT(2, 3) void RuntimeError(const char* fmt, ...);
 
-  template <typename T>
-  void CompileFailed(const char* error, const Result<T>& result) {
+  void CompileFailed(const char* error, const ResultBase& 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,10 +350,9 @@ 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, &thrower,
+        isolate.isolate(), module.get(), &detected,
         &module->module()->functions[0], ExecutionTier::kOptimized);
     CHECK_EQ(23, isolate.Run(instance));
   });