Add comments on tricky areas

Signed-off-by: Jorge Prendes <[email protected]>

Author: jprendes

src/hyperlight_guest_bin/src/guest_function/definition.rs

@@ -94,6 +94,31 @@ macro_rules! impl_host_function {
             ($($P,)*): ParameterTuple,
             R: ResultType<HyperlightGuestError>,
         {
+            // Only functions that can be coerced into a function pointer (i.e., "fn" types)
+            // can be registered as guest functions.
+            //
+            // Note that the "Fn" trait is different from "fn" types in Rust.
+            // "fn" is a type, while "Fn" is a trait.
+            // For example, closures that capture environment implement "Fn" but cannot be
+            // coerced to function pointers.
+            // This means that the closure returned by `into_guest_function` can not capture
+            // any environment, not event `self`, and we must only rely on the type system
+            // to call the correct function.
+            //
+            // Ideally we would implement IntoGuestFunction for any F that can be converted
+            // into a function pointer, but currently there's no way to express that in Rust's
+            // type system.
+            // Therefore, to ensure that F is a "fn" type, we enforce that F is zero-sized
+            // has no Drop impl (the latter is enforced by the Copy bound), and it doesn't
+            // capture any lifetimes (not even through a marker type like PhantomData).
+            //
+            // Note that implementing IntoGuestFunction for "fn($(P),*) -> R" is not an option
+            // either, "fn($(P),*) -> R" is a type that's shared for all function pointers with
+            // that signature, e.g., "fn add(a: i32, b: i32) -> i32 { a + b }" and
+            // "fn sub(a: i32, b: i32) -> i32 { a - b }" both can be coerced to the same
+            // "fn(i32, i32) -> i32" type, so we would need to capture self (a function pointer)
+            // to know exactly which function to call.
+
             #[doc(hidden)]
             const ASSERT_ZERO_SIZED: () = const {
                 assert!(core::mem::size_of::<Self>() == 0)

src/hyperlight_guest_bin/src/lib.rs

@@ -276,6 +276,8 @@ pub mod __private {
             impl FromResult for $ty {
                 type Output = Self;
                 fn from_result(res: Result<Self::Output, HyperlightGuestError>) -> Self {
+                    // Unwrapping here is fine as this would only run in a guest
+                    // and not in the host.
                     res.unwrap()
                 }
             }

src/hyperlight_guest_macro/src/lib.rs

@@ -21,13 +21,16 @@ use syn::parse::{Error, Parse, ParseStream, Result};
 use syn::spanned::Spanned as _;
 use syn::{ForeignItemFn, ItemFn, LitStr, Pat, parse_macro_input};
 
+/// Represents the optional name argument for the guest_function and host_function macros.
 enum NameArg {
     None,
     Name(LitStr),
 }
 
 impl Parse for NameArg {
     fn parse(input: ParseStream) -> Result<Self> {
+        // accepts either nothing or a single string literal
+        // anything else is an error
         if input.is_empty() {
             return Ok(NameArg::None);
         }
@@ -39,8 +42,52 @@ impl Parse for NameArg {
     }
 }
 
+/// Attribute macro to mark a function as a guest function.
+/// This will register the function so that it can be called by the host.
+///
+/// If a name is provided as an argument, that name will be used to register the function.
+/// Otherwise, the function's identifier will be used.
+///
+/// The function arguments must be supported parameter types, and the return type must be
+/// a supported return type or a `Result<T, HyperlightGuestError>` with T being a supported
+/// return type.
+///
+/// # Note
+/// The function will be registered with the host at program initialization regardless of
+/// the visibility modifier used (e.g., `pub`, `pub(crate)`, etc.).
+/// This means that a private functions can be called by the host from beyond its normal
+/// visibility scope.
+///
+/// # Example
+/// ```ignore
+/// use hyperlight_guest_bin::guest_function;
+/// #[guest_function]
+/// fn my_guest_function(arg1: i32, arg2: String) -> i32 {
+///     arg1 + arg2.len() as i32
+/// }
+/// ```
+///
+/// or with a custom name:
+/// ```ignore
+/// use hyperlight_guest_bin::guest_function;
+/// #[guest_function("custom_name")]
+/// fn my_guest_function(arg1: i32, arg2: String) -> i32 {
+///     arg1 + arg2.len() as i32
+/// }
+/// ```
+///
+/// or with a Result return type:
+/// ```ignore
+/// use hyperlight_guest_bin::guest_function;
+/// use hyperlight_guest::bail;
+/// #[guest_function]
+/// fn my_guest_function(arg1: i32, arg2: String) -> Result<i32, HyperlightGuestError> {
+///     bail!("An error occurred");
+/// }
+/// ```
 #[proc_macro_attribute]
 pub fn guest_function(attr: TokenStream, item: TokenStream) -> TokenStream {
+    // Obtain the crate name for hyperlight-guest-bin
     let crate_name =
         crate_name("hyperlight-guest-bin").expect("hyperlight-guest-bin must be a dependency");
     let crate_name = match crate_name {
@@ -51,15 +98,26 @@ pub fn guest_function(attr: TokenStream, item: TokenStream) -> TokenStream {
         }
     };
 
+    // Parse the function definition that we will be working with, and
+    // early return if parsing as `ItemFn` fails.
     let fn_declaration = parse_macro_input!(item as ItemFn);
 
+    // Obtain the name of the function being decorated.
     let ident = fn_declaration.sig.ident.clone();
 
+    // Determine the name used to register the function, either
+    // the provided name or the function's identifier.
     let exported_name = match parse_macro_input!(attr as NameArg) {
         NameArg::None => quote! { stringify!(#ident) },
         NameArg::Name(name) => quote! { #name },
     };
 
+    // Small sanity checks to improve error messages.
+    // These checks are not strictly necessary, as the generated code
+    // would fail to compile anyway (due to the trait bounds of `register_fn`),
+    // but they provide better feedback to the user of the macro.
+
+    // Check that there are no receiver arguments (i.e., `self`, `&self`, `Box<Self>`, etc).
     if let Some(syn::FnArg::Receiver(arg)) = fn_declaration.sig.inputs.first() {
         return Error::new(
             arg.span(),
@@ -69,6 +127,7 @@ pub fn guest_function(attr: TokenStream, item: TokenStream) -> TokenStream {
         .into();
     }
 
+    // Check that the function is not async.
     if fn_declaration.sig.asyncness.is_some() {
         return Error::new(
             fn_declaration.sig.asyncness.span(),
@@ -78,10 +137,14 @@ pub fn guest_function(attr: TokenStream, item: TokenStream) -> TokenStream {
         .into();
     }
 
+    // The generated code will replace the decorated code, so we need to
+    // include the original function declaration in the output.
     let output = quote! {
         #fn_declaration
 
         const _: () = {
+            // Add the function registration in the GUEST_FUNCTION_INIT distributed slice
+            // so that it can be registered at program initialization
             #[#crate_name::__private::linkme::distributed_slice(#crate_name::__private::GUEST_FUNCTION_INIT)]
             #[linkme(crate = #crate_name::__private::linkme)]
             static REGISTRATION: fn() = || {
@@ -93,8 +156,44 @@ pub fn guest_function(attr: TokenStream, item: TokenStream) -> TokenStream {
     output.into()
 }
 
+/// Attribute macro to mark a function as a host function.
+/// This will generate a function that calls the host function with the same name.
+///
+/// If a name is provided as an argument, that name will be used to call the host function.
+/// Otherwise, the function's identifier will be used.
+///
+/// The function arguments must be supported parameter types, and the return type must be
+/// a supported return type or a `Result<T, HyperlightGuestError>` with T being a supported
+/// return type.
+///
+/// # Panic
+/// If the return type is not a Result, the generated function will panic if the host function
+/// returns an error.
+///
+/// # Example
+/// ```ignore
+/// use hyperlight_guest_bin::host_function;
+/// #[host_function]
+/// fn my_host_function(arg1: i32, arg2: String) -> i32;
+/// ```
+///
+/// or with a custom name:
+/// ```ignore
+/// use hyperlight_guest_bin::host_function;
+/// #[host_function("custom_name")]
+/// fn my_host_function(arg1: i32, arg2: String) -> i32;
+/// ```
+///
+/// or with a Result return type:
+/// ```ignore
+/// use hyperlight_guest_bin::host_function;
+/// use hyperlight_guest::error::HyperlightGuestError;
+/// #[host_function]
+/// fn my_host_function(arg1: i32, arg2: String) -> Result<i32, HyperlightGuestError>;
+/// ```
 #[proc_macro_attribute]
 pub fn host_function(attr: TokenStream, item: TokenStream) -> TokenStream {
+    // Obtain the crate name for hyperlight-guest-bin
     let crate_name =
         crate_name("hyperlight-guest-bin").expect("hyperlight-guest-bin must be a dependency");
     let crate_name = match crate_name {
@@ -105,25 +204,42 @@ pub fn host_function(attr: TokenStream, item: TokenStream) -> TokenStream {
         }
     };
 
+    // Parse the function declaration that we will be working with, and
+    // early return if parsing as `ForeignItemFn` fails.
+    // A function declaration without a body is a foreign item function, as that's what
+    // you would use when declaring an FFI function.
     let fn_declaration = parse_macro_input!(item as ForeignItemFn);
 
+    // Destructure the foreign item function to get its components.
     let ForeignItemFn {
         attrs,
         vis,
         sig,
         semi_token: _,
     } = fn_declaration;
 
+    // Obtain the name of the function being decorated.
     let ident = sig.ident.clone();
 
+    // Determine the name used to call the host function, either
+    // the provided name or the function's identifier.
     let exported_name = match parse_macro_input!(attr as NameArg) {
         NameArg::None => quote! { stringify!(#ident) },
         NameArg::Name(name) => quote! { #name },
     };
 
+    // Build the list of argument identifiers to pass to the call_host function.
+    // While doing that, also do some sanity checks to improve error messages.
+    // These checks are not strictly necessary, as the generated code would fail
+    // to compile anyway due to either:
+    // * the trait bounds of `call_host`
+    // * the generated code having invalid syntax
+    // but they provide better feedback to the user of the macro, especially in
+    // the case of invalid syntax.
     let mut args = vec![];
     for arg in sig.inputs.iter() {
         match arg {
+            // Reject receiver arguments (i.e., `self`, `&self`, `Box<Self>`, etc).
             syn::FnArg::Receiver(_) => {
                 return Error::new(
                     arg.span(),
@@ -133,6 +249,12 @@ pub fn host_function(attr: TokenStream, item: TokenStream) -> TokenStream {
                 .into();
             }
             syn::FnArg::Typed(arg) => {
+                // A typed argument: `name: Type`
+                // Technically, the `name` part can be any pattern, e.g., destructuring patterns
+                // like `(a, b): (i32, u64)`, but we only allow simple identifiers here
+                // to keep things simple.
+
+                // Reject anything that is not a simple identifier.
                 let Pat::Ident(pat) = *arg.pat.clone() else {
                     return Error::new(
                         arg.span(),
@@ -142,6 +264,7 @@ pub fn host_function(attr: TokenStream, item: TokenStream) -> TokenStream {
                     .into();
                 };
 
+                // Reject any argument with attributes, e.g., `#[cfg(feature = "gdb")] name: Type`
                 if !pat.attrs.is_empty() {
                     return Error::new(
                         arg.span(),
@@ -151,6 +274,7 @@ pub fn host_function(attr: TokenStream, item: TokenStream) -> TokenStream {
                     .into();
                 }
 
+                // Reject any argument passed by reference
                 if pat.by_ref.is_some() {
                     return Error::new(
                         arg.span(),
@@ -160,6 +284,7 @@ pub fn host_function(attr: TokenStream, item: TokenStream) -> TokenStream {
                     .into();
                 }
 
+                // Reject any mutable argument, e.g., `mut name: Type`
                 if pat.mutability.is_some() {
                     return Error::new(
                         arg.span(),
@@ -169,6 +294,7 @@ pub fn host_function(attr: TokenStream, item: TokenStream) -> TokenStream {
                     .into();
                 }
 
+                // Reject any sub-patterns
                 if pat.subpat.is_some() {
                     return Error::new(
                         arg.span(),
@@ -180,18 +306,23 @@ pub fn host_function(attr: TokenStream, item: TokenStream) -> TokenStream {
 
                 let ident = pat.ident.clone();
 
+                // All checks passed, add the identifier to the argument list.
                 args.push(quote! { #ident });
             }
         }
     }
 
+    // Determine the return type of the function.
+    // If the return type is not specified, it is `()`.
     let ret: proc_macro2::TokenStream = match &sig.output {
         syn::ReturnType::Default => quote! { quote! { () } },
         syn::ReturnType::Type(_, ty) => {
             quote! { #ty }
         }
     };
 
+    // Take the parts of the function declaration and generate a function definition
+    // matching the provided declaration, but with a body that calls the host function.
     let output = quote! {
         #(#attrs)* #vis #sig {
             use #crate_name::__private::FromResult;

src/hyperlight_host/src/func/host_functions.rs

@@ -64,26 +64,26 @@ where
     Args: ParameterTuple,
     Output: SupportedReturnType,
 {
-    // This is a thin wrapper around a `Fn(Args) -> Result<Output>`.
-    // But unlike `Fn` which is a trait, this is a concrete type.
+    // This is a thin wrapper around a `Function<Output, Args, HyperlightError>`.
+    // But unlike `Function<..>` which is a trait, this is a concrete type.
     // This allows us to:
     //  1. Impose constraints on the function arguments and return type.
     //  2. Impose a single function signature.
     //
-    // This second point is important because the `Fn` trait is generic
-    // over the function arguments (with an associated return type).
-    // This means that a given type could implement `Fn` for multiple
+    // This second point is important because the `Function<..>` trait is generic
+    // over the function arguments and return type.
+    // This means that a given type could implement `Function<..>` for multiple
     // function signatures.
     // This means we can't do something like:
     // ```rust,ignore
     // impl<Args, Output, F> SomeTrait for F
     // where
-    //     F: Fn(Args) -> Result<Output>,
+    //     F: Function<Output, Args, HyperlightError>,
     // { ... }
     // ```
-    // because the concrete type F might implement `Fn` for multiple times,
-    // and that would means implementing `SomeTrait` multiple times for the
-    // same type.
+    // because the concrete type F might implement `Function<..>` for multiple
+    // arguments and return types, and that would means implementing `SomeTrait`
+    // multiple times for the same type F.
 
     // Use Arc in here instead of Box because it's useful in tests and
     // presumably in other places to be able to clone a HostFunction and

src/hyperlight_host/src/sandbox/initialized_multi_use.rs

@@ -450,6 +450,8 @@ impl MultiUseSandbox {
                 Output::TYPE,
                 args.into_value(),
             );
+            // Use the ? operator to allow converting any hyperlight_common::func::Error
+            // returned by from_value into a HyperlightError
             let ret = Output::from_value(ret?)?;
             Ok(ret)
         })