Skip to content

Migrate wasmtime to wasmtime-cpp-api#503

Open
leonm1 wants to merge 14 commits intoproxy-wasm:mainfrom
leonm1:api/wasmtime
Open

Migrate wasmtime to wasmtime-cpp-api#503
leonm1 wants to merge 14 commits intoproxy-wasm:mainfrom
leonm1:api/wasmtime

Conversation

@leonm1
Copy link
Contributor

@leonm1 leonm1 commented Mar 2, 2026

Hides many of the implementation details of the wasm-c-api.

Implements memory limiting and execution termination for wasmtime.

Built off of #502.

@leonm1 leonm1 force-pushed the api/wasmtime branch 3 times, most recently from 40e3040 to 68ebe57 Compare March 2, 2026 15:34
Copy link
Member

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implements memory limiting and execution termination for wasmtime.

Could you split this off into a separate PR? I think it's just a few lines, so it should be doable, and I think those changes warrant a proper review instead of being bundled in this mass refactor. Thanks!

@leonm1 leonm1 force-pushed the api/wasmtime branch 2 times, most recently from 680dc57 to 5982aca Compare March 2, 2026 20:15
@leonm1 leonm1 force-pushed the api/wasmtime branch 2 times, most recently from 356f7e2 to dbc1f3e Compare March 12, 2026 02:36
@leonm1 leonm1 requested a review from PiotrSikora March 12, 2026 03:13
Comment on lines -229 to -287
genrule(
name = "prefixed_wasmtime_sources",
srcs = [
"src/wasmtime/types.h",
"src/wasmtime/wasmtime.cc",
],
outs = [
"src/wasmtime/prefixed_types.h",
"src/wasmtime/prefixed_wasmtime.cc",
],
cmd = """
for file in $(SRCS); do
sed -e 's/wasm_/wasmtime_wasm_/g' \
-e 's/include\\/wasm.h/include\\/prefixed_wasm.h/g' \
-e 's/wasmtime\\/types.h/wasmtime\\/prefixed_types.h/g' \
$$file >$(@D)/$$(dirname $$file)/prefixed_$$(basename $$file)
done
""",
)

