[wasm] Split Result/ResultBase in WasmError and Result

We often use ResultBase or VoidResult to store or pass wasm errors
(errors with locations). This CL extracts a WasmError class which can
store an error (can also be empty), and Result<T> which stores an error
or a T (exactly one of them).

R=titzer@chromium.org

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

src/compiler/wasm-compiler.cc

@@ -5847,10 +5847,11 @@ bool TurbofanWasmCompilationUnit::BuildGraphForWasmFunction(
   if (graph_construction_result.failed()) {
     if (FLAG_trace_wasm_compiler) {
       StdoutStream{} << "Compilation failed: "
-                     << graph_construction_result.error_msg() << std::endl;
+                     << graph_construction_result.error().message()
+                     << std::endl;
     }
     native_module->compilation_state()->SetError(
-        wasm_unit_->func_index_, std::move(graph_construction_result));
+        wasm_unit_->func_index_, std::move(graph_construction_result).error());
     return false;
   }
 

src/wasm/compilation-environment.h

@@ -14,7 +14,7 @@ namespace internal {
 namespace wasm {
 
 class NativeModule;
-class ResultBase;
+class WasmError;
 
 enum RuntimeExceptionSupport : bool {
   kRuntimeExceptionSupport = true,
@@ -96,12 +96,12 @@ enum class CompilationEvent : uint8_t {
 // This is the PIMPL interface to that private class.
 class CompilationState {
  public:
-  using callback_t = std::function<void(CompilationEvent, const ResultBase*)>;
+  using callback_t = std::function<void(CompilationEvent, const WasmError*)>;
   ~CompilationState();
 
   void CancelAndWait();
 
-  void SetError(uint32_t func_index, const ResultBase& error_result);
+  void SetError(uint32_t func_index, const WasmError& error);
 
   void SetWireBytesStorage(std::shared_ptr<WireBytesStorage>);
 

src/wasm/decoder.h

@@ -218,10 +218,10 @@ class Decoder {
   template <typename T, typename U = typename std::remove_reference<T>::type>
   Result<U> toResult(T&& val) {
     if (failed()) {
-      TRACE("Result error: %s\n", error_msg_.c_str());
-      return Result<U>::Error(error_offset_, std::move(error_msg_));
+      TRACE("Result error: %s\n", error_.message().c_str());
+      return Result<U>{error_};
     }
-    return Result<U>(std::forward<T>(val));
+    return Result<U>{std::forward<T>(val)};
   }
 
   // Resets the boundaries of this decoder.
@@ -232,17 +232,17 @@ class Decoder {
     pc_ = start;
     end_ = end;
     buffer_offset_ = buffer_offset;
-    error_offset_ = 0;
-    error_msg_.clear();
+    error_ = {};
   }
 
   void Reset(Vector<const uint8_t> bytes, uint32_t buffer_offset = 0) {
     Reset(bytes.begin(), bytes.end(), buffer_offset);
   }
 
-  bool ok() const { return error_msg_.empty(); }
+  bool ok() const { return error_.empty(); }
   bool failed() const { return !ok(); }
   bool more() const { return pc_ < end_; }
+  const WasmError& error() const { return error_; }
 
   const byte* start() const { return start_; }
   const byte* pc() const { return pc_; }
@@ -271,8 +271,7 @@ class Decoder {
   const byte* end_;
   // The offset of the current buffer in the module. Needed for streaming.
   uint32_t buffer_offset_;
-  uint32_t error_offset_ = 0;
-  std::string error_msg_;
+  WasmError error_;
 
  private:
   void verrorf(uint32_t offset, const char* format, va_list args) {
@@ -287,8 +286,7 @@ class Decoder {
     EmbeddedVector<char, kMaxErrorMsg> buffer;
     int len = VSNPrintF(buffer, format, args);
     CHECK_LT(0, len);
-    error_msg_.assign(buffer.start(), len);
-    error_offset_ = offset;
+    error_ = {offset, {buffer.start(), static_cast<size_t>(len)}};
     onFirstError();
   }
 

src/wasm/function-body-decoder-impl.h

@@ -1423,9 +1423,9 @@ class WasmFullDecoder : public WasmDecoder<validate> {
   }
 
   bool TraceFailed() {
-    TRACE("wasm-error module+%-6d func+%d: %s\n\n", this->error_offset_,
-          this->GetBufferRelativeOffset(this->error_offset_),
-          this->error_msg_.c_str());
+    TRACE("wasm-error module+%-6d func+%d: %s\n\n", this->error_.offset(),
+          this->GetBufferRelativeOffset(this->error_.offset()),
+          this->error_.message().c_str());
     return false;
   }
 
@@ -2707,7 +2707,7 @@ class WasmFullDecoder : public WasmDecoder<validate> {
 
   void onFirstError() override {
     this->end_ = this->pc_;  // Terminate decoding loop.
-    TRACE(" !%s\n", this->error_msg_.c_str());
+    TRACE(" !%s\n", this->error_.message().c_str());
     CALL_INTERFACE(OnFirstError);
   }
 

src/wasm/module-decoder.cc

@@ -1000,9 +1000,9 @@ class ModuleDecoderImpl : public Decoder {
       CalculateGlobalOffsets(module_.get());
     }
     ModuleResult result = toResult(std::move(module_));
-    if (verify_functions && result.ok() && intermediate_result_.failed()) {
-      // Copy error code and location.
-      result = ModuleResult::ErrorFrom(std::move(intermediate_result_));
+    if (verify_functions && result.ok() && intermediate_error_.has_error()) {
+      // Copy error message and location.
+      return ModuleResult{std::move(intermediate_error_)};
     }
     return result;
   }
@@ -1057,8 +1057,8 @@ class ModuleDecoderImpl : public Decoder {
       VerifyFunctionBody(zone->allocator(), 0, wire_bytes, module,
                          function.get());
 
-    if (intermediate_result_.failed()) {
-      return FunctionResult::ErrorFrom(std::move(intermediate_result_));
+    if (intermediate_error_.has_error()) {
+      return FunctionResult{std::move(intermediate_error_)};
     }
 
     return FunctionResult(std::move(function));
@@ -1105,7 +1105,7 @@ class ModuleDecoderImpl : public Decoder {
                         sizeof(ModuleDecoderImpl::seen_unordered_sections_) >
                     kLastKnownModuleSection,
                 "not enough bits");
-  VoidResult intermediate_result_;
+  WasmError intermediate_error_;
   ModuleOrigin origin_;
 
   bool has_seen_unordered_section(SectionCode section_code) {
@@ -1224,12 +1224,12 @@ class ModuleDecoderImpl : public Decoder {
 
     // If the decode failed and this is the first error, set error code and
     // location.
-    if (result.failed() && intermediate_result_.ok()) {
+    if (result.failed() && intermediate_error_.empty()) {
       // Wrap the error message from the function decoder.
       std::ostringstream error_msg;
-      error_msg << "in function " << func_name << ": " << result.error_msg();
-      intermediate_result_ =
-          VoidResult::Error(result.error_offset(), error_msg.str());
+      error_msg << "in function " << func_name << ": "
+                << result.error().message();
+      intermediate_error_ = WasmError{result.error().offset(), error_msg.str()};
     }
   }
 
@@ -1620,8 +1620,8 @@ ModuleResult DecodeWasmModule(const WasmFeatures& enabled,
   size_t size = module_end - module_start;
   CHECK_LE(module_start, module_end);
   if (size >= kV8MaxWasmModuleSize) {
-    return ModuleResult::Error(0, "size > maximum module size (%zu): %zu",
-                               kV8MaxWasmModuleSize, size);
+    return ModuleResult{WasmError{0, "size > maximum module size (%zu): %zu",
+                                  kV8MaxWasmModuleSize, size}};
   }
   // TODO(bradnelson): Improve histogram handling of size_t.
   auto size_counter =
@@ -1740,8 +1740,9 @@ FunctionResult DecodeWasmFunctionForTesting(
   // TODO(bradnelson): Improve histogram handling of ptrdiff_t.
   size_histogram->AddSample(static_cast<int>(size));
   if (size > kV8MaxWasmFunctionSize) {
-    return FunctionResult::Error(0, "size > maximum function size (%zu): %zu",
-                                 kV8MaxWasmFunctionSize, size);
+    return FunctionResult{WasmError{0,
+                                    "size > maximum function size (%zu): %zu",
+                                    kV8MaxWasmFunctionSize, size}};
   }
   ModuleDecoderImpl decoder(enabled, function_start, function_end, kWasmOrigin);
   decoder.SetCounters(counters);

src/wasm/streaming-decoder.cc

@@ -128,10 +128,9 @@ class TopTierCompiledCallback {
       : native_module_(std::move(native_module)),
         callback_(std::move(callback)) {}
 
-  void operator()(CompilationEvent event,
-                  const ResultBase* error_result) const {
+  void operator()(CompilationEvent event, const WasmError* error) const {
     if (event != CompilationEvent::kFinishedTopTierCompilation) return;
-    DCHECK_NULL(error_result);
+    DCHECK_NULL(error);
     callback_(native_module_);
 #ifdef DEBUG
     DCHECK(!called_);
@@ -327,7 +326,7 @@ size_t StreamingDecoder::DecodeVarInt32::ReadBytes(
   if (decoder.failed()) {
     if (new_bytes == remaining_buf.size()) {
       // We only report an error if we read all bytes.
-      streaming->Error(decoder.toResult(nullptr));
+      streaming->Error(decoder.error());
     }
     set_offset(offset() + new_bytes);
     return new_bytes;

src/wasm/streaming-decoder.h

@@ -51,7 +51,7 @@ class V8_EXPORT_PRIVATE StreamingProcessor {
   // empty array is passed.
   virtual void OnFinishedStream(OwnedVector<uint8_t> bytes) = 0;
   // Report an error detected in the StreamingDecoder.
-  virtual void OnError(VoidResult result) = 0;
+  virtual void OnError(const WasmError&) = 0;
   // Report the abortion of the stream.
   virtual void OnAbort() = 0;
 
@@ -202,14 +202,14 @@ class V8_EXPORT_PRIVATE StreamingDecoder {
                                  size_t length,
                                  Vector<const uint8_t> length_bytes);
 
-  std::unique_ptr<DecodingState> Error(VoidResult result) {
-    if (ok()) processor_->OnError(std::move(result));
+  std::unique_ptr<DecodingState> Error(const WasmError& error) {
+    if (ok()) processor_->OnError(error);
     Fail();
     return std::unique_ptr<DecodingState>(nullptr);
   }
 
   std::unique_ptr<DecodingState> Error(std::string message) {
-    return Error(VoidResult::Error(module_offset_ - 1, std::move(message)));
+    return Error(WasmError{module_offset_ - 1, std::move(message)});
   }
 
   void ProcessModuleHeader() {

src/wasm/wasm-engine.cc

@@ -93,7 +93,7 @@ MaybeHandle<WasmModuleObject> WasmEngine::SyncCompile(
       DecodeWasmModule(enabled, bytes.start(), bytes.end(), false, kWasmOrigin,
                        isolate->counters(), allocator());
   if (result.failed()) {
-    thrower->CompileFailed("Wasm decoding failed", result);
+    thrower->CompileFailed("Wasm decoding failed", result.error());
     return {};
   }
 

src/wasm/wasm-result.cc

@@ -50,7 +50,7 @@ void PrintFToString(std::string& str, size_t str_offset, const char* format,
 }  // namespace
 
 // static
-std::string ResultBase::FormatError(const char* format, va_list args) {
+std::string WasmError::FormatError(const char* format, va_list args) {
   std::string result;
   VPrintFToString(result, 0, format, args);
   return result;

src/wasm/wasm-result.h

@@ -22,42 +22,44 @@ class Handle;
 
 namespace wasm {
 
-// Base class for Result<T>.
-class V8_EXPORT_PRIVATE ResultBase {
- protected:
-  ResultBase() = default;
+class V8_EXPORT_PRIVATE WasmError {
+ public:
+  WasmError() = default;
 
-  ResultBase& operator=(ResultBase&& other) V8_NOEXCEPT = default;
+  WasmError(uint32_t offset, std::string message)
+      : offset_(offset), message_(std::move(message)) {
+    // The error message must not be empty, otherwise {empty()} would be true.
+    DCHECK(!message_.empty());
+  }
 
- public:
-  ResultBase(ResultBase&& other) V8_NOEXCEPT
-      : error_offset_(other.error_offset_),
-        error_msg_(std::move(other.error_msg_)) {}
+  PRINTF_FORMAT(3, 4)
+  WasmError(uint32_t offset, const char* format, ...) : offset_(offset) {
+    va_list args;
+    va_start(args, format);
+    message_ = FormatError(format, args);
+    va_end(args);
+    // The error message must not be empty, otherwise {empty()} would be true.
+    DCHECK(!message_.empty());
+  }
 
-  bool ok() const { return error_msg_.empty(); }
-  bool failed() const { return !ok(); }
+  bool empty() const { return message_.empty(); }
+  bool has_error() const { return !message_.empty(); }
 
-  uint32_t error_offset() const { return error_offset_; }
-  const std::string& error_msg() const & { return error_msg_; }
-  std::string&& error_msg() && { return std::move(error_msg_); }
+  uint32_t offset() const { return offset_; }
+  const std::string& message() const& { return message_; }
+  std::string&& message() && { return std::move(message_); }
 
  protected:
-  ResultBase(uint32_t error_offset, std::string error_msg)
-      : error_offset_(error_offset), error_msg_(std::move(error_msg)) {
-    // The error message must not be empty, otherwise {failed()} will be false.
-    DCHECK(!error_msg_.empty());
-  }
-
   static std::string FormatError(const char* format, va_list args);
 
  private:
-  uint32_t error_offset_ = 0;
-  std::string error_msg_;
+  uint32_t offset_ = 0;
+  std::string message_;
 };
 
-// The overall result of decoding a function or a module.
+// Either a result of type T, or a WasmError.
 template <typename T>
-class Result : public ResultBase {
+class Result {
  public:
   Result() = default;
 
@@ -65,33 +67,22 @@ class Result : public ResultBase {
   explicit Result(S&& value) : value_(std::forward<S>(value)) {}
 
   template <typename S>
-  Result(Result<S>&& other) V8_NOEXCEPT : ResultBase(std::move(other)),
-                                          value_(std::move(other).value()) {}
+  Result(Result<S>&& other) V8_NOEXCEPT : value_(std::move(other.value_)),
+                                          error_(std::move(other.error_)) {}
 
-  Result& operator=(Result&& other) V8_NOEXCEPT = default;
+  explicit Result(WasmError error) : error_(std::move(error)) {}
 
-  static Result<T> PRINTF_FORMAT(2, 3)
-      Error(uint32_t offset, const char* format, ...) {
-    va_list args;
-    va_start(args, format);
-    Result<T> error_result{offset, FormatError(format, args)};
-    va_end(args);
-    return error_result;
-  }
-
-  static Result<T> Error(uint32_t error_offset, std::string error_msg) {
-    // Call private constructor.
-    return Result<T>{error_offset, std::move(error_msg)};
-  }
-
-  static Result<T> ErrorFrom(ResultBase&& error_result) {
-    return Error(error_result.error_offset(),
-                 std::move(error_result).error_msg());
+  template <typename S>
+  Result& operator=(Result<S>&& other) V8_NOEXCEPT {
+    value_ = std::move(other.value_);
+    error_ = std::move(other.error_);
+    return *this;
   }
 
-  static Result<T> ErrorFrom(const ResultBase& error_result) {
-    return Error(error_result.error_offset(), error_result.error_msg());
-  }
+  bool ok() const { return error_.empty(); }
+  bool failed() const { return error_.has_error(); }
+  const WasmError& error() const& { return error_; }
+  WasmError&& error() && { return std::move(error_); }
 
   // 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
@@ -107,10 +98,11 @@ class Result : public ResultBase {
   }
 
  private:
-  T value_ = T{};
+  template <typename S>
+  friend class Result;
 
-  Result(uint32_t error_offset, std::string error_msg)
-      : ResultBase(error_offset, std::move(error_msg)) {}
+  T value_ = T{};
+  WasmError error_;
 
   DISALLOW_COPY_AND_ASSIGN(Result);
 };
@@ -130,15 +122,15 @@ 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) {
-    DCHECK(result.failed());
-    CompileError("%s: %s @+%u", error, result.error_msg().c_str(),
-                 result.error_offset());
+  void CompileFailed(const char* context, const WasmError& error) {
+    DCHECK(error.has_error());
+    CompileError("%s: %s @+%u", context, error.message().c_str(),
+                 error.offset());
   }
 
-  void CompileFailed(const ResultBase& result) {
-    DCHECK(result.failed());
-    CompileError("%s @+%u", result.error_msg().c_str(), result.error_offset());
+  void CompileFailed(const WasmError& error) {
+    DCHECK(error.has_error());
+    CompileError("%s @+%u", error.message().c_str(), error.offset());
   }
 
   // Create and return exception object.

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

@@ -241,9 +241,8 @@ void TestBuildingGraphWithBuilder(compiler::WasmGraphBuilder* builder,
     }
 #endif
 
-    uint32_t pc = result.error_offset();
-    FATAL("Verification failed; pc = +%x, msg = %s", pc,
-          result.error_msg().c_str());
+    FATAL("Verification failed; pc = +%x, msg = %s", result.error().offset(),
+          result.error().message().c_str());
   }
   builder->LowerInt64();
   if (!CpuFeatures::SupportsWasmSimd128()) {

test/common/wasm/wasm-module-runner.cc

@@ -51,7 +51,7 @@ std::shared_ptr<WasmModule> DecodeWasmModuleForTesting(
   if (decoding_result.failed()) {
     // Module verification failed. throw.
     thrower->CompileError("DecodeWasmModule failed: %s",
-                          decoding_result.error_msg().c_str());
+                          decoding_result.error().message().c_str());
   }
 
   return std::move(decoding_result).value();

test/unittests/wasm/function-body-decoder-unittest.cc

@@ -138,17 +138,16 @@ class FunctionBodyDecoderTest : public TestWithZone {
         VerifyWasmCode(zone()->allocator(), enabled_features_, module,
                        &unused_detected_features, body);
 
-    uint32_t pc = result.error_offset();
     std::ostringstream str;
-    if (expected_success) {
-      str << "Verification failed: pc = +" << pc
-          << ", msg = " << result.error_msg();
+    if (result.failed()) {
+      str << "Verification failed: pc = +" << result.error().offset()
+          << ", msg = " << result.error().message();
     } else {
-      str << "Verification successed, expected failure; pc = +" << pc;
+      str << "Verification successed, expected failure";
     }
     EXPECT_EQ(result.ok(), expected_success) << str.str();
-    if (!expected_success && message) {
-      EXPECT_THAT(result.error_msg(), ::testing::HasSubstr(message));
+    if (result.failed() && message) {
+      EXPECT_THAT(result.error().message(), ::testing::HasSubstr(message));
     }
   }
 

test/unittests/wasm/module-decoder-unittest.cc

@@ -135,10 +135,10 @@ struct CheckLEB1 : std::integral_constant<size_t, num> {
     if (!result.ok()) return; \
   } while (false)
 
-#define EXPECT_NOT_OK(result, msg)                   \
-  do {                                               \
-    EXPECT_FALSE(result.ok());                       \
-    EXPECT_THAT(result.error_msg(), HasSubstr(msg)); \
+#define EXPECT_NOT_OK(result, msg)                         \
+  do {                                                     \
+    EXPECT_FALSE(result.ok());                             \
+    EXPECT_THAT(result.error().message(), HasSubstr(msg)); \
   } while (false)
 
 static size_t SizeOfVarInt(size_t value) {

test/unittests/wasm/streaming-decoder-unittest.cc

@@ -74,7 +74,7 @@ class MockStreamingProcessor : public StreamingProcessor {
   }
 
   // Report an error detected in the StreamingDecoder.
-  void OnError(DecodeResult result) override { result_->ok = false; }
+  void OnError(const WasmError&) override { result_->ok = false; }
 
   void OnAbort() override {}