Stream: git-wasmtime

Topic: wasmtime / issue #6765 Make C-API usable from Rust

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2023 at 04:44):

github-actions[bot] commented on issue #6765:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2023 at 14:50):

alexcrichton commented on issue #6765:

Thanks for the PR! I'm a bit hesitant personally about this, however, because if you're able to use Cargo.toml dependencies is there any reason you couldn't use the wasmtime crate itself? The C API isn't designed to be used as a Rust crate dependency and probably exposes some functions as safe which are actually unsafe for example (which doesn't matter on a C API bounary but matters to Rust).

There's also other niche concerns where the Rust compiler has some odd gotchas about crates that are compiled as both a cdylib and an rlib and I think it would be best to avoid that.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 25 2023 at 14:59):

bjorn3 commented on issue #6765:

There's also other niche concerns where the Rust compiler has some odd gotchas about crates that are compiled as both a cdylib and an rlib and I think it would be best to avoid that.

That is fixed as of https://github.com/rust-lang/rust/pull/113695

view this post on Zulip Wasmtime GitHub notifications bot (Jul 26 2023 at 03:30):

RubixDev commented on issue #6765:

Thanks for the PR! I'm a bit hesitant personally about this, however, because if you're able to use Cargo.toml dependencies is there any reason you couldn't use the wasmtime crate itself?

Yes, I guess it didn't really become clear. Tree-sitter is written in C and provides Rust bindings. The C part uses wasmtime internally. The build script for the Rust bindings compiles the C code. For this, the wasmtime C library must be available because the tree-sitter C code uses it. The easiest way to have it available is to add it as a cargo dependency.

The C API isn't designed to be used as a Rust crate dependency and probably exposes some functions as safe which are actually unsafe for example (which doesn't matter on a C API bounary but matters to Rust).

So the Rust crate does _not_ interface with wasmtime directly (that could be done with the normal wasmtime crate), but the C code it provides bindings to _does_. The safe/unsafe functions from the wasmtime-c-api crate still only get called from C, not from Rust.

I hope this time it's clearer

view this post on Zulip Wasmtime GitHub notifications bot (Jul 26 2023 at 03:31):

RubixDev edited a comment on issue #6765:

Thanks for the PR! I'm a bit hesitant personally about this, however, because if you're able to use Cargo.toml dependencies is there any reason you couldn't use the wasmtime crate itself?

Yes, I guess it didn't really become clear. Tree-sitter is written in C and provides Rust bindings. The C part uses wasmtime internally. The build script for the Rust bindings compiles the C code. For this, the wasmtime C library must be available because the tree-sitter C code uses it. The easiest way to have it available is to add it as a cargo dependency.

The C API isn't designed to be used as a Rust crate dependency and probably exposes some functions as safe which are actually unsafe for example (which doesn't matter on a C API bounary but matters to Rust).

So the Rust crate _does not_ interface with wasmtime directly (that could be done with the normal wasmtime crate), but the C code it provides bindings to _does_. The safe/unsafe functions from the wasmtime-c-api crate still only get called from C, not from Rust.

I hope this time it's clearer

view this post on Zulip Wasmtime GitHub notifications bot (Jul 26 2023 at 06:58):

RubixDev commented on issue #6765:

Ok and to address the failing CI, this is because the C library is expected to just be called wasmtime (or libwasmtime.a), whereas with this change it is called wasmtime_c_api instead. That's obviously not desired, but necessary for tree-sitter to additionally depend on the normal Rust wasmtime crate. I think the best alternative would be to revert the name change and re-export the Rust wasmtime API from the wasmtime-c-api crate. This way, tree-sitter only needs to depend on wasmtime-c-api for both the normal Rust API and the C API.

Before I push this though, I would like feedback on this idea.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 26 2023 at 07:01):

RubixDev edited a comment on issue #6765:

Ok and to address the failing CI, this is because the C library is expected to just be called wasmtime (or libwasmtime.a), whereas with this change it is called wasmtime_c_api instead. That's obviously not desired, but necessary for tree-sitter to additionally depend on the normal Rust wasmtime crate. I think the best alternative would be to revert the name change and re-export the Rust wasmtime API from the wasmtime-c-api crate. This way, tree-sitter only needs to depend on wasmtime-c-api for both the normal Rust API and the C API.

Before I push this though, I would like feedback on this idea.

<details>
<summary>The final diff would then look something like this</summary>

diff --git a/crates/c-api/Cargo.toml b/crates/c-api/Cargo.toml
index a464c0dbd..eb08a548a 100644
--- a/crates/c-api/Cargo.toml
+++ b/crates/c-api/Cargo.toml
@@ -11,7 +11,7 @@ publish = false

 [lib]
 name = "wasmtime"
