mkeeter commented 1 year ago

Thanks for merging in #137!

While trying to integrate it, I realized that it wasn't quite fine-grained enough:

We care about the "print as markdown" functionality, but not about the raw or JSON printing
I'm building on an AArch64 machine, not x86
The display feature includes fn raw(...) and fn json(...), which both require an x86 CPU on the machine building the crate (the former because it calls cpuid!() directly; the latter because of how serialization is implemented)

This PR splits display into three features: display-raw, display-json and display-markdown, whose behavior is exactly what you'd expect 😄

This allows me to build with just display-markdown, which compiles fine on an AArch64 machine.

Codewise, there are no logical changes, but I ended up moving the markdown helper functions into fn markdown itself, to avoid having a ton of conditional compilation directives.

codecov[bot] commented 1 year ago

Codecov Report

Merging #138 (a089828) into master (a837dba) will decrease coverage by 0.68%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
- Coverage   39.34%   38.66%   -0.68%     
==========================================
  Files           4        4              
  Lines        4072     4143      +71     
==========================================
  Hits         1602     1602              
- Misses       2470     2541      +71

Impacted Files	Coverage Δ
src/display.rs	`0.00% <0.00%> (ø)`
src/lib.rs	`51.26% <ø> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

gz commented 1 year ago

hey, sounds good. Looks like I will need to do a closer look into this; because maybe this is an opportunity to finally fix https://github.com/gz/rust-cpuid/issues/87 in short: I think the serialization code calling cpuid is a bug and probably shouldn't happen

AFAICT if the underlying issue of the serialization stuff calling cpuid is gone, we wouldn't need 3 features

mkeeter commented 1 year ago

Agreed! To give a little more context, there were two different limitations that led to this approach:

`display::raw(...)`

The implementation of display::raw uses the cpuid! macro directly, which doesn't exist on AArch64.

This is actually an easy fix: if we make it take a reader, then we wouldn't need to use the macro directly

fn raw(reader: CpuIdReader) {
    // ...
}

`display::json(...)`

This one is harder, and touches on what you talked about in #137.

The CpuIdReader is included in many data structures which we want to serialize. It is annotated with serde(ignore), which means that CpuIdReader must implement Default, which it can't do on an AArch64 system.

I was deliberately not trying to fix this in the PR, since it's tricky! We could implement a dummy reader on AArch64 (which just panics), but I don't love that option either...

One option that would minimize proliferation of features would be

Switch back to a single display feature, with the fix to display::raw from above
Only build the display::json(...) if both display and serialize features are enabled

That would make everything except display::json work when building on AArch64, and wouldn't add any new features.

gz commented 1 year ago

Thanks for the detailed write up.

I feel like the serialize use-case would best be fixed by:

Remove the serialize/deserialize annotation from all data-structures, except CpuIdResult
Maybe add a type that contains a Vec<Vec<CpuIdResult>> that can be shipped over the network/to another CPU
Be able to instantiate CpuId with a custom reader that reads data from the Vec<Vec<CpuIdResult>> instead of calling cpuid

Once the serialize/deserialize is gone from all data-structures that might help with aarch64 as we no longer need Default for CpuIdReader. We could probably just make display::json(...) take a CpuIdReader similar to what you proposed with display::raw().

Maybe I need to make CpuIdReader take something more dynamic (such as a closure) instead of just a fn(u32, u32) -> CpuIdResult so it can access a dynamic Vec<Vec<CpuIdResult>> from the environment.

Let me know if you think this is sensible.

mkeeter commented 1 year ago

I think making the CpuIdReader more generic is a great idea. I'd suggest switching it to a trait + blanket implementation for Fn(u32, u32) -> CpuIdResult, e.g.

/// Implements function to read/write cpuid.
/// This allows to conveniently swap out the underlying cpuid implementation
/// with one that returns data that is deterministic (for unit-testing).
pub trait CpuIdReader: Copy {
    fn cpuid1(&self, eax: u32) -> CpuIdResult {
        self.cpuid2(eax, 0)
    }
    fn cpuid2(&self, eax: u32, ecx: u32) -> CpuIdResult;
}

impl<F> CpuIdReader for F
where
    F: Fn(u32, u32) -> CpuIdResult + Copy,
{
    fn cpuid2(&self, eax: u32, ecx: u32) -> CpuIdResult {
        self(eax, ecx)
    }
}

This would also allow an "offline" reader that pulls from a Vec<Vec<CpuIdResult>>, although I didn't implement that yet.

Serialization is a bit trickier – display::json ends up wanting to serialize a lot of things that have to contain a reader in their bodies, e.g. SoCVendorInfo. I'm not sure of the use-case for display::json, so it's harder to make concrete suggestions.

One option would be to declare the canonical "ship it over the network" form to be Vec<Vec<CpuIdResult>>. Then we could implement only Serialize (and not Deserialize) for the structs that are both (1) printed in display::json and (2) have to contain a CpuIdReader; with a #[serde(skip)] annotation, this would work fine.

(If we did this, I'm not sure if we'd also want to remove Deserialize from everything else, e.g. all of the structs in lib.rs and extended.rs)

I've implemented these ideas in a new branch, which looks pretty clean.

gz commented 1 year ago

This looks great, I like it. Also seems like this change should basically require no changes from the users which is great -- though it might still need a major release. I'm not sure about it need to check the rules again. Anyways, this crate is already at v10 so I guess it's not a big deal ;)

I added this commit https://github.com/gz/rust-cpuid/commit/a9af43c242100c1b7438dd2e3d4e6ebd14126634 which makes sure tests run again and new() and default() are back.

I'm not sure of the use-case for display::json, so it's harder to make concrete suggestions.

I think I added it mostly for testing so I wouldn't forget to make new things serializable. I'm happy to get rid of it completely. I think having Serialize/Deserialize on anything that has a reader in the struct is a bad idea from the start. Hence my suggestion is to only Serialize/Deserialize the CpuIdResult and remove Serialize/Deserialize on everything else.

mkeeter commented 1 year ago

Okay, that all makes sense to me!

I'm going to close this PR, since you're already staging new work on top of my branch. Feel free to tag me for a final review once you're ready to merge, and I can check against Humility and on AArch64.

Thanks for the help on this!

gz commented 1 year ago

Pushed some changes to the branch that we discussed (remove serialization and json), please have a look this might now be in a usable state for you.

One remaining thing I noticed, the library code now contains the string_to_static_str function https://github.com/gz/rust-cpuid/blob/cpuid-trait-and-serialize/src/display.rs#L30 which leaks memory. In the CLI app this was fine as it's very short running and just prints and exits, but since this code is now in the library, we need to find a better way for you and others if you want to use this in a potentially long running process.

mkeeter commented 1 year ago

It looks like we can remove the string_to_static_str with a little refactoring; I pushed to our fork at 390bcf7860102a56c92baa804b66dc5045de58bd.

One remaining issue with your branch:

/// The main type used to query information about the CPU we're running on.
///
/// Other structs can be accessed by going through this type.
#[derive(Clone, Copy)]
pub struct CpuId<R: CpuIdReader = CpuIdReaderNative> {

Specifying this default means that it doesn't build on AArch64, which lacks CpuIdReaderNative.

It's not pretty, but it turns out we could conditionally add this default parameter:

pub struct CpuId<
    #[cfg(any(
        all(target_arch = "x86", not(target_env = "sgx"), target_feature = "sse"),
        all(target_arch = "x86_64", not(target_env = "sgx"))
    ))]
    R: CpuIdReader=CpuIdReaderNative,

    #[cfg(not(any(
        all(target_arch = "x86", not(target_env = "sgx"), target_feature = "sse"),
        all(target_arch = "x86_64", not(target_env = "sgx"))
    )))]
    R: CpuIdReader
> {
    // ...

I've done this in commit 65a11445feaa1f63ae87b5326ecee213fbc142b6

Finally, I realized that CpuIdReader: Copy was too tight a bound; this would prevent us from using any non-trivial closure. I've switched over to Clone, which should be zero-cost for the existing case, but allows for more flexibility. This is done in commit 97789225ec41124feca5a4f3b857e7ca8c9e0809

All of these commits are in the oxidecomputer version of the cpuid-trait-and-serialize branch: https://github.com/oxidecomputer/rust-cpuid/tree/cpuid-trait-and-serialize

In case you're curious, here's the Humility cleanup that this unlocks. In particular, the added flexibility of the CpuIdReader trait means that we can eliminate a truly terrible hack, which was previously necessary to coerce a closure into a fn(u32, u32) -> CpuIdResult

gz commented 1 year ago

Ah good catch with the CpuIdReaderNative. Now I'm actually not sure anymore if it is a good idea to have this default even for x86. I fear it might make downstream code harder to cross-compile as it means anyone that uses CpuId as an argument somewhere now has to plaster the same cfg annotation everywhere (or explicitly specify the generic argument R: CpuIdReader anyways -- which isn't great :/)?

I might drop the commit which means potentially slightly more work for users migrating to v11.

gz / rust-cpuid

More granular display features #138

Codecov Report

`display::raw(...)`

`display::json(...)`