From b538e15548cf3760c71e4b60cf3820c902748c8f Mon Sep 17 00:00:00 2001
From: Ali Mohammad Pur <ali.mpfard@gmail.com>
Date: Thu, 1 Jul 2021 17:03:17 +0430
Subject: [PATCH] LibWasm: Give traps a reason and display it when needed

This makes debugging wasm code a bit easier, as we now know what fails
instead of just "too bad, something went wrong".
---
 Tests/LibWasm/test-wasm.cpp                   |  2 +-
 .../AbstractMachine/AbstractMachine.cpp       |  8 ++--
 .../LibWasm/AbstractMachine/AbstractMachine.h | 19 ++++----
 .../AbstractMachine/BytecodeInterpreter.cpp   | 44 +++++++++----------
 .../AbstractMachine/BytecodeInterpreter.h     | 14 +++---
 .../LibWasm/AbstractMachine/Configuration.cpp |  6 +--
 .../LibWasm/AbstractMachine/Interpreter.h     |  1 +
 Userland/Libraries/LibWasm/Constants.h        |  2 +-
 .../LibWeb/WebAssembly/WebAssemblyObject.cpp  |  2 +-
 Userland/Utilities/wasm.cpp                   |  6 +--
 10 files changed, 54 insertions(+), 50 deletions(-)

diff --git a/Tests/LibWasm/test-wasm.cpp b/Tests/LibWasm/test-wasm.cpp
index 24563898c25..f2cea9f057c 100644
--- a/Tests/LibWasm/test-wasm.cpp
+++ b/Tests/LibWasm/test-wasm.cpp
@@ -254,7 +254,7 @@ JS_DEFINE_NATIVE_FUNCTION(WebAssemblyModule::wasm_invoke)
 
     auto result = WebAssemblyModule::machine().invoke(function_address, arguments);
     if (result.is_trap()) {
-        vm.throw_exception<JS::TypeError>(global_object, "Execution trapped");
+        vm.throw_exception<JS::TypeError>(global_object, String::formatted("Execution trapped: {}", result.trap().reason));
         return {};
     }
 
diff --git a/Userland/Libraries/LibWasm/AbstractMachine/AbstractMachine.cpp b/Userland/Libraries/LibWasm/AbstractMachine/AbstractMachine.cpp
index 49580e31b15..6a84f52361e 100644
--- a/Userland/Libraries/LibWasm/AbstractMachine/AbstractMachine.cpp
+++ b/Userland/Libraries/LibWasm/AbstractMachine/AbstractMachine.cpp
@@ -136,7 +136,7 @@ InstantiationResult AbstractMachine::instantiate(Module const& module, Vector<Ex
             });
             auto result = config.execute(interpreter);
             if (result.is_trap())
-                instantiation_result = InstantiationError { "Global value construction trapped" };
+                instantiation_result = InstantiationError { String::formatted("Global value construction trapped: {}", result.trap().reason) };
             else
                 global_values.append(result.values().first());
         }
@@ -161,7 +161,7 @@ InstantiationResult AbstractMachine::instantiate(Module const& module, Vector<Ex
                 });
                 auto result = config.execute(interpreter);
                 if (result.is_trap()) {
-                    instantiation_result = InstantiationError { "Element construction trapped" };
+                    instantiation_result = InstantiationError { String::formatted("Element construction trapped: {}", result.trap().reason) };
                     return IterationDecision::Continue;
                 }
 
@@ -212,7 +212,7 @@ InstantiationResult AbstractMachine::instantiate(Module const& module, Vector<Ex
             });
             auto result = config.execute(interpreter);
             if (result.is_trap()) {
-                instantiation_result = InstantiationError { "Element section initialisation trapped" };
+                instantiation_result = InstantiationError { String::formatted("Element section initialisation trapped: {}", result.trap().reason) };
                 return IterationDecision::Break;
             }
             auto d = result.values().first().to<i32>();
@@ -270,7 +270,7 @@ InstantiationResult AbstractMachine::instantiate(Module const& module, Vector<Ex
                     });
                     auto result = config.execute(interpreter);
                     if (result.is_trap()) {
-                        instantiation_result = InstantiationError { "Data section initialisation trapped" };
+                        instantiation_result = InstantiationError { String::formatted("Data section initialisation trapped: {}", result.trap().reason) };
                         return;
                     }
                     size_t offset = 0;
