wasm: Add Wasmtime as a new runtime

[mathetake]

Signed-off-by: mathetake takeshi@tetrate.io

Commit Message: wasm: Add Wasmtime(https://github.com/bytecodealliance/wasmtime) as a new runtime for Wasm filter
Additional Description: add a new wasm runtime, named Wasmtime, in addition to existing runtimes like V8 and WAVM.
Risk Level: low
Testing: run in bazel.compile_time_options build target as WAVM.
Docs Changes: Docs will be provided in subsequent PRs.
Release Notes: enable Wasmtime as a Wasm filter runtime optionally.
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

ref: proxy-wasm/proxy-wasm-cpp-host#73

[repokitteh-read-only]

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #13932 was opened by mathetake.

see: more, trace.

[antoniovicente]

Thanks for the pull request. Is there some particular aspect of this change that you'ld like early feedback on? What additional changes you think are needed before this draft PR is ready for review and merge?

[PiotrSikora]

I wonder if this is possible now that you removed jit-dump option?

[PiotrSikora]

Also, perhaps we should remove the "all" option altogether, since it no longer represents all runtimes?

Multiple runtimes can be selected individually anyway, e.g.:

bazel build --config=libc++ --define=wasm=v8 --define=wasm=wavm //source/exe:envoy-static

[mathetake]

Multiple runtimes can be selected individually anyway, e.g.:

I didn't know that. Thanks! I'll remove all option

[mathetake]

and, let me see if the collision with WAVM has been resolved

[mathetake]

I could observe the same collision with jit-dump=disabled:

se --sandbox_debug to see verbose messages from the sandbox
ld.lld: error: duplicate symbol: __jit_debug_descriptor
>>> defined at GDBRegistrationListener.cpp
>>>            GDBRegistrationListener.cpp.o:(__jit_debug_descriptor) in archive bazel-out/k8-fastbuild/bin/bazel/foreign_cc/llvm/lib/libLLVMExecutionEngine.a
>>> defined at wasmtime_runtime.849cvge1-cgu.15
>>>            wasmtime_runtime-361389515.wasmtime_runtime.849cvge1-cgu.15.rcgu.o:(.data.__jit_debug_descriptor+0x0) in archive bazel-out/k8-fastbuild/bin/external/com_github_wasmtime/librust_c_api-220117231.a

ld.lld: error: duplicate symbol: __jit_debug_register_code
>>> defined at GDBRegistrationListener.cpp
>>>            GDBRegistrationListener.cpp.o:(__jit_debug_register_code) in archive bazel-out/k8-fastbuild/bin/bazel/foreign_cc/llvm/lib/libLLVMExecutionEngine.a
>>> defined at wasmtime_runtime.849cvge1-cgu.15
>>>            wasmtime_runtime-361389515.wasmtime_runtime.849cvge1-cgu.15.rcgu.o:(.text.__jit_debug_register_code+0x0) in archive bazel-out/k8-fastbuild/bin/external/com_github_wasmtime/librust_c_api-220117231.a

It's defined in the runtime package which is nothing to do with jit-dump feature https://github.com/bytecodealliance/wasmtime/blob/v0.21.0/crates/runtime/src/jit_int.rs#L32-L41

[mathetake]

and I realized that the V8 defines the same symbol but disable it by default https://chromium.googlesource.com/v8/v8/+/4.10.71/src/gdb-jit.cc#24 , that's why luckily we have not encountered symbol collision in conjunction with V8 and WAVM

[mathetake]

This ugly patch seems to work...

wavm.patch

diff --git a/CMakeLists.txt b/CMakeLists.txt
index eb5d7c2c..2bd2c748 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -720,7 +720,6 @@ if(WAVM_ENABLE_RUNTIME)
 	add_subdirectory(Lib/Runtime)
 	add_subdirectory(Lib/ThreadTest)
 	add_subdirectory(Lib/WASI)
-	add_subdirectory(Lib/wavm-c)
 endif()
 
 if(WAVM_ENABLE_UNWIND)
diff --git a/Lib/LLVMJIT/LLVMModule.cpp b/Lib/LLVMJIT/LLVMModule.cpp
index 5f830959..0e222f36 100644
--- a/Lib/LLVMJIT/LLVMModule.cpp
+++ b/Lib/LLVMJIT/LLVMModule.cpp
@@ -61,9 +61,6 @@ using namespace WAVM::LLVMJIT;
 
 struct LLVMJIT::GlobalModuleState
 {
-	Platform::Mutex gdbRegistrationListenerMutex;
-	llvm::JITEventListener* gdbRegistrationListener = nullptr;
-
 	// A map from address to loaded JIT symbols.
 	Platform::RWMutex addressToModuleMapMutex;
 	std::map<Uptr, LLVMJIT::Module*> addressToModuleMap;
@@ -76,11 +73,8 @@ struct LLVMJIT::GlobalModuleState
 
 	// These constructor and destructor should not be called directly, but must be public in order
 	// to be accessible by std::make_shared.
-	GlobalModuleState()
-	{
-		gdbRegistrationListener = llvm::JITEventListener::createGDBRegistrationListener();
-	}
-	~GlobalModuleState() { delete gdbRegistrationListener; }
+	GlobalModuleState() {}
+	~GlobalModuleState() {}
 };
 
 // Allocates memory for the LLVM object loader.
@@ -516,13 +510,6 @@ Module::Module(const std::vector<U8>& objectBytes,
 
 	// Notify GDB of the new object.
 	{
-		Platform::Mutex::Lock lock(globalModuleState->gdbRegistrationListenerMutex);
-#if LLVM_VERSION_MAJOR >= 8
-		globalModuleState->gdbRegistrationListener->notifyObjectLoaded(
-			reinterpret_cast<Uptr>(this), *object, *loadedObject);
-#else
-		globalModuleState->gdbRegistrationListener->NotifyObjectEmitted(*object, *loadedObject);
-#endif
 	}
 
 	// Create a DWARF context to interpret the debug information in this compilation unit.
@@ -616,15 +603,7 @@ Module::Module(const std::vector<U8>& objectBytes,
 Module::~Module()
 {
 	// Notify GDB that the object is being unloaded.
-	{
-		Platform::Mutex::Lock lock(globalModuleState->gdbRegistrationListenerMutex);
-#if LLVM_VERSION_MAJOR >= 8
-		globalModuleState->gdbRegistrationListener->notifyFreeingObject(
-			reinterpret_cast<Uptr>(this));
-#else
-		globalModuleState->gdbRegistrationListener->NotifyFreeingObject(*object);
-#endif
-	}
+	{}
 
 	// Remove the module from the global address to module map.
 	{
diff --git a/Programs/wavm/CMakeLists.txt b/Programs/wavm/CMakeLists.txt
index 3d00c1df..e3445c13 100644
--- a/Programs/wavm/CMakeLists.txt
+++ b/Programs/wavm/CMakeLists.txt
@@ -13,7 +13,6 @@ set(NonRuntimeSources Testing/DumpTestModules.cpp
 set(RuntimeOnlySources
 			Testing/Benchmark.cpp
 			Testing/RunTestScript.cpp
-			Testing/TestCAPI.c
 			wavm-compile.cpp
 			wavm-run.cpp)
 
@@ -37,6 +36,3 @@ add_test(NAME HashMap COMMAND $<TARGET_FILE:wavm> test hashmap)
 add_test(NAME HashSet COMMAND $<TARGET_FILE:wavm> test hashset)
 add_test(NAME I128 COMMAND $<TARGET_FILE:wavm> test i128)
 
-if(WAVM_ENABLE_RUNTIME)
-	add_test(NAME C-API COMMAND $<TARGET_FILE:wavm> test c-api)
-endif()
diff --git a/Programs/wavm/Testing/wavm-test.cpp b/Programs/wavm/Testing/wavm-test.cpp
index eed47d13..61ef1189 100644
--- a/Programs/wavm/Testing/wavm-test.cpp
+++ b/Programs/wavm/Testing/wavm-test.cpp
@@ -99,7 +99,7 @@ int execTestCommand(int argc, char** argv)
 		case TestCommand::hashSet: return execHashSetTest(argc - 1, argv + 1);
 		case TestCommand::i128: return execI128Test(argc - 1, argv + 1);
 #if WAVM_ENABLE_RUNTIME
-		case TestCommand::cAPI: return execCAPITest(argc - 1, argv + 1);
+		case TestCommand::cAPI: return 0;
 		case TestCommand::benchmark: return execBenchmark(argc - 1, argv + 1);
 		case TestCommand::script: return execRunTestScript(argc - 1, argv + 1);
 #endif

[mathetake]

with this patch, I could link both WAVM and Wasmtime at the same time, but when I run extension with wasmtime, I see weird error (I am completely not sure where this is coming from)

2020-11-10 15:11:32.316][576526][debug][wasm] [source/extensions/common/wasm/wasm.cc:126] Thread-Local Wasm created 3 now active
libunwind: _unw_add_dynamic_fde: bad fde: FDE is really a CIE
libunwind: _unw_add_dynamic_fde: bad fde: FDE is really a CIE
libunwind: _unw_add_dynamic_fde: bad fde: FDE is really a CIE
libunwind: _unw_add_dynamic_fde: bad fde: FDE is really a CIE
libunwind: _unw_add_dynamic_fde: bad fde: FDE is really a CIE

This is not observed when we link Wasmtime alone, so something's broken

[PiotrSikora]

Yeah, I don't think we want to maintain that big of a patch. Removing single directory from CMakeLists.txt is acceptable, but modifying code to link multiple runtimes is probably not a good idea.

Thanks for looking, though!

[mathetake]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13932 (comment) was created by @mathetake.

see: more, trace.

[mathetake]

/retest

[repokitteh-read-only]

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13932 (comment) was created by @mathetake.

see: more, trace.

[lizan]

Should be good to go but need @envoyproxy/dependency-shepherds review.

[PiotrSikora]

@envoyproxy/dependency-shepherds friendly ping.

[moderation]

Brings in two more deps - would be good to have @htuch review. Would like to bring in latest commits as per comments

[moderation]

Please add comment as to why we need to pin to old commit

[moderation]

/lgtm deps