From d8fc3c3daac2b147096ce39c229a5755768445a9 Mon Sep 17 00:00:00 2001 From: Jeffrey Charles Date: Tue, 19 Nov 2024 14:24:10 -0500 Subject: [PATCH] Add support for plugins to dynamically linked modules (#821) --- crates/cli/src/codegen/builder.rs | 5 --- crates/cli/src/codegen/mod.rs | 37 ++++++++++++--------- crates/cli/src/commands.rs | 33 +++++++++++++++---- crates/cli/src/js_config.rs | 5 --- crates/cli/src/plugins.rs | 5 +++ crates/cli/tests/dynamic_linking_test.rs | 41 +++++++++++++++++------- crates/runner/src/lib.rs | 39 ++++++++++++---------- crates/test-macros/src/lib.rs | 2 +- docs/docs-using-dynamic-linking.md | 2 +- docs/docs-using-nodejs.md | 10 +++--- 10 files changed, 111 insertions(+), 68 deletions(-) diff --git a/crates/cli/src/codegen/builder.rs b/crates/cli/src/codegen/builder.rs index ff02a5b3..211aa63d 100644 --- a/crates/cli/src/codegen/builder.rs +++ b/crates/cli/src/codegen/builder.rs @@ -84,11 +84,6 @@ impl CodeGenBuilder { /// Build a [`CodeGenerator`]. pub fn build(self, ty: CodeGenType, js_runtime_config: JsConfig) -> Result { - if let CodeGenType::Dynamic = ty { - if js_runtime_config.has_configs() { - bail!("Cannot set JS runtime options when building a dynamic module") - } - } let mut generator = Generator::new(ty, js_runtime_config, self.plugin); generator.source_compression = self.source_compression; generator.wit_opts = self.wit_opts; diff --git a/crates/cli/src/codegen/mod.rs b/crates/cli/src/codegen/mod.rs index 0e26f8e4..d742b060 100644 --- a/crates/cli/src/codegen/mod.rs +++ b/crates/cli/src/codegen/mod.rs @@ -203,9 +203,21 @@ impl Generator { canonical_abi_realloc_type, ); - let eval_bytecode_type = module.types.add(&[ValType::I32, ValType::I32], &[]); - let (eval_bytecode_fn_id, _) = - module.add_import_func(&import_namespace, "eval_bytecode", eval_bytecode_type); + // User plugins can use `invoke` with a null function name. + // User plugins also won't have an `eval_bytecode` function to + // import. We want to remove `eval_bytecode` from the default + // plugin so we don't want to emit more uses of it. + let eval_bytecode_fn_id = if self.plugin.is_v2_plugin() { + let eval_bytecode_type = module.types.add(&[ValType::I32, ValType::I32], &[]); + let (eval_bytecode_fn_id, _) = module.add_import_func( + &import_namespace, + "eval_bytecode", + eval_bytecode_type, + ); + Some(eval_bytecode_fn_id) + } else { + None + }; let invoke_type = module.types.add( &[ValType::I32, ValType::I32, ValType::I32, ValType::I32], @@ -226,7 +238,7 @@ impl Generator { Ok(Identifiers::new( canonical_abi_realloc_fn_id, - Some(eval_bytecode_fn_id), + eval_bytecode_fn_id, invoke_fn_id, memory_id, )) @@ -269,18 +281,12 @@ impl Generator { .call(eval_bytecode); } else { // Assert we're not emitting a call with a null function to - // invoke for `javy_quickjs_provider_v*`. - // `javy_quickjs_provider_v2` will never support calling `invoke` - // with a null function. Older `javy_quickjs_provider_v3`'s do not - // support being called with a null function. User plugins and - // newer `javy_quickjs_provider_v3`s do support being called with a - // null function. - // Using `assert!` instead of `debug_assert!` because integration - // tests are executed with Javy built with the release profile so - // `debug_assert!`s are stripped out. + // invoke for the v2 plugin. `javy_quickjs_provider_v2` will never + // support calling `invoke` with a null function. The default + // plugin and user plugins do accept null functions. assert!( - self.plugin.is_user_plugin(), - "Using invoke with null function only supported for user plugins" + !self.plugin.is_v2_plugin(), + "Using invoke with null function not supported for v2 plugin" ); instructions .local_get(bytecode_ptr_local) // ptr to bytecode @@ -500,7 +506,6 @@ mod test { JsConfig::default(), Plugin::Default, ); - assert!(!gen.js_runtime_config.has_configs()); assert!(gen.source_compression); assert!(matches!(gen.plugin, Plugin::Default)); assert_eq!(gen.wit_opts, WitOptions::default()); diff --git a/crates/cli/src/commands.rs b/crates/cli/src/commands.rs index 95d5018a..882802bf 100644 --- a/crates/cli/src/commands.rs +++ b/crates/cli/src/commands.rs @@ -234,11 +234,18 @@ impl TryFrom>> for CodegenOptionGroup { options.wit = WitOptions::from_tuple((wit.cloned(), wit_world.cloned()))?; - // This is temporary. So I can make the change for dynamic modules - // separately because it will be a breaking change. - if options.dynamic && options.plugin.is_some() { - bail!("Cannot use plugins for building dynamic modules"); + // We never want to assume the import namespace to use for a + // dynamically linked module. If we do assume the import namespace, any + // change to that assumed import namespace can result in new + // dynamically linked modules not working on existing execution + // environments because there will be unmet import errors when trying + // to instantiate those modules. Since we can't assume the import + // namespace, we must require a plugin so we can derive the import + // namespace from the plugin. + if options.dynamic && options.plugin.is_none() { + bail!("Must specify plugin when using dynamic linking"); } + Ok(options) } } @@ -353,6 +360,8 @@ impl JsConfig { #[cfg(test)] mod tests { + use std::path::PathBuf; + use crate::{ commands::{JsGroupOption, JsGroupValue}, js_config::JsConfig, @@ -360,12 +369,11 @@ mod tests { }; use super::{CodegenOption, CodegenOptionGroup, GroupOption}; - use anyhow::Result; + use anyhow::{Error, Result}; #[test] fn js_config_from_config_values() -> Result<()> { let group = JsConfig::from_group_values(&Plugin::Default, vec![])?; - assert!(!group.has_configs()); assert_eq!(group.get("redirect-stdout-to-stderr"), None); assert_eq!(group.get("javy-json"), None); assert_eq!(group.get("javy-stream-io"), None); @@ -501,10 +509,14 @@ mod tests { let group: CodegenOptionGroup = vec![].try_into()?; assert_eq!(group, CodegenOptionGroup::default()); - let raw = vec![GroupOption(vec![CodegenOption::Dynamic(true)])]; + let raw = vec![GroupOption(vec![ + CodegenOption::Dynamic(true), + CodegenOption::Plugin(PathBuf::from("file.wasm")), + ])]; let group: CodegenOptionGroup = raw.try_into()?; let expected = CodegenOptionGroup { dynamic: true, + plugin: Some(PathBuf::from("file.wasm")), ..Default::default() }; @@ -519,6 +531,13 @@ mod tests { assert_eq!(group, expected); + let raw = vec![GroupOption(vec![CodegenOption::Dynamic(true)])]; + let result: Result = raw.try_into(); + assert_eq!( + result.err().unwrap().to_string(), + "Must specify plugin when using dynamic linking" + ); + Ok(()) } } diff --git a/crates/cli/src/js_config.rs b/crates/cli/src/js_config.rs index 463f74a1..608b5b50 100644 --- a/crates/cli/src/js_config.rs +++ b/crates/cli/src/js_config.rs @@ -11,11 +11,6 @@ impl JsConfig { JsConfig(configs) } - /// Returns true if any configs are set. - pub(crate) fn has_configs(&self) -> bool { - !self.0.is_empty() - } - /// Encode as JSON. pub(crate) fn to_json(&self) -> Result> { Ok(serde_json::to_vec(&self.0)?) diff --git a/crates/cli/src/plugins.rs b/crates/cli/src/plugins.rs index c94d002c..17289b50 100644 --- a/crates/cli/src/plugins.rs +++ b/crates/cli/src/plugins.rs @@ -57,6 +57,11 @@ impl Plugin { matches!(&self, Plugin::User { .. }) } + /// Returns true if the plugin is the legacy v2 plugin. + pub fn is_v2_plugin(&self) -> bool { + matches!(&self, Plugin::V2) + } + /// Returns the plugin Wasm module as a byte slice. pub fn as_bytes(&self) -> &[u8] { match self { diff --git a/crates/cli/tests/dynamic_linking_test.rs b/crates/cli/tests/dynamic_linking_test.rs index 8890bf7d..b4dee4f9 100644 --- a/crates/cli/tests/dynamic_linking_test.rs +++ b/crates/cli/tests/dynamic_linking_test.rs @@ -56,11 +56,26 @@ fn test_errors_in_exported_functions_are_correctly_reported(builder: &mut Builde Ok(()) } -#[javy_cli_test(dyn = true, root = "tests/dynamic-linking-scripts")] +#[javy_cli_test( + dyn = true, + root = "tests/dynamic-linking-scripts", + commands(not(Compile)) +)] // If you need to change this test, then you've likely made a breaking change. pub fn check_for_new_imports(builder: &mut Builder) -> Result<()> { let runner = builder.input("console.js").build()?; - runner.ensure_expected_imports() + runner.ensure_expected_imports(false) +} + +#[javy_cli_test( + dyn = true, + root = "tests/dynamic-linking-scripts", + commands(not(Build)) +)] +// If you need to change this test, then you've likely made a breaking change. +pub fn check_for_new_imports_for_compile(builder: &mut Builder) -> Result<()> { + let runner = builder.input("console.js").build()?; + runner.ensure_expected_imports(true) } #[javy_cli_test(dyn = true, root = "tests/dynamic-linking-scripts")] @@ -93,9 +108,9 @@ fn test_using_runtime_flag_with_dynamic_triggers_error(builder: &mut Builder) -> .input("console.js") .redirect_stdout_to_stderr(false) .build(); - assert!(build_result.is_err_and(|e| e - .to_string() - .contains("Error: Cannot set JS runtime options when building a dynamic module"))); + assert!(build_result.is_err_and(|e| e.to_string().contains( + "error: Property redirect-stdout-to-stderr is not supported for runtime configuration" + ))); Ok(()) } @@ -119,12 +134,16 @@ fn javy_json_identity(builder: &mut Builder) -> Result<()> { } #[javy_cli_test(dyn = true, commands(not(Compile)))] -fn test_using_plugin_with_dynamic_build_fails(builder: &mut Builder) -> Result<()> { - let result = builder.plugin(Plugin::User).input("plugin.js").build(); - let err = result.err().unwrap(); - assert!(err - .to_string() - .contains("Cannot use plugins for building dynamic modules")); +fn test_using_plugin_with_dynamic_works(builder: &mut Builder) -> Result<()> { + let plugin = Plugin::User; + let mut runner = builder + .plugin(Plugin::User) + .preload(plugin.namespace().into(), plugin.path()) + .input("plugin.js") + .build()?; + + let result = runner.exec(&[]); + assert!(result.is_ok()); Ok(()) } diff --git a/crates/runner/src/lib.rs b/crates/runner/src/lib.rs index 7a28b412..aaabb129 100644 --- a/crates/runner/src/lib.rs +++ b/crates/runner/src/lib.rs @@ -23,6 +23,8 @@ pub enum Plugin { V2, Default, User, + /// Pass the default plugin on the CLI as a user plugin. + DefaultAsUser, } impl Plugin { @@ -31,7 +33,7 @@ impl Plugin { Self::V2 => "javy_quickjs_provider_v2", // Could try and derive this but not going to for now since tests // will break if it changes. - Self::Default => "javy_quickjs_provider_v3", + Self::Default | Self::DefaultAsUser => "javy_quickjs_provider_v3", Self::User { .. } => "test_plugin", } } @@ -47,7 +49,7 @@ impl Plugin { .join("src") .join("javy_quickjs_provider_v2.wasm"), Self::User => root.join("test_plugin.wasm"), - Self::Default => root + Self::Default | Self::DefaultAsUser => root .join("..") .join("..") .join("target") @@ -428,7 +430,7 @@ impl Runner { }) } - pub fn ensure_expected_imports(&self) -> Result<()> { + pub fn ensure_expected_imports(&self, expect_eval_bytecode: bool) -> Result<()> { let module = Module::from_binary(self.linker.engine(), &self.wasm)?; let instance_name = self.plugin.namespace(); @@ -436,8 +438,9 @@ impl Runner { .imports() .filter(|i| i.module() == instance_name) .collect::>(); - if imports.len() != 4 { - bail!("Dynamically linked modules should have exactly 4 imports for {instance_name}"); + let expected_import_count = if expect_eval_bytecode { 4 } else { 3 }; + if imports.len() != expected_import_count { + bail!("Dynamically linked modules should have exactly {expected_import_count} imports for {instance_name}"); } let realloc = imports @@ -458,17 +461,19 @@ impl Runner { .find(|i| i.name() == "memory" && i.ty().memory().is_some()) .ok_or_else(|| anyhow!("Should have memory import named memory"))?; - let eval_bytecode = imports - .iter() - .find(|i| i.name() == "eval_bytecode") - .ok_or_else(|| anyhow!("Should have eval_bytecode import"))?; - let ty = eval_bytecode.ty(); - let f = ty.unwrap_func(); - if !f.params().all(|p| p.is_i32()) || f.params().len() != 2 { - bail!("eval_bytecode should accept 2 i32s as parameters"); - } - if f.results().len() != 0 { - bail!("eval_bytecode should return no results"); + if expect_eval_bytecode { + let eval_bytecode = imports + .iter() + .find(|i| i.name() == "eval_bytecode") + .ok_or_else(|| anyhow!("Should have eval_bytecode import"))?; + let ty = eval_bytecode.ty(); + let f = ty.unwrap_func(); + if !f.params().all(|p| p.is_i32()) || f.params().len() != 2 { + bail!("eval_bytecode should accept 2 i32s as parameters"); + } + if f.results().len() != 0 { + bail!("eval_bytecode should return no results"); + } } let invoke = imports @@ -604,7 +609,7 @@ impl Runner { args.push(format!("text-encoding={}", if enabled { "y" } else { "n" })); } - if let Plugin::User = plugin { + if matches!(plugin, Plugin::User | Plugin::DefaultAsUser) { args.push("-C".to_string()); args.push(format!("plugin={}", plugin.path().to_str().unwrap())); } diff --git a/crates/test-macros/src/lib.rs b/crates/test-macros/src/lib.rs index 9fa8ff40..78d09468 100644 --- a/crates/test-macros/src/lib.rs +++ b/crates/test-macros/src/lib.rs @@ -299,7 +299,7 @@ fn expand_cli_tests(test_config: &CliTestConfig, func: syn::ItemFn) -> Result my_code.js -$ javy build -C dynamic -o my_code.wasm my_code.js $ javy emit-plugin -o plugin.wasm +$ javy build -C dynamic -C plugin=plugin.wasm -o my_code.wasm my_code.js $ wasmtime run --preload javy_quickjs_provider_v3=plugin.wasm my_code.wasm hello world! ``` diff --git a/docs/docs-using-nodejs.md b/docs/docs-using-nodejs.md index 485ceec6..f8ec83e0 100644 --- a/docs/docs-using-nodejs.md +++ b/docs/docs-using-nodejs.md @@ -11,15 +11,15 @@ This example shows how to use a dynamically linked Javy compiled Wasm module. We ### Steps -1. The first step is to compile the `embedded.js` with Javy using dynamic linking: +1. Emit the Javy plugin ```shell -javy build -C dynamic -o embedded.wasm embedded.js +javy emit-plugin -o plugin.wasm ``` -2. Next emit the Javy plugin +2. Compile the `embedded.js` with Javy using dynamic linking: ```shell -javy emit-plugin -o plugin.wasm +javy build -C dynamic -C plugin=plugin.wasm -o embedded.wasm embedded.js ``` -3. Then we can run `host.mjs` +3. Run `host.mjs` ```shell node --no-warnings=ExperimentalWarning host.mjs ```