diff --git a/Userland/Libraries/LibWasm/AbstractMachine/AbstractMachine.h b/Userland/Libraries/LibWasm/AbstractMachine/AbstractMachine.h
index 1d3bd0a7760..3302b148fa5 100644
--- a/Userland/Libraries/LibWasm/AbstractMachine/AbstractMachine.h
+++ b/Userland/Libraries/LibWasm/AbstractMachine/AbstractMachine.h
@@ -193,28 +193,29 @@ private:
 };
 
 struct Trap {
-    // Empty value type
+    String reason;
 };
 
 class Result {
 public:
     explicit Result(Vector<Value> values)
-        : m_values(move(values))
+        : m_result(move(values))
     {
     }
 
-    Result(Trap)
-        : m_is_trap(true)
+    Result(Trap trap)
+        : m_result(move(trap))
     {
     }
 
-    auto& values() const { return m_values; }
-    auto& values() { return m_values; }
-    auto is_trap() const { return m_is_trap; }
+    auto is_trap() const { return m_result.has<Trap>(); }
+    auto& values() const { return m_result.get<Vector<Value>>(); }
+    auto& values() { return m_result.get<Vector<Value>>(); }
+    auto& trap() const { return m_result.get<Trap>(); }
+    auto& trap() { return m_result.get<Trap>(); }
 
 private:
-    Vector<Value> m_values;
-    bool m_is_trap { false };
+    Variant<Vector<Value>, Trap> m_result;
 };
 
 using ExternValue = Variant<FunctionAddress, TableAddress, MemoryAddress, GlobalAddress>;
