[wasm] Move error reporting out of FinishCompilation

And remove the TurboFan/Liftoff specific {FinishCompilation}
implementations completely. Compilation errors are now stored in the
{WasmCompilationUnit} directly as a {Result<WasmCode*>}. They are
retrieved via {WasmCompilationUnit::ReportError}, which moves the error
to the {ErrorThrower}.
This prepares more changes to completely remove the {FinishCompilation}
phase.

R=titzer@chromium.org

Bug: v8:7921
Change-Id: I4f9a6e919359aeab074880d0d38211500b76e4ec
Reviewed-on: https://chromium-review.googlesource.com/c/1290975
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Ben Titzer <titzer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#56826}

src/compiler/wasm-compiler.cc

@@ -5152,15 +5152,16 @@ SourcePositionTable* TurbofanWasmCompilationUnit::BuildGraphForWasmFunction(
       new (mcgraph->zone()) SourcePositionTable(mcgraph->graph());
   WasmGraphBuilder builder(wasm_unit_->env_, mcgraph->zone(), mcgraph,
                            wasm_unit_->func_body_.sig, source_position_table);
-  graph_construction_result_ = wasm::BuildTFGraph(
+  wasm::VoidResult graph_construction_result = wasm::BuildTFGraph(
       wasm_unit_->wasm_engine_->allocator(),
       wasm_unit_->native_module_->enabled_features(), wasm_unit_->env_->module,
       &builder, detected, wasm_unit_->func_body_, node_origins);
-  if (graph_construction_result_.failed()) {
+  if (graph_construction_result.failed()) {
     if (FLAG_trace_wasm_compiler) {
       StdoutStream{} << "Compilation failed: "
-                     << graph_construction_result_.error_msg() << std::endl;
+                     << graph_construction_result.error_msg() << std::endl;
     }
+    wasm_unit_->result_.MoveErrorFrom(graph_construction_result);
     return nullptr;
   }
 
@@ -5199,7 +5200,6 @@ Vector<const char> GetDebugName(Zone* zone, int index) {
   memcpy(index_name, name_vector.start(), name_len);
   return Vector<const char>(index_name, name_len);
 }
-
 }  // namespace
 
 void TurbofanWasmCompilationUnit::ExecuteCompilation(
@@ -5239,7 +5239,7 @@ void TurbofanWasmCompilationUnit::ExecuteCompilation(
   SourcePositionTable* source_positions =
       BuildGraphForWasmFunction(detected, &decode_ms, mcgraph, node_origins);
 
-  if (graph_construction_result_.failed()) return;
+  if (wasm_unit_->failed()) return;
 
   if (node_origins) {
     node_origins->AddDecorator();
@@ -5280,38 +5280,12 @@ void TurbofanWasmCompilationUnit::ExecuteCompilation(
         decode_ms, node_count, pipeline_ms);
   }
   if (succeeded) {
-    wasm_code_ = info.wasm_code();
-    wasm_unit_->native_module()->PublishCode(wasm_code_);
+    wasm::WasmCode* code = info.wasm_code();
+    wasm_unit_->result_ = wasm::Result<wasm::WasmCode*>(code);
+    wasm_unit_->native_module()->PublishCode(code);
   }
 }
 
-wasm::WasmCode* TurbofanWasmCompilationUnit::FinishCompilation(
-    wasm::ErrorThrower* thrower) {
-  DCHECK_IMPLIES(graph_construction_result_.failed(), wasm_code_ == nullptr);
-  if (wasm_code_ != nullptr) return wasm_code_;
-
-  if (graph_construction_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(wasm_unit_->native_module()->wire_bytes());
-    wasm::WireBytesRef name_ref =
-        wasm_unit_->native_module()->module()->LookupFunctionName(
-            wire_bytes, wasm_unit_->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",
-               wasm_unit_->func_index_);
-    }
-    thrower->CompileFailed(message.start(), graph_construction_result_);
-  }
-
-  return nullptr;
-}
-
 namespace {
 // Helper for allocating either an GP or FP reg, or the next stack slot.
 class LinkageLocationAllocator {

src/compiler/wasm-compiler.h

@@ -57,12 +57,8 @@ class TurbofanWasmCompilationUnit {
 
   void ExecuteCompilation(wasm::WasmFeatures* detected);
 
-  wasm::WasmCode* FinishCompilation(wasm::ErrorThrower*);
-
  private:
   wasm::WasmCompilationUnit* const wasm_unit_;
-  wasm::WasmCode* wasm_code_ = nullptr;
-  wasm::Result<wasm::DecodeStruct*> graph_construction_result_;
 
   DISALLOW_COPY_AND_ASSIGN(TurbofanWasmCompilationUnit);
 };

src/wasm/baseline/liftoff-compiler.cc

@@ -1920,19 +1920,16 @@ bool LiftoffCompilationUnit::ExecuteCompilation(WasmFeatures* detected) {
   uint32_t frame_slot_count = compiler->GetTotalFrameSlotCount();
   int safepoint_table_offset = compiler->GetSafepointTableOffset();
 
-  code_ = wasm_unit_->native_module_->AddCode(
+  WasmCode* code = wasm_unit_->native_module_->AddCode(
       wasm_unit_->func_index_, desc, frame_slot_count, safepoint_table_offset,
       0, std::move(protected_instructions), std::move(source_positions),
       WasmCode::kLiftoff);
-  wasm_unit_->native_module_->PublishCode(code_);
+  wasm_unit_->result_ = Result<WasmCode*>(code);
+  wasm_unit_->native_module_->PublishCode(code);
 
   return true;
 }
 
-WasmCode* LiftoffCompilationUnit::FinishCompilation(ErrorThrower*) {
-  return code_;
-}
-
 #undef __
 #undef TRACE
 #undef WASM_INSTANCE_OBJECT_OFFSET

src/wasm/baseline/liftoff-compiler.h

@@ -22,14 +22,10 @@ class LiftoffCompilationUnit final {
       : wasm_unit_(wasm_unit) {}
 
   bool ExecuteCompilation(WasmFeatures* detected);
-  WasmCode* FinishCompilation(ErrorThrower*);
 
  private:
   WasmCompilationUnit* const wasm_unit_;
 
-  // Result of compilation:
-  WasmCode* code_;
-
   DISALLOW_COPY_AND_ASSIGN(LiftoffCompilationUnit);
 };
 

src/wasm/decoder.h

@@ -28,11 +28,8 @@ namespace wasm {
     if (FLAG_trace_wasm_decoder && (cond)) PrintF(__VA_ARGS__); \
   } while (false)
 
-// A {DecodeResult} only stores the failure / success status, but no data. Thus
-// we use {nullptr_t} as data value, such that the only valid data stored in
-// this type is a nullptr.
-// Storing {void} would require template specialization.
-using DecodeResult = Result<std::nullptr_t>;
+// A {DecodeResult} only stores the failure / success status, but no data.
+using DecodeResult = VoidResult;
 
 // A helper utility to decode bytes, integers, fields, varints, etc, from
 // a buffer of bytes.

src/wasm/function-compiler.cc

@@ -99,24 +99,30 @@ void WasmCompilationUnit::ExecuteCompilation(WasmFeatures* detected) {
   }
 }
 
-WasmCode* WasmCompilationUnit::FinishCompilation(ErrorThrower* thrower) {
-  WasmCode* ret;
-  switch (mode_) {
-    case ExecutionTier::kBaseline:
-      ret = liftoff_unit_->FinishCompilation(thrower);
-      break;
-    case ExecutionTier::kOptimized:
-      ret = turbofan_unit_->FinishCompilation(thrower);
-      break;
-    case ExecutionTier::kInterpreter:
-      UNREACHABLE();  // TODO(titzer): finish interpreter entry stub.
+void WasmCompilationUnit::FinishCompilation() {
+  // TODO(clemensh): Move this somewhere else, then remove FinishCompilation.
+  if (!failed()) {
+    RecordStats(result(), counters_);
   }
-  if (ret == nullptr) {
-    thrower->RuntimeError("Error finalizing code.");
+}
+
+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 {
-    RecordStats(ret, counters_);
+    SNPrintF(message, "Compiling wasm function \"wasm-function[%d]\" failed",
+             func_index_);
   }
-  return ret;
+  thrower->CompileFailed(message.start(), result_);
 }
 
 void WasmCompilationUnit::SwitchMode(ExecutionTier new_mode) {
@@ -155,7 +161,12 @@ WasmCode* WasmCompilationUnit::CompileWasmFunction(
                            function_body,
                            function->func_index, isolate->counters(), mode);
   unit.ExecuteCompilation(detected);
-  return unit.FinishCompilation(thrower);
+  unit.FinishCompilation();
+  if (unit.failed()) {
+    unit.ReportError(thrower);
+    return nullptr;
+  }
+  return unit.result();
 }
 
 }  // namespace wasm

src/wasm/function-compiler.h

@@ -92,16 +92,24 @@ class WasmCompilationUnit final {
   ~WasmCompilationUnit();
 
   void ExecuteCompilation(WasmFeatures* detected);
-  WasmCode* FinishCompilation(ErrorThrower* thrower);
+  void FinishCompilation();
+
+  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;
 
   static WasmCode* CompileWasmFunction(
       Isolate* isolate, NativeModule* native_module, WasmFeatures* detected,
       ErrorThrower* thrower, ModuleEnv* env, const WasmFunction* function,
       ExecutionTier = GetDefaultExecutionTier());
 
-  NativeModule* native_module() const { return native_module_; }
-  ExecutionTier mode() const { return mode_; }
-
  private:
   friend class LiftoffCompilationUnit;
   friend class compiler::TurbofanWasmCompilationUnit;
@@ -113,6 +121,8 @@ class WasmCompilationUnit final {
   int func_index_;
   NativeModule* native_module_;
   ExecutionTier mode_;
+  wasm::Result<WasmCode*> result_;
+
   // LiftoffCompilationUnit, set if {mode_ == kLiftoff}.
   std::unique_ptr<LiftoffCompilationUnit> liftoff_unit_;
   // TurbofanWasmCompilationUnit, set if {mode_ == kTurbofan}.

src/wasm/module-compiler.cc

@@ -358,21 +358,22 @@ WasmCode* LazyCompileFunction(Isolate* isolate, NativeModule* native_module,
                     module_start + func->code.offset(),
                     module_start + func->code.end_offset()};
 
-  ErrorThrower thrower(isolate, "WasmLazyCompile");
   WasmCompilationUnit unit(isolate->wasm_engine(), module_env, native_module,
                            body, func_index, isolate->counters());
   unit.ExecuteCompilation(
       native_module->compilation_state()->detected_features());
-  WasmCode* wasm_code = unit.FinishCompilation(&thrower);
-
-  if (WasmCode::ShouldBeLogged(isolate)) wasm_code->LogCode(isolate);
+  unit.FinishCompilation();
 
   // If there is a pending error, something really went wrong. The module was
   // verified before starting execution with lazy compilation.
   // This might be OOM, but then we cannot continue execution anyway.
   // TODO(clemensh): According to the spec, we can actually skip validation at
   // module creation time, and return a function that always traps here.
-  CHECK(!thrower.error());
+  CHECK(!unit.failed());
+
+  WasmCode* code = unit.result();
+
+  if (WasmCode::ShouldBeLogged(isolate)) code->LogCode(isolate);
 
   int64_t func_size =
       static_cast<int64_t>(func->code.end_offset() - func->code.offset());
@@ -385,7 +386,7 @@ WasmCode* LazyCompileFunction(Isolate* isolate, NativeModule* native_module,
       compilation_time != 0 ? static_cast<int>(func_size / compilation_time)
                             : 0);
 
-  return wasm_code;
+  return code;
 }
 
 Address CompileLazy(Isolate* isolate, NativeModule* native_module,
@@ -548,17 +549,16 @@ void FinishCompilationUnits(CompilationState* compilation_state,
     std::unique_ptr<WasmCompilationUnit> unit =
         compilation_state->GetNextExecutedUnit();
     if (unit == nullptr) break;
-    WasmCode* result = unit->FinishCompilation(thrower);
+    unit->FinishCompilation();
 
-    if (thrower->error()) {
+    if (unit->failed()) {
+      unit->ReportError(thrower);
       compilation_state->Abort();
       break;
     }
 
     // Update the compilation state.
     compilation_state->OnFinishedUnit();
-    DCHECK_IMPLIES(result == nullptr, thrower->error());
-    if (result == nullptr) break;
   }
   if (!compilation_state->failed()) {
     compilation_state->RestartBackgroundTasks();
@@ -789,17 +789,19 @@ class FinishCompileTask : public CancelableTask {
         break;
       }
 
-      ErrorThrower thrower(compilation_state_->isolate(), "AsyncCompile");
-      WasmCode* result = unit->FinishCompilation(&thrower);
+      unit->FinishCompilation();
 
-      if (thrower.error()) {
-        DCHECK_NULL(result);
+      if (unit->failed()) {
+        ErrorThrower thrower(compilation_state_->isolate(), "AsyncCompile");
+        unit->ReportError(&thrower);
         compilation_state_->OnError(&thrower);
         compilation_state_->SetFinisherIsRunning(false);
         thrower.Reset();
         break;
       }
 
+      WasmCode* result = unit->result();
+
       if (compilation_state_->baseline_compilation_finished()) {
         // If Liftoff compilation finishes it will directly start executing.
         // As soon as we have Turbofan-compiled code available, it will

src/wasm/wasm-result.h

@@ -122,7 +122,7 @@ class V8_EXPORT_PRIVATE ErrorThrower {
   PRINTF_FORMAT(2, 3) void RuntimeError(const char* fmt, ...);
 
   template <typename T>
-  void CompileFailed(const char* error, Result<T>& result) {
+  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());
@@ -168,6 +168,11 @@ class V8_EXPORT_PRIVATE ErrorThrower {
   DISALLOW_NEW_AND_DELETE();
 };
 
+// Use {nullptr_t} as data value to indicate that this only stores the error,
+// but no result value (the only valid value is {nullptr}).
+// [Storing {void} would require template specialization.]
+using VoidResult = Result<std::nullptr_t>;
+
 }  // namespace wasm
 }  // namespace internal
 }  // namespace v8

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

@@ -412,7 +412,6 @@ void WasmFunctionCompiler::Build(const byte* start, const byte* end) {
                                          ->wire_bytes();
 
   ModuleEnv module_env = builder_->CreateModuleEnv();
-  ErrorThrower thrower(isolate(), "WasmFunctionCompiler::Build");
   ScopedVector<uint8_t> func_wire_bytes(function_->code.length());
   memcpy(func_wire_bytes.start(), wire_bytes.start() + function_->code.offset(),
          func_wire_bytes.length());
@@ -426,11 +425,9 @@ void WasmFunctionCompiler::Build(const byte* start, const byte* end) {
                            isolate()->counters(), tier);
   WasmFeatures unused_detected_features;
   unit.ExecuteCompilation(&unused_detected_features);
-  WasmCode* wasm_code = unit.FinishCompilation(&thrower);
-  if (WasmCode::ShouldBeLogged(isolate())) {
-    wasm_code->LogCode(isolate());
-  }
-  CHECK(!thrower.error());
+  unit.FinishCompilation();
+  CHECK(!unit.failed());
+  if (WasmCode::ShouldBeLogged(isolate())) unit.result()->LogCode(isolate());
 }
 
 WasmFunctionCompiler::WasmFunctionCompiler(Zone* zone, FunctionSig* sig,