Can we make the macro crate take no feature?

That way the max length check would stay in place (and I think they should, otherwise suddenly enabling a feature could mean your code doesn't compile anymore is you had exceeded the length). Also we would avoid having to recompile the macro crate. But I don't know if that has any real impact.

What I would do, is make create_trace_record as always inline, and make it a no-op if the feature is disabled in the hyperlight_guest_tracing crate

So these are the two options we've got:

• use a feature in the macro crate so that we could make the macro do nothing when the feature is not set
• make the create_trace_record a no-op based on a feature in hyperlight-guest-tracing

I am not sure which one is the best for us, but I tend to agree with you @jprendes, turning on a feature and your code not compiling is not a good overall experience.
@syntactically what do you think, do you have strong opinions on using the first option (current PR state).

@dblnz I don't feel super incredibly strongly either way, but I just feel that it is cleaner to avoid inserting code that will then need to be optimised out, even if we are fairly confident in the optimisations.

I also don't quite follow where there is a static check (that could break compilation) being omitted in the non-trace case? I would say that anything that we can check statically, we should unconditionally check in the macro code, basically doing everything the same up to actually putting the tokens in. I do see that there is a dynamic check in the macro-inserted code, but that doesn't affect compilation. I also don't fully understand why that check is dynamic---I think it would be preferable to make it happen at macro-run-time, since it seems to check the length of a literal, and then it would be easy to make it happen unconditionally, even when not actually expanding into code.

@dblnz I don't feel super incredibly strongly either way, but I just feel that it is cleaner to avoid inserting code that will then need to be optimised out, even if we are fairly confident in the optimisations.

I also don't quite follow where there is a static check (that could break compilation) being omitted in the non-trace case? I would say that anything that we can check statically, we should unconditionally check in the macro code, basically doing everything the same up to actually putting the tokens in. I do see that there is a dynamic check in the macro-inserted code, but that doesn't affect compilation. I also don't fully understand why that check is dynamic---I think it would be preferable to make it happen at macro-run-time, since it seems to check the length of a literal, and then it would be easy to make it happen unconditionally, even when not actually expanding into code.

The check cannot happen at macro-run-time, because the below static check uses the MAX_TRACE_MSG_LEN value which comes from hyperlight_guest_tracing crate.
I cannot set hyperlight_guest_tracing as a dependency in hyperlight_guest_tracing_macro because I would create a circular dependency.
Currently, we re-export the macros from hyperlight_guest_tracing_macro in hyperlight_guest_tracing.
To avoid the explained problem, I've added this static check so the length would be checked at the generated code compile time.

                    const _: () = assert!(
                        #_entry_msg.len() <= hyperlight_guest_tracing::MAX_TRACE_MSG_LEN,
                        "Trace message exceeds the maximum bytes length",
                    );

I agree that the length check should happen no matter if the tracing is enabled or not, I'll make the change to do that.

The compilation time remark that @jprendes mentioned was in reference to having the feature trace change the macros behavior. This involves recompiling the hyperlight_guest_tracing_macro, when we can have it generate the same code, and the change would come from the generated code that behaves differently due to a function call now being dummy.

// hyperlight-guest-tracing

#[cfg(feature = "trace")]
#[inline(always)]
pub fn create_trace_record(_msg: &str) {
// actual creation of record
}

#[cfg(not(feature = "trace"))]
#[inline(always)]
pub fn create_trace_record(_msg: &str) {}

// hyperlight-guest-tracing-macro

        let expanded = quote! {
            {
                const _: () = assert!(
                    #entry_msg.len() <= hyperlight_guest_tracing::MAX_TRACE_MSG_LEN,
                    "Trace message exceeds the maximum bytes length",
                );
                ::hyperlight_guest_tracing::create_trace_record(#entry_msg);
                let __trace_result = #statement;
                ::hyperlight_guest_tracing::create_trace_record(#exit_msg);
                __trace_result
            }

I do see that there is a dynamic check in the macro-inserted code, but that doesn't affect compilation.

It is a const evaluated check, so it happens at compilation time, it's a no-op at runtime.

The check cannot happen at macro-run-time, because the below static check uses the MAX_TRACE_MSG_LEN value which comes from hyperlight_guest_tracing crate. I cannot set hyperlight_guest_tracing as a dependency in hyperlight_guest_tracing_macro because I would create a circular dependency. Currently, we re-export the macros from hyperlight_guest_tracing_macro in hyperlight_guest_tracing. To avoid the explained problem, I've added this static check so the length would be checked at the generated code compile time.

Ah, got it.

It is a const evaluated check, so it happens at compilation time, it's a no-op at runtime.

Sorry, I missed the binding (was looking for a const {} block). Thanks for catching that.

I have a small preference for the macro not changing behaviour depending on the feature, the opposite of @syntactically, but either option will do the job, so... ship it! :-)

Relying too much on cross-module inlining still makes me a tad bit uneasy, but perhaps I have just been burned one time to many by C compilers/accidental dynamic linking/etc. Up to you @dblnz.