Update hyperlight-guest-tracing crate to conditionally enable traces at compile time

[dblnz]

Description

This PR addresses two things

• Add check for trace! macro empty message
• Change hyperlight-guest-tracing-macro to not generate code gated by #[cfg(feature = "trace_guest")] because that causes errors like below if the user does not define a feature with the exact name trace_guest.
• Change test examples to use the hyperlight-guest-tracing crate to generate trace records in the guest (outside of hyperlight-guest and hyperlight-guest-bin crates)

Guests compile example

# Cargo.toml

 hyperlight-guest-tracing = { path = "../../../hyperlight_guest_tracing" }

[features]
default = []
trace= ["hyperlight-guest-bin/trace_guest"]

➜  simpleguest git:(main) ✗ cargo build --features trace
warning: unexpected `cfg` condition value: `trace_guest`
   --> src/main.rs:102:1
    |
102 | #[hyperlight_guest_tracing::trace_function]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: expected values for `feature` are: `default`, `mem_profile`, `trace`, and `unwind_guest`
    = note: using a cfg inside a attribute macro will use the cfgs from the destination crate and not the ones from the defining crate
    = help: try referring to `hyperlight_guest_tracing::trace_function` crate for guidance on how handle this unexpected cfg
    = help: the attribute macro `hyperlight_guest_tracing::trace_function` may come from an old version of the `hyperlight_guest_tracing_macro` crate, try updating your dependency with `cargo update -p hyperlight_guest_tracing_macro`
    = note: see <https://doc.rust-lang.org/nightly/rustc/check-cfg/cargo-specifics.html> for more information about checking conditional configuration
    = note: `#[warn(unexpected_cfgs)]` on by default
    = note: this warning originates in the attribute macro `hyperlight_guest_tracing::trace_function` (in Nightly builds, run with -Z macro-backtrace for more info)

warning: `simpleguest` (bin "simpleguest") generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.02s
➜  simpleguest git:(main) ✗    

Solution

Define a trace feature for the hyperlight-guest-tracing crate and based on whether it is set or not enable either the tracing implementation or a dummy/empty implementation.

This makes removing the #[cfg(feature = "trace_guest")] from hyperlight-guest-tracing-macro` unnecessary.

[jprendes]

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

[dblnz]

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).

[syntactically]

@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]

@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
            }

[jprendes]

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.

[jprendes]

LGTM.

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! :-)

[syntactically]

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.