Reland "[wasm] Store compile errors in CompilationState"

This is a reland of bf3d7b9a

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}

Bug: v8:8343, v8:7921
Change-Id: Iaa5c89d224cb2bcfca2d12eba305413a9ad95618
Reviewed-on: https://chromium-review.googlesource.com/c/1304547
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57126}

src/compiler/wasm-compiler.cc

@@ -5198,19 +5198,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,
@@ -5220,9 +5219,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();
@@ -5243,7 +5242,7 @@ SourcePositionTable* TurbofanWasmCompilationUnit::BuildGraphForWasmFunction(
   if (FLAG_trace_wasm_decode_time) {
     *decode_ms = decode_timer.Elapsed().InMillisecondsF();
   }
-  return source_position_table;
+  return true;
 }
 
 namespace {
@@ -5292,10 +5291,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/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

@@ -88,6 +88,10 @@ class Result : public ResultBase {
                  std::move(error_result).error_msg());
   }
 
+  static Result<T> ErrorFrom(const ResultBase& error_result) {
+    return Error(error_result.error_offset(), error_result.error_msg());
+  }
+
   // Accessor for the value. Returns const reference if {this} is l-value or
   // const, and returns r-value reference if {this} is r-value. This allows to
   // extract non-copyable values like {std::unique_ptr} by using
@@ -125,13 +129,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));
   });