diff --git a/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.cpp b/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.cpp
index 3b0d3dd12ce..7244b591c55 100644
--- a/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.cpp
+++ b/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.cpp
@@ -17,7 +17,7 @@ namespace Wasm {
 
 #define TRAP_IF_NOT(x)                                                                         \
     do {                                                                                       \
-        if (trap_if_not(x)) {                                                                  \
+        if (trap_if_not(x, #x)) {                                                              \
             dbgln_if(WASM_TRACE_DEBUG, "Trapped because {} failed, at line {}", #x, __LINE__); \
             return;                                                                            \
         }                                                                                      \
@@ -25,14 +25,14 @@ namespace Wasm {
 
 #define TRAP_IF_NOT_NORETURN(x)                                                                \
     do {                                                                                       \
-        if (trap_if_not(x)) {                                                                  \
+        if (trap_if_not(x, #x)) {                                                              \
             dbgln_if(WASM_TRACE_DEBUG, "Trapped because {} failed, at line {}", #x, __LINE__); \
         }                                                                                      \
     } while (false)
 
 void BytecodeInterpreter::interpret(Configuration& configuration)
 {
-    m_do_trap = false;
+    m_trap.clear();
     auto& instructions = configuration.frame().expression().instructions();
     auto max_ip_value = InstructionPointer { instructions.size() };
     auto& current_ip_value = configuration.ip();
@@ -40,13 +40,13 @@ void BytecodeInterpreter::interpret(Configuration& configuration)
 
     while (current_ip_value < max_ip_value) {
         if (executed_instructions++ >= Constants::max_allowed_executed_instructions_per_call) [[unlikely]] {
-            m_do_trap = true;
+            m_trap = Trap { "Exceeded maximum allowed number of instructions" };
             return;
         }
         auto& instruction = instructions[current_ip_value.value()];
         auto old_ip = current_ip_value;
         interpret(configuration, current_ip_value, instruction);
-        if (m_do_trap)
+        if (m_trap.has_value())
             return;
         if (current_ip_value == old_ip) // If no jump occurred
             ++current_ip_value;
@@ -83,7 +83,7 @@ void BytecodeInterpreter::load_and_push(Configuration& configuration, Instructio
     auto& address = configuration.frame().module().memories().first();
     auto memory = configuration.store().get(address);
     if (!memory) {
-        m_do_trap = true;
+        m_trap = Trap { "Nonexistent memory" };
         return;
     }
     auto& arg = instruction.arguments().get<Instruction::MemoryArgument>();
@@ -91,12 +91,12 @@ void BytecodeInterpreter::load_and_push(Configuration& configuration, Instructio
     TRAP_IF_NOT(entry.has<Value>());
     auto base = entry.get<Value>().to<i32>();
     if (!base.has_value()) {
-        m_do_trap = true;
+        m_trap = Trap { "Memory access out of bounds" };
         return;
     }
     auto instance_address = base.value() + static_cast<i64>(arg.offset);
     if (instance_address < 0 || static_cast<u64>(instance_address + sizeof(ReadType)) > memory->size()) {
-        m_do_trap = true;
+        m_trap = Trap { "Memory access out of bounds" };
         dbgln("LibWasm: Memory access out of bounds (expected 0 <= {} and {} <= {})", instance_address, instance_address + sizeof(ReadType), memory->size());
         return;
     }
@@ -117,7 +117,7 @@ void BytecodeInterpreter::store_to_memory(Configuration& configuration, Instruct
     TRAP_IF_NOT(base.has_value());
     auto instance_address = base.value() + static_cast<i64>(arg.offset);
     if (instance_address < 0 || static_cast<u64>(instance_address + data.size()) > memory->size()) {
-        m_do_trap = true;
+        m_trap = Trap { "Memory access out of bounds" };
         dbgln("LibWasm: Memory access out of bounds (expected 0 <= {} and {} <= {})", instance_address, instance_address + data.size(), memory->size());
         return;
     }
@@ -146,14 +146,14 @@ void BytecodeInterpreter::call_address(Configuration& configuration, FunctionAdd
 
     configuration.stack().entries().remove(configuration.stack().size() - span.size(), span.size());
 
-    Result result { Trap {} };
+    Result result { Trap { ""sv } };
     {
         CallFrameHandle handle { *this, configuration };
         result = configuration.call(*this, address, move(args));
     }
 
     if (result.is_trap()) {
-        m_do_trap = true;
+        m_trap = move(result.trap());
         return;
     }
 
@@ -254,7 +254,7 @@ T BytecodeInterpreter::read_value(ReadonlyBytes data)
     stream >> value;
     if (stream.handle_any_error()) {
         dbgln("Read from {} failed", data.data());
-        m_do_trap = true;
+        m_trap = Trap { "Read from memory failed" };
     }
     return value;
 }
@@ -266,7 +266,7 @@ float BytecodeInterpreter::read_value<float>(ReadonlyBytes data)
     LittleEndian<u32> raw_value;
     stream >> raw_value;
     if (stream.handle_any_error())
-        m_do_trap = true;
+        m_trap = Trap { "Read from memory failed" };
     return bit_cast<float>(static_cast<u32>(raw_value));
 }
 
@@ -277,7 +277,7 @@ double BytecodeInterpreter::read_value<double>(ReadonlyBytes data)
     LittleEndian<u64> raw_value;
     stream >> raw_value;
     if (stream.handle_any_error())
-        m_do_trap = true;
+        m_trap = Trap { "Read from memory failed" };
     return bit_cast<double>(static_cast<u64>(raw_value));
 }
 
@@ -319,7 +319,7 @@ template<typename V, typename T>
 MakeSigned<T> BytecodeInterpreter::checked_signed_truncate(V value)
 {
     if (isnan(value) || isinf(value)) { // "undefined", let's just trap.
-        m_do_trap = true;
+        m_trap = Trap { "Signed truncation undefined behaviour" };
         return 0;
     }
 
@@ -334,7 +334,7 @@ MakeSigned<T> BytecodeInterpreter::checked_signed_truncate(V value)
         return static_cast<SignedT>(truncated);
 
     dbgln_if(WASM_TRACE_DEBUG, "Truncate out of range error");
-    m_do_trap = true;
+    m_trap = Trap { "Signed truncation out of range" };
     return true;
 }
 
@@ -342,7 +342,7 @@ template<typename V, typename T>
 MakeUnsigned<T> BytecodeInterpreter::checked_unsigned_truncate(V value)
 {
     if (isnan(value) || isinf(value)) { // "undefined", let's just trap.
-        m_do_trap = true;
+        m_trap = Trap { "Unsigned truncation undefined behaviour" };
         return 0;
     }
     double truncated;
@@ -356,7 +356,7 @@ MakeUnsigned<T> BytecodeInterpreter::checked_unsigned_truncate(V value)
         return static_cast<UnsignedT>(truncated);
 
     dbgln_if(WASM_TRACE_DEBUG, "Truncate out of range error");
-    m_do_trap = true;
+    m_trap = Trap { "Unsigned truncation out of range" };
     return true;
 }
 
@@ -490,7 +490,7 @@ void BytecodeInterpreter::interpret(Configuration& configuration, InstructionPoi
 
     switch (instruction.opcode().value()) {
     case Instructions::unreachable.value():
-        m_do_trap = true;
+        m_trap = Trap { "Unreachable" };
         return;
     case Instructions::nop.value():
         return;
@@ -1087,7 +1087,7 @@ void BytecodeInterpreter::interpret(Configuration& configuration, InstructionPoi
     default:
     unimplemented:;
         dbgln("Instruction '{}' not implemented", instruction_name(instruction.opcode()));
-        m_do_trap = true;
+        m_trap = Trap { String::formatted("Unimplemented instruction {}", instruction_name(instruction.opcode())) };
         return;
     }
 }
@@ -1097,7 +1097,7 @@ void DebuggerBytecodeInterpreter::interpret(Configuration& configuration, Instru
     if (pre_interpret_hook) {
         auto result = pre_interpret_hook(configuration, ip, instruction);
         if (!result) {
-            m_do_trap = true;
+            m_trap = Trap { "Trapped by user request" };
             return;
         }
     }
@@ -1106,7 +1106,7 @@ void DebuggerBytecodeInterpreter::interpret(Configuration& configuration, Instru
         if (post_interpret_hook) {
             auto result = post_interpret_hook(configuration, ip, instruction, *this);
             if (!result) {
-                m_do_trap = true;
+                m_trap = Trap { "Trapped by user request" };
                 return;
             }
         }
diff --git a/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.h b/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.h
index d494511cfe9..7a917ab6fc5 100644
--- a/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.h
+++ b/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.h
@@ -14,8 +14,9 @@ namespace Wasm {
 struct BytecodeInterpreter : public Interpreter {
     virtual void interpret(Configuration&) override;
     virtual ~BytecodeInterpreter() override = default;
-    virtual bool did_trap() const override { return m_do_trap; }
-    virtual void clear_trap() override { m_do_trap = false; }
+    virtual bool did_trap() const override { return m_trap.has_value(); }
+    virtual String trap_reason() const override { return m_trap.value().reason; }
+    virtual void clear_trap() override { m_trap.clear(); }
 
     struct CallFrameHandle {
         explicit CallFrameHandle(BytecodeInterpreter& interpreter, Configuration& configuration)
@@ -48,13 +49,14 @@ protected:
     T read_value(ReadonlyBytes data);
 
     Vector<Value> pop_values(Configuration& configuration, size_t count);
-    bool trap_if_not(bool value)
+    bool trap_if_not(bool value, String reason)
     {
         if (!value)
-            m_do_trap = true;
-        return m_do_trap;
+            m_trap = Trap { move(reason) };
+        return m_trap.has_value();
     }
-    bool m_do_trap { false };
+
+    Optional<Trap> m_trap;
 };
 
 struct DebuggerBytecodeInterpreter : public BytecodeInterpreter {
diff --git a/Userland/Libraries/LibWasm/AbstractMachine/Configuration.cpp b/Userland/Libraries/LibWasm/AbstractMachine/Configuration.cpp
index 8e93ca5af0e..ed7c925b19a 100644
--- a/Userland/Libraries/LibWasm/AbstractMachine/Configuration.cpp
+++ b/Userland/Libraries/LibWasm/AbstractMachine/Configuration.cpp
@@ -68,10 +68,10 @@ Result Configuration::execute(Interpreter& interpreter)
 {
     interpreter.interpret(*this);
     if (interpreter.did_trap())
-        return Trap {};
+        return Trap { interpreter.trap_reason() };
 
     if (stack().size() <= frame().arity() + 1)
-        return Trap {};
+        return Trap { "Not enough values to return from call" };
 
     Vector<Value> results;
     results.ensure_capacity(frame().arity());
@@ -80,7 +80,7 @@ Result Configuration::execute(Interpreter& interpreter)
     auto label = stack().pop();
     // ASSERT: label == current frame
     if (!label.has<Label>())
-        return Trap {};
+        return Trap { "Invalid stack configuration" };
     return Result { move(results) };
 }
 
diff --git a/Userland/Libraries/LibWasm/AbstractMachine/Interpreter.h b/Userland/Libraries/LibWasm/AbstractMachine/Interpreter.h
index 0f17d4f1bff..2cbe9889824 100644
--- a/Userland/Libraries/LibWasm/AbstractMachine/Interpreter.h
+++ b/Userland/Libraries/LibWasm/AbstractMachine/Interpreter.h
@@ -14,6 +14,7 @@ struct Interpreter {
     virtual ~Interpreter() = default;
     virtual void interpret(Configuration&) = 0;
     virtual bool did_trap() const = 0;
+    virtual String trap_reason() const = 0;
     virtual void clear_trap() = 0;
 };
 
diff --git a/Userland/Libraries/LibWasm/Constants.h b/Userland/Libraries/LibWasm/Constants.h
index 51438ce5229..b228c148632 100644
--- a/Userland/Libraries/LibWasm/Constants.h
+++ b/Userland/Libraries/LibWasm/Constants.h
@@ -38,6 +38,6 @@ static constexpr auto page_size = 64 * KiB;
 
 // Limits
 static constexpr auto max_allowed_call_stack_depth = 1000;
-static constexpr auto max_allowed_executed_instructions_per_call = 64 * 1024 * 1024;
+static constexpr auto max_allowed_executed_instructions_per_call = 1024 * 1024 * 1024;
 
 }
diff --git a/Userland/Libraries/LibWeb/WebAssembly/WebAssemblyObject.cpp b/Userland/Libraries/LibWeb/WebAssembly/WebAssemblyObject.cpp
index c21dd3daf6f..1c298bdbad2 100644
--- a/Userland/Libraries/LibWeb/WebAssembly/WebAssemblyObject.cpp
+++ b/Userland/Libraries/LibWeb/WebAssembly/WebAssemblyObject.cpp
@@ -432,7 +432,7 @@ JS::NativeFunction* create_native_function(Wasm::FunctionAddress address, String
             auto result = WebAssemblyObject::s_abstract_machine.invoke(address, move(values));
             // FIXME: Use the convoluted mapping of errors defined in the spec.
             if (result.is_trap()) {
-                vm.throw_exception<JS::TypeError>(global_object, "Wasm execution trapped (WIP)");
+                vm.throw_exception<JS::TypeError>(global_object, String::formatted("Wasm execution trapped (WIP): {}", result.trap().reason));
                 return {};
             }
 
diff --git a/Userland/Utilities/wasm.cpp b/Userland/Utilities/wasm.cpp
index bccbd5db66f..008bb2b2731 100644
--- a/Userland/Utilities/wasm.cpp
+++ b/Userland/Utilities/wasm.cpp
@@ -35,10 +35,10 @@ static bool post_interpret_hook(Wasm::Configuration&, Wasm::InstructionPointer&
 {
     if (interpreter.did_trap()) {
         g_continue = false;
-        const_cast<Wasm::Interpreter&>(interpreter).clear_trap();
         warnln("Trapped when executing ip={}", ip);
         g_printer.print(instr);
-        warnln("");
+        warnln("Trap reason: {}", interpreter.trap_reason());
+        const_cast<Wasm::Interpreter&>(interpreter).clear_trap();
     }
     return true;
 }
@@ -206,7 +206,7 @@ static bool pre_interpret_hook(Wasm::Configuration& config, Wasm::InstructionPoi
                 result = config.call(g_interpreter, *address, move(values));
             }
             if (result.is_trap())
-                warnln("Execution trapped!");
+                warnln("Execution trapped: {}", result.trap().reason);
             if (!result.values().is_empty())
                 warnln("Returned:");
             for (auto& value : result.values()) {