-crate-type = ["staticlib", "cdylib"]
+crate-type = ["staticlib", "cdylib", "rlib"]
 doc = false
 test = false
 doctest = false
diff --git a/crates/c-api/src/lib.rs b/crates/c-api/src/lib.rs
index 4bc111b5d..3ef450731 100644
--- a/crates/c-api/src/lib.rs
+++ b/crates/c-api/src/lib.rs
@@ -11,6 +11,8 @@

 #![allow(non_snake_case, non_camel_case_types, non_upper_case_globals)]

+pub use wasmtime::*;
+
 mod config;
 mod engine;
 mod error;

</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jul 26 2023 at 14:21):

alexcrichton commented on issue #6765:

That is fixed as of https://github.com/rust-lang/rust/pull/113695

Oh nice! I was also thinking of issues though with LTO where rlib+cdylib has at least caused problems there where LTO settings effectively don't apply. That may have been fixed, though, I'm not sure.

Tree-sitter is written in C and provides Rust bindings

Hm ok it might be good to better understand your use case first then. This is a recipe for things going wrong where a Rust crate depends on a C library which depends on a Rust library. There's two versions of the rust standard library in that final program and that may or may not be expected (and may also cause things like linking issues).

I'm also still not sure how this PR is expected to be integrated successfully? Why is reexporting wasmtime::* necessary? Are you using both Wasmtime's C API and the native Rust API? Or are the Rust bindings for tree-sitter exclusively using tree-sitter's own C API?

At least for me I've never found a way to strike a right balance providing a C API from a Rust crate. I've gotten requests like this historically to support the C API both when used from Rust and C and I don't know how best to organize and enable that. In the end though if this works for you (it'd be good to end-to-end test to confirm) then my concerns are pretty niche and we can go ahead and merge it.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 26 2023 at 14:36):

bjorn3 commented on issue #6765:

Oh nice! I was also thinking of issues though with LTO where rlib+cdylib has at least caused problems there where LTO settings effectively don't apply. That may have been fixed, though, I'm not sure.

Seems not. echo | rustc - --crate-type rlib --crate-type cdylib -Clto=thin gives error: lto can only be run for executables, cdylibs and static library outputs

view this post on Zulip Wasmtime GitHub notifications bot (Jul 27 2023 at 10:32):

RubixDev commented on issue #6765:

Hm ok it might be good to better understand your use case first then. This is a recipe for things going wrong where a Rust crate depends on a C library which depends on a Rust library. There's two versions of the rust standard library in that final program and that may or may not be expected (and may also cause things like linking issues).

Well, the proposed solution only has one Rust and one C part which interoperate. The tree-sitter Rust bindings call functions from the C part, and the C part calls the wasmtime functions from the Rust part. Note that the wasmtime functions are in the same Rust binary as the tree-sitter bindings (because the wasmtime C API is used as a dependency) so there will only be one Rust stdlib and no linking issues. The use of the C API is required as tree-sitter should also be usable from C, just that there users will have to manually provide wasmtime.

I'm also still not sure how this PR is expected to be integrated successfully? Why is reexporting wasmtime::* necessary? Are you using both Wasmtime's C API and the native Rust API? Or are the Rust bindings for tree-sitter exclusively using tree-sitter's own C API?

The Rust bindings don't interface with the wasmtime C API directly, but the C code called from the Rust bindings uses the wasmtime C API. Additionally, the Rust bindings use a small part of the native Rust API, actually just Engine. Due to the lib name clash between wasmtime and wasmtime-c-api, a re-export is required for the usage of both crates simultaneously.

In the end though if this works for you (it'd be good to end-to-end test to confirm) then my concerns are pretty niche and we can go ahead and merge it.

I'm not sure what exactly you mean by "end-to-end test", but I've successfully used the fork locally.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 07 2023 at 14:21):

alexcrichton commented on issue #6765:

Can you explain more about why pub use is required? That's venturing more into the territory of this seems to situation-solve a specific use case which isn't documented and won't be guarnateed to continue working. For example that's a very easy thing to overlook and accidentally remove during a refactoring or something like that.

Again though I don't know if there are any widely used idioms about defining C APIs in Rust and using them from Rust as well. This is not a use case I've ever seen before work successfully, so I don't know how best to organize this.

Note that the wasmtime functions are in the same Rust binary as the tree-sitter bindings (because the wasmtime C API is used as a dependency)

I'll note as well that what you're describing here is a circular dependency between libraries which is not guaranteed to work with all native linkers. To get circular dependencies to always work native linkers have --start-group and --end-group options (or similar) which are required to indicate that there are circular references within that set of arguments passed. The Rust compiler would not pass these for arbitrary libraries and there's no means of doing so in general. While some platforms may work (e.g. the ones you've tested) it's unlikely to work on all platforms or into the future (e.g. in the face of future refactorings)

Sorry if I may be repeating myself, I've been doing a bit of travel and lost context on this...

view this post on Zulip Wasmtime GitHub notifications bot (Sep 28 2023 at 23:19):

maxbrunsfeld commented on issue #6765:

:wave: I wrote the Tree-sitter PR that gave rise to this change. It'd be great to land this functionality, so that Tree-sitter can provide the WASM plugin functionality that's implemented in https://github.com/tree-sitter/tree-sitter/pull/1864, without having to depend on a fork of wasmtime.

I'll note as well that what you're describing here is a circular dependency between libraries which is not guaranteed to work with all native linkers

I don't think there's a dependency cycle here. The dependencies between the different compilation units are as follows:

(1. Tree-sitter Rust crate)
  |
  +---> (2. Tree-sitter C library)
  |      |
  +------+----> (3. Wasmtime C API)
  |      |
  +------+----> (4. Wasmtime Rust Crate)

All of the edges in this graph are expressed directly to Cargo, except for the dependency between 2 and 3 (because 2 is a C library compiled in a build script). When compiling with Cargo, the linking works fine though, I imagine because 1 and 2 are part of the same crate, and 1 has a Cargo dependency on 3.

It seems to work without issue on all of the platforms I've tried it on, and it's useful.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 28 2023 at 23:23):

maxbrunsfeld edited a comment on issue #6765:

:wave: I wrote the Tree-sitter PR that gave rise to this change. It'd be great to land this functionality, so that Tree-sitter can provide the WASM plugin functionality that's implemented in https://github.com/tree-sitter/tree-sitter/pull/1864, without having to depend on a fork of wasmtime.

I'll note as well that what you're describing here is a circular dependency between libraries which is not guaranteed to work with all native linkers

I don't think there's a dependency cycle here. The dependencies between the different compilation units are as follows:

(1. Tree-sitter Rust crate)
 |
 +---> (2. Tree-sitter C library)
 |      |
 +------+------> (3. Wasmtime C API)
 |                |
 +----------------+------> (4. Wasmtime Rust Crate)

All of the edges in this graph are expressed directly to Cargo, except for the dependency between 2 and 3 (because 2 is a C library compiled in a build script). When compiling with Cargo, the linking works fine though, I imagine because 1 and 2 are part of the same crate, and 1 has a Cargo dependency on 3.

It seems to work without issue on all of the platforms I've tried it on, and it's useful.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 28 2023 at 23:29):

maxbrunsfeld commented on issue #6765:

Can you explain more about why pub use is required?

The pub use isn't strictly required, but we do need some way to access the items in the wasmtime crate, while depending on the wasmtime-c-api crate. Right now, we're prevented from importing anything from wasmtime while depending on wasmtime-c-api, because wasmtime-c-api does this in its Cargo.toml:

https://github.com/bytecodealliance/wasmtime/blob/main/crates/c-api/Cargo.toml#L12-L13

[lib]
name = "wasmtime"

view this post on Zulip Wasmtime GitHub notifications bot (Sep 29 2023 at 14:08):

alexcrichton commented on issue #6765:

Ok thanks for coming back to this, and given how useful it is for y'all I think this is ok to land. If issues come up we can try to work with you to figure out how to fix it (if any), and otherwise can take care of things in the meantime.

Testing locally I don't think the pub use is required with a bit of a Cargo workaround:

[dependencies]
wasmtime = "*"
this-is-your-name = { version = "*", package = 'wasmtime-c-api' }

The package key tricks cargo into passing --extern this_is_your_name=.... Note that this-is-your-name could even be wasmtime-c-api which would allow Rust code to use wasmtime_c_api::....

I'm going to go ahead and queue this up for merge and I'll send a follow-up to remove the pub use under the assumption that it's best to go ahead and land this somewhat-old PR rather than ask for further changes.

Thank you again for following up, it's appreciated!

view this post on Zulip Wasmtime GitHub notifications bot (Sep 29 2023 at 16:26):

maxbrunsfeld commented on issue #6765:

Thanks so much @alexcrichton, and I really appreciate that solution of using a custom dependency name. I didn't know you could do that!

Last updated: Nov 22 2024 at 17:03 UTC