Save an unconditional clone of guest parameters for registered guest functions

[ludfjig]

vs main:

Only affects registered guest functions (not c api, nor when someone implements their own guest_dispatch_function).

GuestFunctionDefinition was made generic to be able to contain both GuestFunc and CGuestFunc (for use with c-api), also removes some unsafe code and adds some type-safety

[dblnz]

This is fine from my point of view. I've got burnt by this slight difference between the guest_dispatch_function definition and the other function definitions.

[syntactically]

The perf improvement looks great, and I don't think that the amount of breaking that you mention there is necessarily a barrier. If we wanted to be very proper I suppose we would introduce a new GuestFunctionDefinitionV2 structure and mark the original deprecated so that a misuse threw an error---but I don't know if it is worth it?

Two slight comments:

• Is there a reason why we have the function pointer on the GuestFunctionDefinition be a usize, rather than an appropriate fn() type, which might help catch any other misuses of this?
• The other direction that we could go here is perhaps to see if it is possible to get rid of the requirement of having an owned FunctionCall struct in order to build the parameter tuple to hand off to the registered guest function. I would be curious if you have any thoughts on doing that. Having this be owned the way it is seems a little questionable, because it means that the copy that is currently happening in try_pop_shared_input_data_into is impossible to soundly eliminate. Of course, the lifetime constraints get a little bit more hairy with the changes to buffer lifetime and support for async code in Add HIP 0001 ring buffer proposal for Hyperlight I/O #1112, but it's still not totally obvious to me that making this owned would not preclude some nice ability to make direct references into the I/O buffers during the lifetime of the call. @andreiltd would you care to chime in on this at all?

[syntactically]

If we wanted to be very proper I suppose we would introduce a new GuestFunctionDefinitionV2 structure and mark the original deprecated so that a misuse threw an error---but I don't know if it is worth it?

I know @jsturtevant has strong opinions on the permissibility of breaking changes that can cause silent failure in downstreams, so paging him on this as well.

[ludfjig]

Maybe we can rename guest_dispatch_function randomly between every version, and provide a macro that defines it, which would force users to use our macro :D We can then make changes in the future without breaking people (hopefully).

Regarding the owned FunctionCall indeed it does make things less flexible in the future, especially if we'd want to borrow directly from raw buffer with appropriate lifetimes. Maybe we can provide a method that users should call in order to get the parameters, to allow even more flexibility

[jprendes]

This LGTM.
I agree with Lucy's concerns about borrowing data from the buffer directly.
But borrowing from the buffer would imply a complete redesign of the parameters / return value, and will certainly introduce a breaking chage regardless of what we do here.
For example, a guest function that would take a String would not be able to borrow from the buffer, it would have to take a &str, same with Vec<u8>. And those are very common patterns that we have encouraged people to use.
Until then this gives us a significant performance win, so it gets a thumbs up from me :-)

[jprendes]

Maybe we can rename guest_dispatch_function randomly between every version, and provide a macro that defines it, which would force users to use our macro :D We can then make changes in the future without breaking people (hopefully).

Regarding the owned FunctionCall indeed it does make things less flexible in the future, especially if we'd want to borrow directly from raw buffer with appropriate lifetimes. Maybe we can provide a method that users should call in order to get the parameters, to allow even more flexibility

Or we can make the type private, and make people use the guest_function macro :-D (joking not joking)

[jsturtevant]

If we wanted to be very proper I suppose we would introduce a new GuestFunctionDefinitionV2 structure and mark the original deprecated so that a misuse threw an error---but I don't know if it is worth it?

I know @jsturtevant has strong opinions on the permissibility of breaking changes that can cause silent failure in downstreams, so paging him on this as well.

This seems like a pretty small and reasonable thing to do to avoid any downstream users having any issues with this. I believe building a habit of recognizing and coming up with patterns to avoid unexpected downstream issues is a good thing.

[ludfjig]

Updated @syntactically @jprendes @dblnz @jsturtevant

[jsturtevant]

LGTM

[Copilot]

Pull request overview

This pull request eliminates unsafe mem::transmute operations by introducing type-safe function pointer handling for guest function registration. The change replaces raw usize function pointers with typed function pointers (GuestFunc for Rust, CGuestFunc for C API), improving memory safety and type correctness. Additionally, the function signature changes from accepting a borrowed &FunctionCall to an owned FunctionCall, avoiding unnecessary clones during guest function dispatch.

Changes:

• Introduce GuestFunc type alias and make GuestFunctionDefinition generic over function pointer types
• Remove unsafe mem::transmute calls when invoking guest functions, replacing with direct typed function pointer invocation
• Change guest function signature from fn(&FunctionCall) to fn(FunctionCall) for owned parameter passing

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/hyperlight_guest_bin/src/guest_function/definition.rs	Introduces GuestFunc type alias and makes GuestFunctionDefinition generic over function pointer type F, updates function signatures to accept owned FunctionCall
src/hyperlight_guest_bin/src/guest_function/register.rs	Makes GuestFunctionRegister generic over function pointer type, reorganizes methods for generic vs. GuestFunc-specific implementations
src/hyperlight_guest_bin/src/guest_function/call.rs	Removes unsafe mem::transmute and directly invokes typed function pointers
src/hyperlight_guest_bin/src/host_comm.rs	Updates print_output_with_host_print to accept owned FunctionCall and use remove(0) instead of cloning
src/hyperlight_guest_bin/src/lib.rs	Exports GuestFunc type publicly and updates static REGISTERED_GUEST_FUNCTIONS type annotation
src/hyperlight_guest_capi/src/dispatch.rs	Updates C API to use typed CGuestFunc function pointers, removes unsafe transmute, maintains &FfiFunctionCall signature for C compatibility
src/tests/rust_guests/simpleguest/src/main.rs	Updates test code to use new typed API with GuestFunctionDefinition::<GuestFunc>::new and pass function pointer directly
src/hyperlight_component_util/src/guest.rs	Updates component generation code to accept owned FunctionCall and pass function pointers without casting to usize
CHANGELOG.md	Documents the breaking change in API for GuestFunctionDefinition::new

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.