From 6b50f232426742fcf9e71420c37bf4831bca2e31 Mon Sep 17 00:00:00 2001 From: Ali Mohammad Pur Date: Sat, 25 Feb 2023 11:15:11 +0330 Subject: [PATCH] LibWasm+LibWeb: Sneak a JS::Completion into Wasm::Result Imported functions in Wasm may throw JS exceptions, and we need to preserve these exceptions so we can pass them to the calling JS code. This also adds a `assert_wasm_result()` API to Result for cases where only Wasm traps or values are expected (e.g. internal uses) to avoid making LibWasm (pointlessly) handle JS exceptions that will never show up in reality. --- Meta/Lagom/CMakeLists.txt | 2 +- Tests/LibWasm/CMakeLists.txt | 2 +- Tests/LibWasm/test-wasm.cpp | 3 ++ .../AbstractMachine/AbstractMachine.cpp | 8 +-- .../LibWasm/AbstractMachine/AbstractMachine.h | 54 ++++++++++++++++++- .../AbstractMachine/BytecodeInterpreter.cpp | 9 +++- .../AbstractMachine/BytecodeInterpreter.h | 16 ++++-- Userland/Libraries/LibWasm/CMakeLists.txt | 2 +- .../LibWeb/WebAssembly/WebAssemblyObject.cpp | 14 ++--- Userland/Utilities/CMakeLists.txt | 2 +- Userland/Utilities/wasm.cpp | 18 ++++--- 11 files changed, 95 insertions(+), 35 deletions(-) diff --git a/Meta/Lagom/CMakeLists.txt b/Meta/Lagom/CMakeLists.txt index 7dae1bfde5..432ab5cf87 100644 --- a/Meta/Lagom/CMakeLists.txt +++ b/Meta/Lagom/CMakeLists.txt @@ -552,7 +552,7 @@ if (BUILD_LAGOM) endif() add_executable(wasm ../../Userland/Utilities/wasm.cpp) - target_link_libraries(wasm LibCore LibWasm LibLine LibMain) + target_link_libraries(wasm LibCore LibWasm LibLine LibMain LibJS) add_executable(xml ../../Userland/Utilities/xml.cpp) target_link_libraries(xml LibCore LibXML LibMain) diff --git a/Tests/LibWasm/CMakeLists.txt b/Tests/LibWasm/CMakeLists.txt index f3b3f86666..8e393132f3 100644 --- a/Tests/LibWasm/CMakeLists.txt +++ b/Tests/LibWasm/CMakeLists.txt @@ -1,2 +1,2 @@ -serenity_testjs_test(test-wasm.cpp test-wasm LIBS LibWasm) +serenity_testjs_test(test-wasm.cpp test-wasm LIBS LibWasm LibJS) install(TARGETS test-wasm RUNTIME DESTINATION bin OPTIONAL) diff --git a/Tests/LibWasm/test-wasm.cpp b/Tests/LibWasm/test-wasm.cpp index 233f9f90f5..c882b95a3f 100644 --- a/Tests/LibWasm/test-wasm.cpp +++ b/Tests/LibWasm/test-wasm.cpp @@ -246,6 +246,9 @@ JS_DEFINE_NATIVE_FUNCTION(WebAssemblyModule::wasm_invoke) if (result.is_trap()) return vm.throw_completion(TRY_OR_THROW_OOM(vm, String::formatted("Execution trapped: {}", result.trap().reason))); + if (result.is_completion()) + return result.completion(); + if (result.values().is_empty()) return JS::js_null(); diff --git a/Userland/Libraries/LibWasm/AbstractMachine/AbstractMachine.cpp b/Userland/Libraries/LibWasm/AbstractMachine/AbstractMachine.cpp index ddc8cc4538..d3d962b8ce 100644 --- a/Userland/Libraries/LibWasm/AbstractMachine/AbstractMachine.cpp +++ b/Userland/Libraries/LibWasm/AbstractMachine/AbstractMachine.cpp @@ -175,7 +175,7 @@ InstantiationResult AbstractMachine::instantiate(Module const& module, Vectorexpression, 1, }); - auto result = config.execute(interpreter); + auto result = config.execute(interpreter).assert_wasm_result(); if (result.is_trap()) { instantiation_result = InstantiationError { DeprecatedString::formatted("Element section initialisation trapped: {}", result.trap().reason) }; return IterationDecision::Break; @@ -315,7 +315,7 @@ InstantiationResult AbstractMachine::instantiate(Module const& module, Vector #include +// NOTE: Special case for Wasm::Result. +#include + namespace Wasm { class Configuration; @@ -171,6 +174,35 @@ struct Trap { DeprecatedString reason; }; +// A variant of Result that does not include external reasons for error (JS::Completion, for now). +class PureResult { +public: + explicit PureResult(Vector values) + : m_result(move(values)) + { + } + + PureResult(Trap trap) + : m_result(move(trap)) + { + } + + auto is_trap() const { return m_result.has(); } + auto& values() const { return m_result.get>(); } + auto& values() { return m_result.get>(); } + auto& trap() const { return m_result.get(); } + auto& trap() { return m_result.get(); } + +private: + friend class Result; + explicit PureResult(Variant, Trap>&& result) + : m_result(move(result)) + { + } + + Variant, Trap> m_result; +}; + class Result { public: explicit Result(Vector values) @@ -183,14 +215,34 @@ public: { } + Result(JS::Completion completion) + : m_result(move(completion)) + { + VERIFY(m_result.get().is_abrupt()); + } + + Result(PureResult&& result) + : m_result(result.m_result.downcast()) + { + } + auto is_trap() const { return m_result.has(); } + auto is_completion() const { return m_result.has(); } auto& values() const { return m_result.get>(); } auto& values() { return m_result.get>(); } auto& trap() const { return m_result.get(); } auto& trap() { return m_result.get(); } + auto& completion() { return m_result.get(); } + auto& completion() const { return m_result.get(); } + + PureResult assert_wasm_result() && + { + VERIFY(!is_completion()); + return PureResult(move(m_result).downcast, Trap>()); + } private: - Variant, Trap> m_result; + Variant, Trap, JS::Completion> m_result; }; using ExternValue = Variant; diff --git a/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.cpp b/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.cpp index 2e11c1ccf1..ee5baaaf17 100644 --- a/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.cpp +++ b/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.cpp @@ -34,7 +34,7 @@ namespace Wasm { void BytecodeInterpreter::interpret(Configuration& configuration) { - m_trap.clear(); + m_trap = Empty {}; auto& instructions = configuration.frame().expression().instructions(); auto max_ip_value = InstructionPointer { instructions.size() }; auto& current_ip_value = configuration.ip(); @@ -51,7 +51,7 @@ void BytecodeInterpreter::interpret(Configuration& configuration) auto& instruction = instructions[current_ip_value.value()]; auto old_ip = current_ip_value; interpret(configuration, current_ip_value, instruction); - if (m_trap.has_value()) + if (did_trap()) return; if (current_ip_value == old_ip) // If no jump occurred ++current_ip_value; @@ -140,6 +140,11 @@ void BytecodeInterpreter::call_address(Configuration& configuration, FunctionAdd return; } + if (result.is_completion()) { + m_trap = move(result.completion()); + return; + } + configuration.stack().entries().ensure_capacity(configuration.stack().size() + result.values().size()); for (auto& entry : result.values().in_reverse()) configuration.stack().entries().unchecked_append(move(entry)); diff --git a/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.h b/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.h index 55ffc1eeec..9cb1385d30 100644 --- a/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.h +++ b/Userland/Libraries/LibWasm/AbstractMachine/BytecodeInterpreter.h @@ -15,9 +15,15 @@ namespace Wasm { struct BytecodeInterpreter : public Interpreter { virtual void interpret(Configuration&) override; virtual ~BytecodeInterpreter() override = default; - virtual bool did_trap() const override { return m_trap.has_value(); } - virtual DeprecatedString trap_reason() const override { return m_trap.value().reason; } - virtual void clear_trap() override { m_trap.clear(); } + virtual bool did_trap() const override { return !m_trap.has(); } + virtual DeprecatedString trap_reason() const override + { + return m_trap.visit( + [](Empty) -> DeprecatedString { VERIFY_NOT_REACHED(); }, + [](Trap const& trap) { return trap.reason; }, + [](JS::Completion const& completion) { return completion.value()->to_string_without_side_effects().release_value().to_deprecated_string(); }); + } + virtual void clear_trap() override { m_trap = Empty {}; } struct CallFrameHandle { explicit CallFrameHandle(BytecodeInterpreter& interpreter, Configuration& configuration) @@ -62,10 +68,10 @@ protected: { if (!value) m_trap = Trap { reason }; - return m_trap.has_value(); + return !m_trap.has(); } - Optional m_trap; + Variant m_trap; StackInfo m_stack_info; }; diff --git a/Userland/Libraries/LibWasm/CMakeLists.txt b/Userland/Libraries/LibWasm/CMakeLists.txt index f86cbe7e00..5b78f92da6 100644 --- a/Userland/Libraries/LibWasm/CMakeLists.txt +++ b/Userland/Libraries/LibWasm/CMakeLists.txt @@ -8,7 +8,7 @@ set(SOURCES ) serenity_lib(LibWasm wasm) -target_link_libraries(LibWasm PRIVATE LibCore) +target_link_libraries(LibWasm PRIVATE LibCore LibJS) # FIXME: Install these into usr/Tests/LibWasm include(wasm_spec_tests) diff --git a/Userland/Libraries/LibWeb/WebAssembly/WebAssemblyObject.cpp b/Userland/Libraries/LibWeb/WebAssembly/WebAssemblyObject.cpp index 6073622e92..212f0e8204 100644 --- a/Userland/Libraries/LibWeb/WebAssembly/WebAssemblyObject.cpp +++ b/Userland/Libraries/LibWeb/WebAssembly/WebAssemblyObject.cpp @@ -200,20 +200,12 @@ JS::ThrowCompletionOr WebAssemblyObject::instantiate_module(JS::VM& vm, for (auto& entry : arguments) argument_values.append(to_js_value(vm, entry)); - auto result_or_error = JS::call(vm, function, JS::js_undefined(), move(argument_values)); - if (result_or_error.is_error()) { - return Wasm::Trap(); - } + auto result = TRY(JS::call(vm, function, JS::js_undefined(), move(argument_values))); if (type.results().is_empty()) return Wasm::Result { Vector {} }; - if (type.results().size() == 1) { - auto value_or_error = to_webassembly_value(vm, result_or_error.release_value(), type.results().first()); - if (value_or_error.is_error()) - return Wasm::Trap {}; - - return Wasm::Result { Vector { value_or_error.release_value() } }; - } + if (type.results().size() == 1) + return Wasm::Result { Vector { TRY(to_webassembly_value(vm, result, type.results().first())) } }; // FIXME: Multiple returns TODO(); diff --git a/Userland/Utilities/CMakeLists.txt b/Userland/Utilities/CMakeLists.txt index 6316ffb0aa..ec1928cf68 100644 --- a/Userland/Utilities/CMakeLists.txt +++ b/Userland/Utilities/CMakeLists.txt @@ -133,7 +133,7 @@ target_link_libraries(unzip PRIVATE LibArchive LibCompress LibCrypto) target_link_libraries(update-cpp-test-results PRIVATE LibCpp) target_link_libraries(useradd PRIVATE LibCrypt) target_link_libraries(wallpaper PRIVATE LibGfx LibGUI) -target_link_libraries(wasm PRIVATE LibWasm LibLine) +target_link_libraries(wasm PRIVATE LibWasm LibLine LibJS) target_link_libraries(wsctl PRIVATE LibGUI LibIPC) target_link_libraries(xml PRIVATE LibXML) target_link_libraries(zip PRIVATE LibArchive LibCompress LibCrypto) diff --git a/Userland/Utilities/wasm.cpp b/Userland/Utilities/wasm.cpp index 00652bbd57..38c061095a 100644 --- a/Userland/Utilities/wasm.cpp +++ b/Userland/Utilities/wasm.cpp @@ -206,15 +206,17 @@ static bool pre_interpret_hook(Wasm::Configuration& config, Wasm::InstructionPoi Wasm::Result result { Wasm::Trap {} }; { Wasm::BytecodeInterpreter::CallFrameHandle handle { g_interpreter, config }; - result = config.call(g_interpreter, *address, move(values)); + result = config.call(g_interpreter, *address, move(values)).assert_wasm_result(); } - if (result.is_trap()) + if (result.is_trap()) { warnln("Execution trapped: {}", result.trap().reason); - if (!result.values().is_empty()) - warnln("Returned:"); - for (auto& value : result.values()) { - g_stdout->write(" -> "sv.bytes()).release_value_but_fixme_should_propagate_errors(); - g_printer->print(value); + } else { + if (!result.values().is_empty()) + warnln("Returned:"); + for (auto& value : result.values()) { + g_stdout->write(" -> "sv.bytes()).release_value_but_fixme_should_propagate_errors(); + g_printer->print(value); + } } continue; } @@ -513,7 +515,7 @@ ErrorOr serenity_main(Main::Arguments arguments) outln(); } - auto result = machine.invoke(g_interpreter, run_address.value(), move(values)); + auto result = machine.invoke(g_interpreter, run_address.value(), move(values)).assert_wasm_result(); if (debug) launch_repl();