-
Notifications
You must be signed in to change notification settings - Fork 33
Remove host function definition region dependency #389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,9 @@ See the License for the specific language governing permissions and | |
| limitations under the License. | ||
| */ | ||
|
|
||
| use hyperlight_common::flatbuffer_wrappers::function_types::{ParameterType, ReturnType}; | ||
| use hyperlight_common::flatbuffer_wrappers::host_function_definition::HostFunctionDefinition; | ||
| use hyperlight_common::flatbuffer_wrappers::host_function_details::HostFunctionDetails; | ||
| use hyperlight_host::func::{HostFunction, ParameterTuple, Registerable, SupportedReturnType}; | ||
| use hyperlight_host::sandbox::config::SandboxConfiguration; | ||
| use hyperlight_host::{GuestBinary, Result, UninitializedSandbox, new_error}; | ||
|
|
@@ -31,6 +34,8 @@ use crate::build_info::BuildInfo; | |
| /// With that `WasmSandbox` you can load a Wasm module through the `load_module` method and get a `LoadedWasmSandbox` which can then execute functions defined in the Wasm module. | ||
| pub struct ProtoWasmSandbox { | ||
| pub(super) inner: Option<UninitializedSandbox>, | ||
| /// Tracks registered host function definitions for pushing to the guest at load time | ||
| host_function_definitions: Vec<HostFunctionDefinition>, | ||
| } | ||
|
|
||
| impl Registerable for ProtoWasmSandbox { | ||
|
|
@@ -39,6 +44,13 @@ impl Registerable for ProtoWasmSandbox { | |
| name: &str, | ||
| hf: impl Into<HostFunction<Output, Args>>, | ||
| ) -> Result<()> { | ||
| // Track the host function definition for pushing to guest at load time | ||
| self.host_function_definitions.push(HostFunctionDefinition { | ||
| function_name: name.to_string(), | ||
| parameter_types: Some(Args::TYPE.to_vec()), | ||
| return_type: Output::TYPE, | ||
| }); | ||
|
|
||
| self.inner | ||
| .as_mut() | ||
| .ok_or(new_error!("inner sandbox was none")) | ||
|
|
@@ -65,7 +77,18 @@ impl ProtoWasmSandbox { | |
| let inner = UninitializedSandbox::new(guest_binary, cfg)?; | ||
| metrics::gauge!(METRIC_ACTIVE_PROTO_WASM_SANDBOXES).increment(1); | ||
| metrics::counter!(METRIC_TOTAL_PROTO_WASM_SANDBOXES).increment(1); | ||
| Ok(Self { inner: Some(inner) }) | ||
|
|
||
| // HostPrint is always registered by UninitializedSandbox, so include it by default | ||
| let host_function_definitions = vec![HostFunctionDefinition { | ||
| function_name: "HostPrint".to_string(), | ||
| parameter_types: Some(vec![ParameterType::String]), | ||
| return_type: ReturnType::Int, | ||
| }]; | ||
|
|
||
| Ok(Self { | ||
| inner: Some(inner), | ||
| host_function_definitions, | ||
| }) | ||
| } | ||
|
|
||
| /// Load the Wasm runtime into the sandbox and return a `WasmSandbox` | ||
|
|
@@ -75,12 +98,22 @@ impl ProtoWasmSandbox { | |
| /// The returned `WasmSandbox` can be then be cached and used to load a different Wasm module. | ||
| /// | ||
| pub fn load_runtime(mut self) -> Result<WasmSandbox> { | ||
| // Serialize host function definitions to push to the guest during InitWasmRuntime | ||
| let host_function_definitions = HostFunctionDetails { | ||
| host_functions: Some(std::mem::take(&mut self.host_function_definitions)), | ||
| }; | ||
|
|
||
| let host_function_definitions_bytes: Vec<u8> = (&host_function_definitions) | ||
| .try_into() | ||
| .map_err(|e| new_error!("Failed to serialize host function details: {:?}", e))?; | ||
|
|
||
| let mut sandbox = match self.inner.take() { | ||
| Some(s) => s.evolve()?, | ||
| None => return Err(new_error!("No inner sandbox found.")), | ||
| }; | ||
|
|
||
| let res: i32 = sandbox.call("InitWasmRuntime", ())?; | ||
| // Pass host function definitions to the guest as a parameter | ||
| let res: i32 = sandbox.call("InitWasmRuntime", (host_function_definitions_bytes,))?; | ||
| if res != 0 { | ||
| return Err(new_error!( | ||
| "InitWasmRuntime Failed with error code {:?}", | ||
|
|
@@ -99,6 +132,13 @@ impl ProtoWasmSandbox { | |
| name: impl AsRef<str>, | ||
| host_func: impl Into<HostFunction<Output, Args>>, | ||
| ) -> Result<()> { | ||
| // Track the host function definition for pushing to guest at load time | ||
| self.host_function_definitions.push(HostFunctionDefinition { | ||
| function_name: name.as_ref().to_string(), | ||
| parameter_types: Some(Args::TYPE.to_vec()), | ||
| return_type: Output::TYPE, | ||
| }); | ||
|
Comment on lines
+135
to
+140
|
||
|
|
||
| self.inner | ||
| .as_mut() | ||
| .ok_or(new_error!("inner sandbox was none"))? | ||
|
|
@@ -111,6 +151,9 @@ impl ProtoWasmSandbox { | |
| &mut self, | ||
| print_func: impl Into<HostFunction<i32, (String,)>>, | ||
| ) -> Result<()> { | ||
| // HostPrint definition is already tracked from new() since | ||
| // UninitializedSandbox always registers a default HostPrint. | ||
| // This method only replaces the implementation, not the definition. | ||
| self.inner | ||
| .as_mut() | ||
| .ok_or(new_error!("inner sandbox was none"))? | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,5 +14,7 @@ opt-level = 'z' | |
| strip = true | ||
|
|
||
|
|
||
| [workspace] # indicate that this crate is not part of any workspace | ||
|
|
||
| [dependencies] | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -96,7 +96,7 @@ pub fn guest_dispatch_function(function_call: FunctionCall) -> Result<Vec<u8>> { | |||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| #[instrument(skip_all, level = "Info")] | ||||||||||||||||||||||||||||||||||||||
| fn init_wasm_runtime() -> Result<Vec<u8>> { | ||||||||||||||||||||||||||||||||||||||
| fn init_wasm_runtime(function_call: &FunctionCall) -> Result<Vec<u8>> { | ||||||||||||||||||||||||||||||||||||||
| let mut config = Config::new(); | ||||||||||||||||||||||||||||||||||||||
| config.with_custom_code_memory(Some(alloc::sync::Arc::new(platform::WasmtimeCodeMemory {}))); | ||||||||||||||||||||||||||||||||||||||
| #[cfg(gdb)] | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -112,9 +112,32 @@ fn init_wasm_runtime() -> Result<Vec<u8>> { | |||||||||||||||||||||||||||||||||||||
| let mut linker = Linker::new(&engine); | ||||||||||||||||||||||||||||||||||||||
| wasip1::register_handlers(&mut linker)?; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| let hostfuncs = hostfuncs::get_host_function_details() | ||||||||||||||||||||||||||||||||||||||
| .host_functions | ||||||||||||||||||||||||||||||||||||||
| .unwrap_or_default(); | ||||||||||||||||||||||||||||||||||||||
| // Parse host function details pushed by the host as a parameter | ||||||||||||||||||||||||||||||||||||||
| let params = function_call.parameters.as_ref().ok_or_else(|| { | ||||||||||||||||||||||||||||||||||||||
| HyperlightGuestError::new( | ||||||||||||||||||||||||||||||||||||||
| ErrorCode::GuestFunctionParameterTypeMismatch, | ||||||||||||||||||||||||||||||||||||||
| "InitWasmRuntime: missing parameters".to_string(), | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
| })?; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| let bytes = match params.first() { | ||||||||||||||||||||||||||||||||||||||
| Some(ParameterValue::VecBytes(ref b)) => b, | ||||||||||||||||||||||||||||||||||||||
| _ => { | ||||||||||||||||||||||||||||||||||||||
| return Err(HyperlightGuestError::new( | ||||||||||||||||||||||||||||||||||||||
| ErrorCode::GuestFunctionParameterTypeMismatch, | ||||||||||||||||||||||||||||||||||||||
| "InitWasmRuntime: first parameter must be VecBytes".to_string(), | ||||||||||||||||||||||||||||||||||||||
| )) | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+125
to
+130
|
||||||||||||||||||||||||||||||||||||||
| _ => { | |
| return Err(HyperlightGuestError::new( | |
| ErrorCode::GuestFunctionParameterTypeMismatch, | |
| "InitWasmRuntime: first parameter must be VecBytes".to_string(), | |
| )) | |
| } | |
| Some(_) => { | |
| return Err(HyperlightGuestError::new( | |
| ErrorCode::GuestFunctionParameterTypeMismatch, | |
| "InitWasmRuntime: first parameter must be VecBytes".to_string(), | |
| )) | |
| } | |
| None => { | |
| return Err(HyperlightGuestError::new( | |
| ErrorCode::GuestFunctionParameterTypeMismatch, | |
| "InitWasmRuntime: expected 1 parameter".to_string(), | |
| )) | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly related to your PR, but can we use the #[guest_func] macro on init_wasm_runtime instead of manually calling register_function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe teh same applies to all usages of register_function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
host_function_definitionsis built by blindly pushing entries, but you also pre-seed it with aHostPrintdefinition innew(). If a caller later registers a host function namedHostPrintviaregister(...)/register_host_function(...), the list will contain duplicates and the guestinit_wasm_runtimewill attempt tolinker.func_newthe same name twice, which should error and prevent the runtime from loading. Consider enforcing uniqueness by name (e.g., replace existing definition instead of pushing, or special-caseHostPrint).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did think about this, but wasn't sure what the best thing to do was. Anyone have any thoughts? This feels like it could lead to some strange bugs as well