From 39bbd5764709b84c42f91907cef61a441b18f450 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Tue, 1 Mar 2022 03:03:18 -0800 Subject: [PATCH 1/3] wasmtime: fix for params array getting out of scope. Broken in proxy-wasm/proxy-wasm-cpp-host#217. Signed-off-by: Piotr Sikora --- .github/workflows/test.yml | 7 +++++ src/v8/v8.cc | 6 ++++ src/wasmtime/wasmtime.cc | 60 +++++++++++++++++++++++--------------- 3 files changed, 50 insertions(+), 23 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f94bca82e..c43f3bfeb 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -180,6 +180,13 @@ jobs: arch: x86_64 action: test flags: --config=clang-asan-strict --define=crypto=system + - name: 'Wasmtime on Linux/x86_64 with TSan' + engine: 'wasmtime' + repo: 'com_github_bytecodealliance_wasmtime' + os: ubuntu-20.04 + arch: x86_64 + action: test + flags: --config=clang-tsan - name: 'Wasmtime on Linux/aarch64' engine: 'wasmtime' repo: 'com_github_bytecodealliance_wasmtime' diff --git a/src/v8/v8.cc b/src/v8/v8.cc index 71a7483e8..60248f056 100644 --- a/src/v8/v8.cc +++ b/src/v8/v8.cc @@ -585,6 +585,8 @@ void V8::getModuleFunctionImpl(std::string_view function_name, *function = [func, function_name, this](ContextBase *context, Args... args) -> void { const bool log = cmpLogLevel(LogLevel::trace); SaveRestoreContext saved_context(context); + + // Workaround for MSVC++ not supporting zero-sized arrays. wasm::own trap = nullptr; if constexpr (sizeof...(args) > 0) { wasm::Val params[] = {makeVal(args)...}; @@ -599,6 +601,7 @@ void V8::getModuleFunctionImpl(std::string_view function_name, } trap = func->call(nullptr, nullptr); } + if (trap) { fail(FailState::RuntimeError, getFailMessage(std::string(function_name), std::move(trap))); return; @@ -634,6 +637,8 @@ void V8::getModuleFunctionImpl(std::string_view function_name, const bool log = cmpLogLevel(LogLevel::trace); SaveRestoreContext saved_context(context); wasm::Val results[1]; + + // Workaround for MSVC++ not supporting zero-sized arrays. wasm::own trap = nullptr; if constexpr (sizeof...(args) > 0) { wasm::Val params[] = {makeVal(args)...}; @@ -648,6 +653,7 @@ void V8::getModuleFunctionImpl(std::string_view function_name, } trap = func->call(nullptr, results); } + if (trap) { fail(FailState::RuntimeError, getFailMessage(std::string(function_name), std::move(trap))); return R{}; diff --git a/src/wasmtime/wasmtime.cc b/src/wasmtime/wasmtime.cc index 4bbc32b5f..5b5f1c46a 100644 --- a/src/wasmtime/wasmtime.cc +++ b/src/wasmtime/wasmtime.cc @@ -591,21 +591,28 @@ void Wasmtime::getModuleFunctionImpl(std::string_view function_name, } *function = [func, function_name, this](ContextBase *context, Args... args) -> void { - wasm_val_vec_t params; + const bool log = cmpLogLevel(LogLevel::trace); + SaveRestoreContext saved_context(context); + wasm_val_vec_t results = WASM_EMPTY_VEC; + + // Workaround for MSVC++ not supporting zero-sized arrays. + WasmTrapPtr trap; if constexpr (sizeof...(args) > 0) { wasm_val_t params_arr[] = {makeVal(args)...}; - params = WASM_ARRAY_VEC(params_arr); + wasm_val_vec_t params = WASM_ARRAY_VEC(params_arr); + if (log) { + integration()->trace("[host->vm] " + std::string(function_name) + "(" + + printValues(¶ms) + ")"); + } + trap.reset(wasm_func_call(func, ¶ms, &results)); } else { - params = WASM_EMPTY_VEC; - } - wasm_val_vec_t results = WASM_EMPTY_VEC; - const bool log = cmpLogLevel(LogLevel::trace); - if (log) { - integration()->trace("[host->vm] " + std::string(function_name) + "(" + printValues(¶ms) + - ")"); + wasm_val_vec_t params = WASM_EMPTY_VEC; + if (log) { + integration()->trace("[host->vm] " + std::string(function_name) + "()"); + } + trap.reset(wasm_func_call(func, ¶ms, &results)); } - SaveRestoreContext saved_context(context); - WasmTrapPtr trap{wasm_func_call(func, ¶ms, &results)}; + if (trap) { WasmByteVec error_message; wasm_trap_message(trap.get(), error_message.get()); @@ -645,22 +652,29 @@ void Wasmtime::getModuleFunctionImpl(std::string_view function_name, } *function = [func, function_name, this](ContextBase *context, Args... args) -> R { - wasm_val_vec_t params; + const bool log = cmpLogLevel(LogLevel::trace); + SaveRestoreContext saved_context(context); + wasm_val_t results_arr[1]; + wasm_val_vec_t results = WASM_ARRAY_VEC(results_arr); + + // Workaround for MSVC++ not supporting zero-sized arrays. + WasmTrapPtr trap; if constexpr (sizeof...(args) > 0) { wasm_val_t params_arr[] = {makeVal(args)...}; - params = WASM_ARRAY_VEC(params_arr); + wasm_val_vec_t params = WASM_ARRAY_VEC(params_arr); + if (log) { + integration()->trace("[host->vm] " + std::string(function_name) + "(" + + printValues(¶ms) + ")"); + } + trap.reset(wasm_func_call(func, ¶ms, &results)); } else { - params = WASM_EMPTY_VEC; - } - wasm_val_t results_arr[1]; - wasm_val_vec_t results = WASM_ARRAY_VEC(results_arr); - const bool log = cmpLogLevel(LogLevel::trace); - if (log) { - integration()->trace("[host->vm] " + std::string(function_name) + "(" + printValues(¶ms) + - ")"); + wasm_val_vec_t params = WASM_EMPTY_VEC; + if (log) { + integration()->trace("[host->vm] " + std::string(function_name) + "()"); + } + trap.reset(wasm_func_call(func, ¶ms, &results)); } - SaveRestoreContext saved_context(context); - WasmTrapPtr trap{wasm_func_call(func, ¶ms, &results)}; + if (trap) { WasmByteVec error_message; wasm_trap_message(trap.get(), error_message.get()); From 011e2b3016f5a57adb9a71aa81ede468455f72ce Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Tue, 1 Mar 2022 03:07:47 -0800 Subject: [PATCH 2/3] review: style. Signed-off-by: Piotr Sikora --- src/v8/v8.cc | 4 ++-- src/wasmtime/wasmtime.cc | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/v8/v8.cc b/src/v8/v8.cc index 60248f056..22a52c806 100644 --- a/src/v8/v8.cc +++ b/src/v8/v8.cc @@ -585,9 +585,9 @@ void V8::getModuleFunctionImpl(std::string_view function_name, *function = [func, function_name, this](ContextBase *context, Args... args) -> void { const bool log = cmpLogLevel(LogLevel::trace); SaveRestoreContext saved_context(context); + wasm::own trap = nullptr; // Workaround for MSVC++ not supporting zero-sized arrays. - wasm::own trap = nullptr; if constexpr (sizeof...(args) > 0) { wasm::Val params[] = {makeVal(args)...}; if (log) { @@ -637,9 +637,9 @@ void V8::getModuleFunctionImpl(std::string_view function_name, const bool log = cmpLogLevel(LogLevel::trace); SaveRestoreContext saved_context(context); wasm::Val results[1]; + wasm::own trap = nullptr; // Workaround for MSVC++ not supporting zero-sized arrays. - wasm::own trap = nullptr; if constexpr (sizeof...(args) > 0) { wasm::Val params[] = {makeVal(args)...}; if (log) { diff --git a/src/wasmtime/wasmtime.cc b/src/wasmtime/wasmtime.cc index 5b5f1c46a..06f4f0e3c 100644 --- a/src/wasmtime/wasmtime.cc +++ b/src/wasmtime/wasmtime.cc @@ -594,9 +594,9 @@ void Wasmtime::getModuleFunctionImpl(std::string_view function_name, const bool log = cmpLogLevel(LogLevel::trace); SaveRestoreContext saved_context(context); wasm_val_vec_t results = WASM_EMPTY_VEC; + WasmTrapPtr trap; // Workaround for MSVC++ not supporting zero-sized arrays. - WasmTrapPtr trap; if constexpr (sizeof...(args) > 0) { wasm_val_t params_arr[] = {makeVal(args)...}; wasm_val_vec_t params = WASM_ARRAY_VEC(params_arr); @@ -656,9 +656,9 @@ void Wasmtime::getModuleFunctionImpl(std::string_view function_name, SaveRestoreContext saved_context(context); wasm_val_t results_arr[1]; wasm_val_vec_t results = WASM_ARRAY_VEC(results_arr); + WasmTrapPtr trap; // Workaround for MSVC++ not supporting zero-sized arrays. - WasmTrapPtr trap; if constexpr (sizeof...(args) > 0) { wasm_val_t params_arr[] = {makeVal(args)...}; wasm_val_vec_t params = WASM_ARRAY_VEC(params_arr); From 5db188f8592114556a4513b14c2dc239ab7e3c95 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Tue, 1 Mar 2022 03:22:21 -0800 Subject: [PATCH 3/3] review: use -c opt instead of TSan (it fails for another reason). Signed-off-by: Piotr Sikora --- .github/workflows/test.yml | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index c43f3bfeb..fd8b7ae2c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -172,7 +172,7 @@ jobs: os: ubuntu-20.04 arch: x86_64 action: test - flags: --config=clang + flags: --config=clang -c opt - name: 'Wasmtime on Linux/x86_64 with ASan' engine: 'wasmtime' repo: 'com_github_bytecodealliance_wasmtime' @@ -180,13 +180,6 @@ jobs: arch: x86_64 action: test flags: --config=clang-asan-strict --define=crypto=system - - name: 'Wasmtime on Linux/x86_64 with TSan' - engine: 'wasmtime' - repo: 'com_github_bytecodealliance_wasmtime' - os: ubuntu-20.04 - arch: x86_64 - action: test - flags: --config=clang-tsan - name: 'Wasmtime on Linux/aarch64' engine: 'wasmtime' repo: 'com_github_bytecodealliance_wasmtime'