[wasm] Make error messages deterministic

Compilation only stores whether an error has been found, but not the
exact error or it's location. This is generated by running a validation
pass once all wire bytes have been received.
This unifies error messages by removing one more location where we
generate compilation error messages, and makes it deterministic because
a) we always report the error in the first failing function, and
b) if names are present, the error message will always contain the
   function name.

R=titzer@chromium.org

Bug: chromium:926311, v8:8814
Change-Id: I79551b8bb73dcee503484de343a3ada60a6add4f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1521112
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Ben Titzer <titzer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60242}

src/compiler/wasm-compiler.cc

@@ -6020,8 +6020,7 @@ TurbofanWasmCompilationUnit::~TurbofanWasmCompilationUnit() = default;
 bool TurbofanWasmCompilationUnit::BuildGraphForWasmFunction(
     wasm::CompilationEnv* env, const wasm::FunctionBody& func_body,
     wasm::WasmFeatures* detected, double* decode_ms, MachineGraph* mcgraph,
-    NodeOriginTable* node_origins, SourcePositionTable* source_positions,
-    wasm::WasmError* error_out) {
+    NodeOriginTable* node_origins, SourcePositionTable* source_positions) {
   base::ElapsedTimer decode_timer;
   if (FLAG_trace_wasm_decode_time) {
     decode_timer.Start();
@@ -6039,7 +6038,6 @@ bool TurbofanWasmCompilationUnit::BuildGraphForWasmFunction(
                      << graph_construction_result.error().message()
                      << std::endl;
     }
-    *error_out = graph_construction_result.error();
     return false;
   }
 
@@ -6111,11 +6109,9 @@ wasm::WasmCompilationResult TurbofanWasmCompilationUnit::ExecuteCompilation(
                                       : nullptr;
   SourcePositionTable* source_positions =
       new (mcgraph->zone()) SourcePositionTable(mcgraph->graph());
-  wasm::WasmError error;
   if (!BuildGraphForWasmFunction(env, func_body, detected, &decode_ms, mcgraph,
-                                 node_origins, source_positions, &error)) {
-    DCHECK(!error.empty());
-    return wasm::WasmCompilationResult{std::move(error)};
+                                 node_origins, source_positions)) {
+    return wasm::WasmCompilationResult{};
   }
 
   if (node_origins) {
@@ -6161,9 +6157,7 @@ wasm::WasmCompilationResult InterpreterCompilationUnit::ExecuteCompilation(
   wasm::WasmFullDecoder<wasm::Decoder::kValidate, wasm::EmptyInterface> decoder(
       &zone, module, env->enabled_features, detected, func_body);
   decoder.Decode();
-  if (decoder.failed()) {
-    return wasm::WasmCompilationResult{decoder.error()};
-  }
+  if (decoder.failed()) return wasm::WasmCompilationResult{};
 
   wasm::WasmCompilationResult result = CompileWasmInterpreterEntry(
       wasm_unit_->wasm_engine_, env->enabled_features, wasm_unit_->func_index_,

src/compiler/wasm-compiler.h

@@ -55,8 +55,7 @@ class TurbofanWasmCompilationUnit {
                                  wasm::WasmFeatures* detected,
                                  double* decode_ms, MachineGraph* mcgraph,
                                  NodeOriginTable* node_origins,
-                                 SourcePositionTable* source_positions,
-                                 wasm::WasmError* error_out);
+                                 SourcePositionTable* source_positions);
 
   wasm::WasmCompilationResult ExecuteCompilation(wasm::CompilationEnv*,
                                                  const wasm::FunctionBody&,

src/wasm/baseline/liftoff-compiler.cc

@@ -2004,12 +2004,12 @@ WasmCompilationResult LiftoffCompilationUnit::ExecuteCompilation(
   LiftoffCompiler* compiler = &decoder.interface();
   if (decoder.failed()) {
     compiler->OnFirstError(&decoder);
-    return WasmCompilationResult{decoder.error()};
+    return WasmCompilationResult{};
   }
   if (!compiler->ok()) {
     // Liftoff compilation failed.
     counters->liftoff_unsupported_functions()->Increment();
-    return WasmCompilationResult{WasmError{0, "Liftoff bailout"}};
+    return WasmCompilationResult{};
   }
 
   counters->liftoff_compiled_functions()->Increment();

src/wasm/compilation-environment.h

@@ -109,7 +109,7 @@ class CompilationState {
 
   void AbortCompilation();
 
-  void SetError(uint32_t func_index, const WasmError& error);
+  void SetError();
 
   void SetWireBytesStorage(std::shared_ptr<WireBytesStorage>);
 

src/wasm/function-compiler.cc

@@ -195,8 +195,7 @@ WasmCompilationResult WasmCompilationUnit::ExecuteCompilation(
 WasmCode* WasmCompilationUnit::Publish(WasmCompilationResult result,
                                        NativeModule* native_module) {
   if (!result.succeeded()) {
-    native_module->compilation_state()->SetError(func_index_,
-                                                 std::move(result.error));
+    native_module->compilation_state()->SetError();
     return nullptr;
   }
 

src/wasm/function-compiler.h

@@ -51,12 +51,7 @@ struct WasmCompilationResult {
  public:
   MOVE_ONLY_WITH_DEFAULT_CONSTRUCTORS(WasmCompilationResult);
 
-  explicit WasmCompilationResult(WasmError error) : error(std::move(error)) {}
-
-  bool succeeded() const {
-    DCHECK_EQ(code_desc.buffer != nullptr, error.empty());
-    return error.empty();
-  }
+  bool succeeded() const { return code_desc.buffer != nullptr; }
   operator bool() const { return succeeded(); }
 
   CodeDesc code_desc;
@@ -65,8 +60,6 @@ struct WasmCompilationResult {
   uint32_t tagged_parameter_slots = 0;
   OwnedVector<byte> source_positions;
   OwnedVector<trap_handler::ProtectedInstructionData> protected_instructions;
-
-  WasmError error;
 };
 
 class WasmCompilationUnit final {

src/wasm/module-compiler.cc

@@ -120,7 +120,6 @@ class CompilationStateImpl {
  public:
   CompilationStateImpl(const std::shared_ptr<NativeModule>& native_module,
                        std::shared_ptr<Counters> async_counters);
-  ~CompilationStateImpl();
 
   // Cancel all background compilation and wait for all tasks to finish. Call
   // this before destructing this object.
@@ -150,10 +149,10 @@ class CompilationStateImpl {
   void RestartBackgroundCompileTask();
   void RestartBackgroundTasks();
 
-  void SetError(uint32_t func_index, const WasmError& error);
+  void SetError();
 
   bool failed() const {
-    return compile_error_.load(std::memory_order_relaxed) != nullptr;
+    return compile_failed_.load(std::memory_order_relaxed);
   }
 
   bool baseline_compilation_finished() const {
@@ -166,27 +165,6 @@ class CompilationStateImpl {
   CompileMode compile_mode() const { return compile_mode_; }
   WasmFeatures* detected_features() { return &detected_features_; }
 
-  // Call {GetCompileError} from foreground threads only, since we access
-  // NativeModule::wire_bytes, which is set from the foreground thread once the
-  // stream has finished.
-  WasmError GetCompileError() {
-    CompilationError* error = compile_error_.load(std::memory_order_acquire);
-    DCHECK_NOT_NULL(error);
-    std::ostringstream error_msg;
-    error_msg << "Compiling wasm function \"";
-    wasm::ModuleWireBytes wire_bytes(native_module_->wire_bytes());
-    wasm::WireBytesRef name_ref = native_module_->module()->LookupFunctionName(
-        wire_bytes, error->func_index);
-    if (name_ref.is_set()) {
-      wasm::WasmName name = wire_bytes.GetNameOrNull(name_ref);
-      error_msg.write(name.start(), name.length());
-    } else {
-      error_msg << "wasm-function[" << error->func_index << "]";
-    }
-    error_msg << "\" failed: " << error->error.message();
-    return WasmError{error->error.offset(), error_msg.str()};
-  }
-
   void SetWireBytesStorage(
       std::shared_ptr<WireBytesStorage> wire_bytes_storage) {
     base::MutexGuard guard(&mutex_);
@@ -200,22 +178,14 @@ class CompilationStateImpl {
   }
 
  private:
-  struct CompilationError {
-    uint32_t const func_index;
-    WasmError const error;
-    CompilationError(uint32_t func_index, WasmError error)
-        : func_index(func_index), error(std::move(error)) {}
-  };
-
   NativeModule* const native_module_;
   const std::shared_ptr<BackgroundCompileToken> background_compile_token_;
   const CompileMode compile_mode_;
   const std::shared_ptr<Counters> async_counters_;
 
-  // Compilation error, atomically updated, but at most once (nullptr -> error).
-  // Uses acquire-release semantics (acquire on load, release on update).
-  // For checking whether an error is set, relaxed semantics can be used.
-  std::atomic<CompilationError*> compile_error_{nullptr};
+  // Compilation error, atomically updated. This flag can be updated and read
+  // using relaxed semantics.
+  std::atomic<bool> compile_failed_{false};
 
   // This mutex protects all information of this {CompilationStateImpl} which is
   // being accessed concurrently.
@@ -287,9 +257,7 @@ CompilationState::~CompilationState() { Impl(this)->~CompilationStateImpl(); }
 
 void CompilationState::AbortCompilation() { Impl(this)->AbortCompilation(); }
 
-void CompilationState::SetError(uint32_t func_index, const WasmError& error) {
-  Impl(this)->SetError(func_index, error);
-}
+void CompilationState::SetError() { Impl(this)->SetError(); }
 
 void CompilationState::SetWireBytesStorage(
     std::shared_ptr<WireBytesStorage> wire_bytes_storage) {
@@ -538,10 +506,7 @@ void CompileInParallel(Isolate* isolate, NativeModule* native_module) {
   compilation_state->PublishDetectedFeatures(isolate, detected_features);
 }
 
-void CompileSequentially(Isolate* isolate, NativeModule* native_module,
-                         ErrorThrower* thrower) {
-  DCHECK(!thrower->error());
-
+void CompileSequentially(Isolate* isolate, NativeModule* native_module) {
   ModuleWireBytes wire_bytes(native_module->wire_bytes());
   const WasmModule* module = native_module->module();
   WasmFeatures detected = kNoWasmFeatures;
@@ -554,10 +519,7 @@ void CompileSequentially(Isolate* isolate, NativeModule* native_module,
     // Compile the function.
     WasmCompilationUnit::CompileWasmFunction(isolate, native_module, &detected,
                                              &func, tier);
-    if (comp_state->failed()) {
-      thrower->CompileFailed(comp_state->GetCompileError());
-      break;
-    }
+    if (comp_state->failed()) break;
   }
   UpdateFeatureUseCounts(isolate, detected);
 }
@@ -566,16 +528,15 @@ void ValidateSequentially(Isolate* isolate, NativeModule* native_module,
                           ErrorThrower* thrower) {
   DCHECK(!thrower->error());
 
-  ModuleWireBytes wire_bytes(native_module->wire_bytes());
+  ModuleWireBytes wire_bytes{native_module->wire_bytes()};
   const WasmModule* module = native_module->module();
   uint32_t start = module->num_imported_functions;
   uint32_t end = start + module->num_declared_functions;
   for (uint32_t i = start; i < end; ++i) {
-    const WasmFunction& func = module->functions[i];
+    const WasmFunction* func = &module->functions[i];
 
-    const byte* base = wire_bytes.start();
-    FunctionBody body{func.sig, func.code.offset(), base + func.code.offset(),
-                      base + func.code.end_offset()};
+    Vector<const uint8_t> code = wire_bytes.GetFunctionBytes(func);
+    FunctionBody body{func->sig, func->code.offset(), code.start(), code.end()};
     DecodeResult result;
     {
       auto time_counter = SELECT_WASM_COUNTER(
@@ -588,11 +549,18 @@ void ValidateSequentially(Isolate* isolate, NativeModule* native_module,
                               &detected, body);
     }
     if (result.failed()) {
-      TruncatedUserString<> name(wire_bytes.GetNameOrNull(&func, module));
-      thrower->CompileError("Compiling function #%d:%.*s failed: %s @+%u", i,
-                            name.length(), name.start(),
-                            result.error().message().c_str(),
-                            result.error().offset());
+      WasmName name = wire_bytes.GetNameOrNull(func, module);
+      if (name.start() == nullptr) {
+        thrower->CompileError("Compiling function #%d failed: %s @+%u", i,
+                              result.error().message().c_str(),
+                              result.error().offset());
+      } else {
+        TruncatedUserString<> name(wire_bytes.GetNameOrNull(func, module));
+        thrower->CompileError("Compiling function #%d:\"%.*s\" failed: %s @+%u",
+                              i, name.length(), name.start(),
+                              result.error().message().c_str(),
+                              result.error().offset());
+      }
       break;
     }
   }
@@ -627,11 +595,12 @@ void CompileNativeModule(Isolate* isolate, ErrorThrower* thrower,
     if (compile_parallel) {
       CompileInParallel(isolate, native_module);
     } else {
-      CompileSequentially(isolate, native_module, thrower);
+      CompileSequentially(isolate, native_module);
     }
     auto* compilation_state = Impl(native_module->compilation_state());
     if (compilation_state->failed()) {
-      thrower->CompileFailed(compilation_state->GetCompileError());
+      ValidateSequentially(isolate, native_module, thrower);
+      CHECK(thrower->error());
     }
   }
 }
@@ -953,7 +922,7 @@ void AsyncCompileJob::FinishCompile() {
   FinishModule();
 }
 
-void AsyncCompileJob::AsyncCompileFailed(const WasmError& error) {
+void AsyncCompileJob::DecodeFailed(const WasmError& error) {
   ErrorThrower thrower(isolate_, "WebAssembly.compile()");
   thrower.CompileFailed(error);
   // {job} keeps the {this} pointer alive.
@@ -962,6 +931,16 @@ void AsyncCompileJob::AsyncCompileFailed(const WasmError& error) {
   resolver_->OnCompilationFailed(thrower.Reify());
 }
 
+void AsyncCompileJob::AsyncCompileFailed() {
+  ErrorThrower thrower(isolate_, "WebAssembly.compile()");
+  ValidateSequentially(isolate_, native_module_.get(), &thrower);
+  DCHECK(thrower.error());
+  // {job} keeps the {this} pointer alive.
+  std::shared_ptr<AsyncCompileJob> job =
+      isolate_->wasm_engine()->RemoveCompileJob(this);
+  resolver_->OnCompilationFailed(thrower.Reify());
+}
+
 void AsyncCompileJob::AsyncCompileSucceeded(Handle<WasmModuleObject> result) {
   resolver_->OnCompilationSucceeded(result);
 }
@@ -986,7 +965,9 @@ class AsyncCompileJob::CompilationStateCallback {
         break;
       case CompilationEvent::kFailedCompilation: {
         DCHECK(!last_event_.has_value());
-        job_->DoSync<CompileFailed>();
+        if (job_->DecrementAndCheckFinisherCount()) {
+          job_->DoSync<CompileFailed>();
+        }
         break;
       }
       default:
@@ -1173,8 +1154,8 @@ class AsyncCompileJob::DecodeFail : public CompileStep {
 
   void RunInForeground(AsyncCompileJob* job) override {
     TRACE_COMPILE("(1b) Decoding failed.\n");
-    // {job_} is deleted in AsyncCompileFailed, therefore the {return}.
-    return job->AsyncCompileFailed(error_);
+    // {job_} is deleted in DecodeFailed, therefore the {return}.
+    return job->DecodeFailed(error_);
   }
 };
 
@@ -1234,11 +1215,10 @@ class AsyncCompileJob::CompileFailed : public CompileStep {
  private:
   void RunInForeground(AsyncCompileJob* job) override {
     TRACE_COMPILE("(3a) Compilation failed\n");
+    DCHECK(job->native_module_->compilation_state()->failed());
 
-    WasmError error =
-        Impl(job->native_module_->compilation_state())->GetCompileError();
     // {job_} is deleted in AsyncCompileFailed, therefore the {return}.
-    return job->AsyncCompileFailed(error);
+    return job->AsyncCompileFailed();
   }
 };
 
@@ -1271,6 +1251,7 @@ class AsyncCompileJob::CompileFinished : public CompileStep {
  private:
   void RunInForeground(AsyncCompileJob* job) override {
     TRACE_COMPILE("(3b) Compilation finished\n");
+    DCHECK(!job->native_module_->compilation_state()->failed());
     // Sample the generated code size when baseline compilation finished.
     job->native_module_->SampleCodeSize(job->isolate_->counters(),
                                         NativeModule::kAfterBaseline);
@@ -1452,7 +1433,11 @@ void AsyncStreamingProcessor::OnFinishedStream(OwnedVector<uint8_t> bytes) {
   job_->wire_bytes_ = ModuleWireBytes(bytes.as_vector());
   job_->native_module_->SetWireBytes(std::move(bytes));
   if (needs_finish) {
-    job_->FinishCompile();
+    if (job_->native_module_->compilation_state()->failed()) {
+      job_->AsyncCompileFailed();
+    } else {
+      job_->FinishCompile();
+    }
   }
 }
 
@@ -1503,11 +1488,6 @@ CompilationStateImpl::CompilationStateImpl(
           1, std::min(FLAG_wasm_num_compilation_tasks,
                       V8::GetCurrentPlatform()->NumberOfWorkerThreads()))) {}
 
-CompilationStateImpl::~CompilationStateImpl() {
-  CompilationError* error = compile_error_.load(std::memory_order_acquire);
-  if (error != nullptr) delete error;
-}
-
 void CompilationStateImpl::AbortCompilation() {
   background_compile_token_->Cancel();
   // No more callbacks after abort.
@@ -1683,19 +1663,13 @@ void CompilationStateImpl::RestartBackgroundTasks() {
   }
 }
 
-void CompilationStateImpl::SetError(uint32_t func_index,
-                                    const WasmError& error) {
-  DCHECK(error.has_error());
-  std::unique_ptr<CompilationError> compile_error =
-      base::make_unique<CompilationError>(func_index, error);
-  CompilationError* expected = nullptr;
-  bool set = compile_error_.compare_exchange_strong(
-      expected, compile_error.get(), std::memory_order_acq_rel);
-  // Ignore all but the first error. If the previous value is not nullptr, just
-  // return (and free the allocated error).
-  if (!set) return;
-  // If set successfully, give up ownership.
-  compile_error.release();
+void CompilationStateImpl::SetError() {
+  bool expected = false;
+  if (!compile_failed_.compare_exchange_strong(expected, true,
+                                               std::memory_order_relaxed)) {
+    return;  // Already failed before.
+  }
+
   base::MutexGuard callbacks_guard(&callbacks_mutex_);
   for (auto& callback : callbacks_) {
     callback(CompilationEvent::kFailedCompilation);

src/wasm/module-compiler.h

@@ -109,7 +109,8 @@ class AsyncCompileJob {
 
   void FinishCompile();
 
-  void AsyncCompileFailed(const WasmError&);
+  void DecodeFailed(const WasmError&);
+  void AsyncCompileFailed();
 
   void AsyncCompileSucceeded(Handle<WasmModuleObject> result);
 
@@ -154,7 +155,7 @@ class AsyncCompileJob {
   // Copy of the module wire bytes, moved into the {native_module_} on its
   // creation.
   std::unique_ptr<byte[]> bytes_copy_;
-  // Reference to the wire bytes (hold in {bytes_copy_} or as part of
+  // Reference to the wire bytes (held in {bytes_copy_} or as part of
   // {native_module_}).
   ModuleWireBytes wire_bytes_;
   Handle<Context> native_context_;

test/mjsunit/wasm/async-compile.js

@@ -66,7 +66,7 @@ assertPromiseResult(async function badFunctionInTheMiddle() {
   let buffer = builder.toBuffer();
   await assertCompileError(
       buffer,
-      'Compiling wasm function \"bad\" failed: ' +
+      'Compiling function #10:\"bad\" failed: ' +
           'expected 1 elements on the stack for fallthru to @1, found 0 @+94');
 }());
 

test/mjsunit/wasm/errors.js

@@ -55,7 +55,7 @@ function assertConversionError(bytes, imports, msg) {
 
 (function TestValidationError() {
   print(arguments.callee.name);
-  let f_error = msg => 'Compiling wasm function "f" failed: ' + msg;
+  let f_error = msg => 'Compiling function #0:"f" failed: ' + msg;
   assertCompileError(
       builder().addFunction('f', kSig_i_v).end().toBuffer(),
       f_error('function body must end with "end" opcode @+24'));
@@ -176,7 +176,6 @@ function import_error(index, module, func, msg) {
   assertConversionError(buffer, {}, kTrapMsgs[kTrapTypeError]);
 })();
 
-
 (function InternalDebugTrace() {
   print(arguments.callee.name);
   var builder = new WasmModuleBuilder();
@@ -192,3 +191,18 @@ function import_error(index, module, func, msg) {
   }).exports.main;
   main();
 })();
+
+(function TestMultipleCorruptFunctions() {
+  print(arguments.callee.name);
+  // Generate a module with multiple corrupt functions. The error message must
+  // be deterministic.
+  var builder = new WasmModuleBuilder();
+  var sig = builder.addType(kSig_v_v);
+  for (let i = 0; i < 10; ++i) {
+    builder.addFunction('f' + i, sig).addBody([kExprEnd]);
+  }
+  assertCompileError(
+      builder.toBuffer(),
+      'Compiling function #0:"f0" failed: ' +
+          'trailing code after function end @+33');
+})();

test/mjsunit/wasm/unicode.js

@@ -51,7 +51,7 @@ checkExports('☺☺mul☺☺', '☺☺mul☺☺', '☺☺add☺☺', '☺☺add
   builder.addFunction('three snowmen: ☃☃☃', kSig_i_v).addBody([]).exportFunc();
   assertThrows(
       () => builder.instantiate(), WebAssembly.CompileError,
-      /Compiling wasm function "three snowmen: ☃☃☃" failed: /);
+      /Compiling function #0:"three snowmen: ☃☃☃" failed: /);
 })();
 
 (function errorMessageUnicodeInImportModuleName() {