cc_library(
name = "prefixed_wasmtime_lib",
srcs = [
"src/common/types.h",
"src/wasmtime/prefixed_types.h",
"src/wasmtime/prefixed_wasmtime.cc",
],
hdrs = ["include/proxy-wasm/wasmtime.h"],
copts = [
"-DWASM_API_EXTERN=",
],
defines = [
"PROXY_WASM_HAS_RUNTIME_WASMTIME",
"PROXY_WASM_HOST_ENGINE_WASMTIME",
],
# See: https://bytecodealliance.github.io/wasmtime/c-api/
linkopts = select({
"@platforms//os:macos": [],
"@platforms//os:windows": [
"ws2_32.lib",
"advapi32.lib",
"userenv.lib",
"ntdll.lib",
"shell32.lib",
"ole32.lib",
"bcrypt.lib",
],
"//conditions:default": [
"-ldl",
"-lm",
"-lpthread",
],
}),
deps = [
":wasm_vm_headers",
"@com_github_bytecodealliance_wasmtime//:prefixed_wasmtime_lib",
],
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sholdn't this be part of #501?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we were using the wasm-c-api, I kept the prefixed_wasmtime_sources genrule to rewrite all of our references from wasm_ to wasmtime_wasm_ in src/wasmtime/wasmtime.cc.

Now, this is no longer needed since all the rewriting happens inside the wasmtime-c-api-impl crate and we can just use the C++ API without any rewriting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks!

On the same topic - with the migration to Wasmtime C++ API, do we still need the prefixed version of Wasmtime, or only the Wasmtime C++ API symbols are now exported and it doesn't conflict with other engines exporting Wasm C/C++ API symbols?

Copy link
Member

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm, looking at the passed arguments in failed tests, it seems like some endianness conversions are broken.

@leonm1 leonm1 marked this pull request as ready for review March 13, 2026 23:19
@leonm1 leonm1 requested a review from PiotrSikora March 13, 2026 23:19
static inline uint32_t bswap(uint32_t x) { return __builtin_bswap32(x); }
static inline auto bswap(auto x) { return __builtin_bswap64(x); }
#define htowasm(x, vm_uses_wasm_byte_order) ((vm_uses_wasm_byte_order) ? bswap(x) : (x))
#define wasmtoh(x, vm_uses_wasm_byte_order) ((vm_uses_wasm_byte_order) ? bswap(x) : (x))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this change? AFAICT, all the callers are always passing uint32_t.

Copy link
Contributor Author

@leonm1 leonm1 Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a few hostcalls that use int64_t:

Word increment_metric(Word metric_id, int64_t offset);

Word wasi_unstable_clock_time_get(Word, uint64_t, Word);

Comment on lines +76 to +87
template <typename... Args> void InPlaceConvertWasmToHostEndianness(Args &...args) {
(void(args = wasmtoh(convertWordToUint32(args), true)), ...);
}
proxy_wasm::Word ConvertWasmToHostEndianness(const proxy_wasm::Word &arg) {
return proxy_wasm::Word(wasmtoh(convertWordToUint32(arg), true));
}
template <typename... Args> void InPlaceConvertHostToWasmEndianness(Args &...args) {
(void(args = htowasm(convertWordToUint32(args), true)), ...);
}
template <typename... Args> auto ConvertHostToWasmEndianness(const Args &...args) {
return std::make_tuple((htowasm(convertWordToUint32(args), true))...);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of this approach. We're converting the value to little-endian here, but the value type is still native and not Wasm, which is different approach than we've used previously (and still do in other places).

Also, as discussed on Slack, Wasmtime is taking different actions based on the value types, so I'm not as convinced that this is strictly correct solution as I'd be if we were converting to a correct type.

Copy link
Contributor Author

@leonm1 leonm1 Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... I think this is a bug in wasmtime's C++ API. Filed bytecodealliance/wasmtime#12784.

For example, the arg values passed from Wasmtime to the function provided during linking will be regular uint32_t-typed values but in little endian. Since the cpp wrapper is performing the conversion from uint32_t to wasmtime_val_raw_t and the inverse

In the meantime, I can restore functions to convert to wasmtime_val_t

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about merging as-is since #525 introduced testing for the ABI boundary?

Once the bug is fixed in wasmtime, our tests will alert us that we should remove our endianness handling here when we update past the fixed version.

leonm1 added 7 commits March 17, 2026 13:05
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
There's enough test cases that this warrants being run in parallel via its own test target.

Signed-off-by: Matt Leon <mattleon@google.com>
Uses bswap64 by default to handle int64 and uint64 values.

For float and doubles, uses appropriate-size bswap operators by first
bit-casting floats and doubles to their same-size int types. Otherwise,
they will be coerced to an int type before conversion and be returned as
int values.

Signed-off-by: Matt Leon <mattleon@google.com>
leonm1 added 3 commits March 18, 2026 17:01
Without first converting to uint32_t, the getters for wasm_val_t for
wamr, wasmtime, and wasmedge returned a signed integer type. For uint32
values high enough to be in the negative range, sign extension would be
applied when the value was coerced into proxy_wasm::Word. This resulted
in Word values that did not match the comment on Word

```
// Represents a Wasm-native word-sized datum. On 32-bit VMs, the high bits are always zero.
// The Wasm/VM API treats all bits as significant.
```

nor the exact value returned from the wasm plugin.

Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
leonm1 added 4 commits March 18, 2026 17:26
Signed-off-by: Matt Leon <mattleon@google.com>
Hides many of the implementation details of the wasm-c-api.

Note: adds `wat` feature to wasmtime c headers to fix the following build error, but note that `wat` support is not enabled in the wasmtime build, this just adds the headers to allow the cpp api to compile.

```
external/com_github_bytecodealliance_wasmtime/crates/c-api/include/wasmtime/module.hh:39:17: error: use of undeclared identifier 'wat2wasm'
   39 |     auto wasm = wat2wasm(wat);
      |                 ^
1 error generated.
```

Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants