feat(rust) add native link option - #1685
BjornTheProgrammer wants to merge 6 commits into
Conversation
|
Could you address the question I had in the other PR? This seems like a lot to take on in terms of maintainership here because only codegen is tested, not any runtime parts, and since that comment the component model has continued to add features like async which I'm not sure how would map to native counterparts. |
|
Sure! I'll respond to the previous comment.
Yes. This has been demonstrated in the Pumpkin PR I shared. It allows for native and wasm targets to be compiled interchangeably with no code changes from the plugin developer.
The reason a direct native call doesn't work here is that the guest is compiled separately from the host and loaded with dlopen. Rust has no stable ABI, so passing native types like String or Vec across that boundary, potentially across different rustc versions, or from a non-Rust host won't work. The canonical ABI is already implemented by wit-bindgen and tapping into that is easy. We wouldn't really have to "serialize/deserialize." Nothing is encoded to a buffer. Scalars pass as-is and strings cross as pointer+length, so simple calls are effectively direct calls already. Reusing the existing ABI also keeps the change small. The existing codegen already emits the full ABI representation on native targets (the unreachable!() stubs sit after all of that), so this PR just swaps the stub for a dispatch through a registered function pointer and hex-encodes the symbol names. Only around +130 lines of actual generator code. Most of that is the register hooks, which exist so the host doesn't have to re-export every import symbol into the dlopen'd library (the -rdynamic problem) and can implement only the subset of imports it needs. And because it's the same underlying C ABI emission, async, resources, and every other WIT construct should work without a parallel code path. Which is why I didn't really see the need to test the runtime part, since whatever ABI works on wasm should just work with native. A direct-call mode (traits with idiomatic signatures) is imaginable, but I don't think it fits wit-bindgen's shape. There's no language-neutral contract for it to target, so it would be a separate parallel bindings mode per generator, none of which could interoperate across the dlopen or language boundary. The canonical ABI is the one representation every generator here already shares. Which is what lets, e.g., a C++ host load a Rust plugin under this scheme. Theoretically this could be added to the other generators as well. |
|
@cpetig I know you've done similar work, so it'd be great if you could weigh in here as well. |
|
Wow, good to see growing interest in applying the component model to native binaries. I have a long living fork of wit-bindgen at https://github.com/cpetig/wit-bindgen/ which adds
The symmetric ABI is what makes the real difference in usability as the (also mental) overhead for (maintaining) a host mode (mesh) code generator is significant. I guess we should have a discussion about standardizing native names in a |
cpetig
left a comment
cpetig
left a comment
There was a problem hiding this comment.
I feel that the amount of code changes can be reduced by making the native symbol name generation and target-conditional linker attributes the default behavior. This is how I would solve this.
| /// // still does; use `type_section_suffix` to tell them apart. See | ||
| /// // `wit_bindgen_rust::Opts::link_native_symbols` for the full list of | ||
| /// // symbols a host can expect. | ||
| /// link_native_symbols: true, |
There was a problem hiding this comment.
(written before I saw that your code is likely the same when the native flag is turned on)
I think my approach of using conditional compilation to choose between both naming schemes at compilation time of identical generated code is preferable. See e.g. https://github.com/cpetig/wit-bindgen/blob/work-in-progress/crates/cpp/tests/symmetric_future/future/src/future_world.rs#L17-L24 for an example.
| /// `#`, `[` and `]` characters that canonical names contain. Names that | ||
| /// survive encoding unchanged (`$root` exports, for instance) are emitted | ||
| /// once with no `cfg` rather than twice. | ||
| fn core_export_symbols(&self, export_name: &str) -> Vec<(&'static str, String)> { |
There was a problem hiding this comment.
ok, I see, this is additive, so the generated code is unchanged in the default case.
I will leave this decision to Alex, but for my branch I decided that it should become the default, simplifying the generation logic and making the flag unnecessary. (It only adds visual clutter to the generated code, no runtime effects)
There was a problem hiding this comment.
I pushed some changes that now make it the default as well.
|
@cpetig thank you for the review! I hadn't realized you had a fork tackling this issue as well, and I would love to combine our work. We're already aligned on naming (the hex encoding this PR moves into wit-bindgen-core is the same scheme your C++ generator uses). I've also taken your suggestion. Native symbol name generation and target-conditional linker attributes are now the default behavior, and the link_native_symbols option is gone entirely. Exports are emitted once with paired cfg_attr(target_arch = "wasm32", ...) export names. The one place I deliberately differ from your fork is imports. Instead of undefined externs, the old unreachable!() slot is filled with a dispatch through a _wit_bindgen_register(func: unsafe extern "C" fn(...)) hook. That keeps the default free for existing users (bindings crates still build and link everywhere on host targets. No import stub libraries needed, and calling an unregistered import aborts with a message, same as the stub did), and it's what makes runtime-discovered plugins work: a dlopen'd cdylib can't resolve import symbols against the host executable without -rdynamic on Linux, and can't on Windows at all, which is why I abandoned #1565. The hooks are convention-agnostic. They just carry a core-signature function pointer, so a symmetric mode could register through the same mechanism later. On the symmetric ABI, I had considered something similar for this PR, but I was worried about whether something requiring maintenance at that scale would be accepted, which is why I settled on the existing wasm32 ABI. It also helped that wasmtime's bindgen existed as a reference for making a fully working native host runtime (native-wit). A lot of our changes are shared regardless. Would it make sense to land this PR as the common substrate (standardized names, no stubs, a portable dispatch mechanism), and then adopt the symmetric ABI as an opt-in mode once the BuildTargets.md discussion stabilizes? Happy to help draft the naming section there. The encoding and hook naming in this PR could serve as input. |
|
Hmm seems like a single unit test fails due to a naming conflict after my changes. I just had an idea how to fix this that might make naming resolution never a problem and also simplify the rest of the code. I'll probably push some changes tomorrow, but for now I would love to hear your input! |
f2ddae4 to
ae12065
Compare
ae12065 to
daf88e2
Compare
|
I've made some changes to how I register exports. Additionally I've added support for async, futures, and streams in native. I also made an end-to-end test to validate that nothing breaks between versions using libloading. Please let me know what you think about it. |
f3db443 to
5ceeb2e
Compare
|
Um, is there any progress in this PR? I noticed the last commit was a week ago, I'm just wondering. |
I've made all of the changes that I felt were needed, and the CI just failed a single test because I think it timed out. Should be ready for review. |
|
What's the verdict on this? Still interested in having this? |
|
Apologies I've definitely let this languish and haven't been attentive. Overall on this I feel like I'm missing something pretty fundamental or not understanding something, and by missing that I'm wary to merge this in and try to maintain it. Having tests is definitely good, and I agree this isn't the biggest change ever, but despite these parts I'm still not entirely sure what to do about this. One thing I'm particulary wary of is what this test is doing internally which I'm assuming is roughly what external consumers would also be doing -- the Another piece I haven't thought too carefully about is all the memory management/ownership/lifetimes w.r.t the canonical ABI and these functions. For example wasm exports receiving a string inherently expect an owned allocation to be received, but when calling a wasm import that takes a string no allocation is made. This is part of the implicit assumption that something else is what's translating between addresss spaces, but by putting everything in the same process that's breaking a pretty fundamental assumption. At a skim of this PR I'm not seeing where this is handled for example and where the export either no longer assumes it owns the memory or the caller allocates separate memory. I guess put a bit more broadly everything about WIT and wit-bindgen and such has so far been developed without much heed to a use case like native plugins or native dynamic libraries. I'm wary of breaking this foundational assumption which can have rippling effects far beyond just some tests in this repo or similar. Now there's no really any way to quantify this one way or another, so I'm mostly just trying to explain my rationale of being wary -- I don't think it's possible for me to not be wary of a change like this but that doesn't mean it can't happen. That's not really an answer for a verdict per-se, but I'm also not really equipped to say "definitely yes" or "definitely no" to a change like this. I can try to help out and explain some of the concerns/wariness I have and see how this takes shape over time, but I definitely don't have a picture in my head of "if you did this it'd be landable" for example. |
|
@alexcrichton No worries, and thanks for writing all this out!
I think I explained the split badly in the PR. This PR is only the guest side. The test hand-writes The safe part is supposed to live in a separate host crate, the same way wasmtime does for wasm. That's what native-wit is. It has a Note that native-wit is still relatively unpolished, and more of a proof of concept then what the actual final product would look like. // the one unsafe call: "this is a trusted plugin for this world"
let plugin = unsafe { native::Plugin::new(&path)? };
// safe from here on, same API as wasmtime
plugin.call_init_plugin(&mut store).await?;After loading the library, There's one hole I know about: the marker only covers the package name, version, and world name, not the actual WIT contents. So if someone changes an interface without bumping the version, an old plugin still passes the check. I can switch the marker to include a hash of the world in this PR, which would make that a load-time error too.
This PR doesn't change anything about ownership on the guest side. The generated code assumes exactly what it does on wasm. The "something else translating between address spaces" still exists, it's just the native host now instead of wasmtime. In native-wit:
So nothing ever gets freed by a different allocator than the one that allocated it, which also matters because every You're right that none of this is written down in the PR. I wrote it up in the native-wit README and can add a shorter version here as the contract a native host needs to follow.
Yeah, that makes sense. What I'd say is that this PR doesn't change the guest-side model at all. The canonical ABI and ownership rules stay the same. What's new is the symbol naming (hex encoding, same scheme as @cpetig's fork), the world marker, exporting the allocator, and the resolver hook replacing the For a real-world check, Pumpkin has a pretty big wasmtime + wit-bindgen plugin API, and these are all the changes it took to load native plugins next to wasm ones. The existing
Somethings that could be done to make this more landable is make the world marker include a hash of the WIT contents, put the native output behind an experimental off-by-default option if you'd rather it not be the default, and/or keep all the host-side generation in native-wit so the maintenance cost here is just the guest changes. Would that be enough for you to feel ok maintaining it? If you'd rather wait until native naming gets standardized (e.g. WebAssembly/component-model#378), I'm also happy to help push that along with @cpetig. |
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Ok thanks for writing all that out! That seems to me like a reasonable middle-ground here. I do think it'd be good to have some long-form documentation about how native is intended to work and what's there, but that's fine to live either in this repo or in your repo externally since that's where the primary development point would be.
Otherwise though I've got some comments below, but the main one is that I'm wondering if the generator changes can be simplified by using the helper macro that the rt module itself uses in the generated code since that'd encapsulate the implementation details and such.
|
|
||
| /// Native counterpart of the C-defined `wasip3_task_set` above. Uses | ||
| /// thread local so there is no possible panic on multi thread when polling | ||
| /// two async exports at the same time. | ||
| #[cfg(all(not(target_family = "wasm"), feature = "std"))] | ||
| pub unsafe fn wasip3_task_set(ptr: *mut wasip3_task) -> *mut wasip3_task { | ||
| use core::cell::Cell; | ||
| std::thread_local!( | ||
| static CURRENT: Cell<*mut wasip3_task> = const { Cell::new(core::ptr::null_mut()) } | ||
| ); | ||
| CURRENT.with(|current| current.replace(ptr)) | ||
| } |
There was a problem hiding this comment.
FWIW the purpose of this symbol on wasm was to specifically have a weak definition to handle cases where imports are using one version of wit-bindgen and exports are using another. I suspect that'll come up less rarely for your native use case, but perhaps something to consider.
There was a problem hiding this comment.
i believe it can also happen with set_import_resolver and cabi_realloc which cause a duplicate symbol error
| /// Without `std` there are no thread-locals on stable Rust, so this falls | ||
| /// back to one global slot, which requires the host to not poll two async | ||
| /// exports at the same time. | ||
| #[cfg(all(not(target_family = "wasm"), not(feature = "std")))] | ||
| pub unsafe fn wasip3_task_set(ptr: *mut wasip3_task) -> *mut wasip3_task { | ||
| use core::sync::atomic::{AtomicPtr, Ordering}; | ||
| static CURRENT: AtomicPtr<wasip3_task> = AtomicPtr::new(core::ptr::null_mut()); | ||
| CURRENT.swap(ptr, Ordering::AcqRel) | ||
| } |
There was a problem hiding this comment.
I'm kind of hesitant to do this sort of fallback because sometimes it's appropriate and sometimes not. Would it be possible to somehow hook this up into the resolver for native?
| static CACHE: ::core::sync::atomic::AtomicPtr<()> = | ||
| ::core::sync::atomic::AtomicPtr::new(::core::ptr::null_mut()); | ||
| // Named so as not to shadow any parameter. | ||
| let mut __impl = CACHE.load(::core::sync::atomic::Ordering::Acquire); | ||
| if __impl.is_null() { | ||
| __impl = crate::rt::resolve_import( | ||
| crate::rt::native_imports::cstr(concat!($module, "\0")), | ||
| crate::rt::native_imports::cstr(concat!($name, "\0")), | ||
| ); | ||
| CACHE.store(__impl, ::core::sync::atomic::Ordering::Release); | ||
| } | ||
| let __func: unsafe extern "C" fn($($ty),*) $(-> $ret)? = | ||
| unsafe { ::core::mem::transmute(__impl) }; |
There was a problem hiding this comment.
To reduce the size of the macro expansion here a bit, could this be restructured as:
fn foo() {
static CACHE: ... = ...;
let ptr = $crate::rt::resolve_import(module, name, &CACHE);
unsafe { transmute::<A, B>(ptr)(...) }
}basically moving the if and load/store to the rt module where resolve_import could have a fast/cold path similar to what the macro has today to basically generate the same code
| return ptr; | ||
| } | ||
|
|
||
| #[cfg(not(target_arch = "wasm32"))] |
There was a problem hiding this comment.
Mind switching to target_family = "wasm"? I've been meaning to do that for the whole crate for some time and just haven't gotten around to it.
| #[unsafe(no_mangle)] | ||
| pub unsafe extern "C" fn __wit_bindgen_set_import_resolver( | ||
| resolver: Option<ImportResolver>, | ||
| ctx: *mut (), | ||
| ) { | ||
| let resolver = match resolver { | ||
| Some(resolver) => resolver as *mut (), | ||
| None => core::ptr::null_mut(), | ||
| }; | ||
| RESOLVER_CTX.store(ctx, Ordering::Relaxed); | ||
| RESOLVER.store(resolver, Ordering::Release); | ||
| } |
There was a problem hiding this comment.
Would it be possible to not worry about no_mangle here and let external callers do that? I'm otherwise a bit worried about mixing multiple versions of wit-bindgen and having the symbols clash
There was a problem hiding this comment.
Otherwise though the ownership/uninstallation/synchronization/etc story here is a bit suspect -- could this move to being under a lock?
For example some possible issues with this current protocol are:
- Concurrent invocations of setting the resolve can corrupt the resolver/ctx static
- Uninstalling a resolver doesn't mean it's not in use by other threads
| let module_display = module.to_string_lossy(); | ||
| let name_display = name.to_string_lossy(); |
There was a problem hiding this comment.
It seems like some care is taken for efficiency to move the module/name around as a CStr as opposed to a &str which is relatively unidiomatic, but here ther're immediately turned into Rust strings which is a pretty heavyweight operation and additionally seems to sort of defeat the purpose of optimizing things here. Is the intentiona that this doesn't happen in typical execution?
| /// `component-type` custom section on wasm: binding the same world twice in | ||
| /// one binary needs a `type_section_suffix` (and `export_prefix` for the | ||
| /// export names). | ||
| /// |
There was a problem hiding this comment.
I'm a bit wary to document too much here because this crate is sort of only half the story. Could the section here be generally reworded to say "things compile, but won't work by default. If you want things to work go to this URL over here for more information" where that URL is either one you host or some *.md in this repo
| // Natively the intrinsics are resolved through the host's import | ||
| // resolver, exactly like every other import. | ||
| let rt = self.r#gen.runtime_path(); | ||
| let handle = || ("handle".to_string(), "u32"); | ||
| let mut extra: Vec<(String, &str)> = Vec::new(); | ||
| if let PayloadFor::Stream = payload_for { | ||
| extra.push(("amt".to_string(), "usize")); | ||
| } | ||
| let mut native_intrinsics = String::new(); | ||
| for (rust_name, core_name, params, result) in [ |
There was a problem hiding this comment.
Instead of hand-writing this, could this maybe be redone by exporting the helper macro in the wit-bindgen crate (probably as #[doc(hidden)]) and then that's used here?
| /// Emits the world's native marker symbol, `__wit_bindgen_world_<world>`, | ||
| /// which hosts look up before calling anything else to check that the | ||
| /// library they opened implements the world they expect. It's keyed on | ||
| /// the world name and `type_section_suffix`, like the `component-type` | ||
| /// section on wasm. | ||
| fn finish_native_world_marker(&mut self, resolve: &Resolve, world: WorldId) { | ||
| let world = &resolve.worlds[world]; | ||
| let pkg = world | ||
| .package | ||
| .map(|p| resolve.packages[p].name.to_string()) | ||
| .unwrap_or_default(); | ||
| let name = format!("{pkg}/{}", world.name); | ||
| let suffix = self.opts.type_section_suffix.as_deref().unwrap_or(""); | ||
| let symbol = symbol_name::make_external_component(&format!("{name}{suffix}")); | ||
| uwriteln!( | ||
| self.src, | ||
| r#" | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| #[unsafe(no_mangle)] | ||
| #[allow(non_snake_case)] | ||
| pub extern "C" fn __wit_bindgen_world_{symbol}() -> *const ::core::ffi::c_char {{ | ||
| c"{name}".as_ptr() | ||
| }} | ||
| "# | ||
| ); | ||
| } |
There was a problem hiding this comment.
So for this, ideally what this would do would be to return the wasm-world-encoded-as-WIT, much like the custom section present for wasm today. Whether that's a function or a section in the object I don't mind either way, but in the long run you'll probably want the full WIT type information as opposed to just the world name.
| # Deliberately not a member of the wit-bindgen workspace: `tests/native_e2e.rs` | ||
| # builds this crate with a nested `cargo build` and `dlopen`s the result. | ||
| [workspace] |
There was a problem hiding this comment.
Maintaining multiple Cargo.locks and such, or not having one here, is something that I'm a bit worried about in terms of flaky CI failures. I'm also a bit worried from earlier about the degree of unsafety w.r.t. how the host side of the test is written (which is autogenerated mostly you say on your side).
Given that, what would you think about either removing this test or reducing its scope? For example asserting it can be compiled would be fine, but actually running the output seems like it sort of best needs the other half of running things which lives elsewhere. If that becomes problematic over time we can probably try to figure out some other scheme, but I'm hopeful that things aren't changing too rapidly such that if you've tested this PR then future PRs have a low chance of breakage.
850e186 to
321ac67
Compare
This branch has not been deployed
7 participants
Addresses bytecodealliance/wit-bindgen#1062. Currently the Rust bindings stub with
unreachable!()when compiling to wasm. This PR makes bindings usable on native targets, so people can test and run component code in ordinary native environments.This is a new approach to what was initially done in #1565.
The reason this PR needs to exist is described by a comment in the original PR
To solve this issue I've added a register function which can add a pointer to the host function. This fixes the above issue.
Testing
The Pumpkin PR is interesting because the approach shows how one can make very minimal changes and get wasmtime and native runtimes working at the same time.