bitbybit 2.0.1 breaks dependent crates

[Marcondiro]

Hi and thanks a lot for the work you put in this crate!

Unfortunately the new 2.0.1 version can break crates depending on bitbybit 2.0.0.
As it is just patch update that should not have API changes, cargo can silently update 2.0.0 to 2.0.1 automatically.
It looks like 2.0.1 causes the builder function to not be available anymore in some cases, therefore this breaks the API.
Perhaps the version 2.0.1 should be yanked in favor of a 3.0.0?

Thanks again!

[danlehmann]

Do you have an example of a bitfield that breaks? I'd like to see if it can be fixed for 2.0.2. Unless you're talking about a Builder that was unbuildable before?

[Marcondiro]

Yes, in my specific case this used to have the builder() method, then it was possible to "build" it by just accessing the public .value

#[bitfield(u64)]
#[derive(Debug)]
pub struct IptOption {
    /// Output channel for captured trace data. Must be
    /// [`IptOption::TRACE_MODE_BUFFER`] or [`IptOption::TRACE_MODE_FILE`];
    /// any other value is rejected with `STATUS_INVALID_PARAMETER`.
    #[bits(0..=3, rw)]
    pub trace_mode: u4,
    /// Which extra timing packets are generated beyond the default TSC
    /// packets. One of the `TIMING_MODE_*` associated constants; values
    /// `>= 3` are rejected.
    #[bits(4..=7, rw)]
    pub timing_mode: u4,
    /// `IA32_RTIT_CTL.MTCFreq` (SDM bits 14:17 of the real MSR) — selects
    /// the periodicity of MTC packets. Must be a value the CPU advertises
    /// as legal (`CPUID.14H.1:EAX[31:16]` bitmap). Meaningful only when
    /// `timing_mode` is [`IptOption::TIMING_MODE_MTC`] or
    /// [`IptOption::TIMING_MODE_MTC_CYC`].
    #[bits(8..=11, rw)]
    pub mtc_freq: u4,
    /// `IA32_RTIT_CTL.CycThresh` (SDM bits 19:22 of the real MSR) — CYC
    /// packet threshold. Validated against `CPUID.14H.1:EBX[15:0]`.
    /// Meaningful only when `timing_mode` is
    /// [`IptOption::TIMING_MODE_MTC_CYC`].
    #[bits(12..=15, rw)]
    pub cyc_thresh: u4,
    /// Trace (`ToPA`) buffer size, as `4 KiB << buffer_size_exp` (legal up to
    /// `15` → 128 MiB). Honored in full on bare metal no matter how large;
    /// under Hyper'V without PT passthrough, values above `8` (1 MiB) make
    /// the start call fail outright instead of being scaled down. Must be
    /// non-zero for file-mode start.
    #[bits(16..=19, rw)]
    pub buffer_size_exp: u4,
    /// Reserved when tracing by process/thread handle (must be `0` for a
    /// direct `StartProcessTraceEx` (opcode 16) buffer/file-mode start).
    #[bits(20..=22, rw)]
    pub match_settings: u3,
    /// If set, a process started with this option causes child processes it
    /// creates to automatically start tracing with the same option value,
    /// bypassing the per-image IFEO registry lookup.
    #[bit(23, rw)]
    pub inherit_to_children: bool,
    /// Controls `IA32_RTIT_CTL.OS`/`.User`. One of the `RING_FILTER_*`
    /// associated constants. There are two parallel encodings depending on
    /// which API surface is setting it — see the constants' docs — using
    /// the wrong half for the wrong opcode is rejected. Kernel-inclusive
    /// values require `SeDebugPrivilege`.
    #[bits(24..=27, rw)]
    pub ring_filter: u4,
    #[bits(28..=63)]
    _reserved: u36,
}

I realize only now that the builder was supposed to be used with the build() method, probably since it was not available I assumed that .value was ok...
Not sure about the wider ecosystem impact but still an API breakage :/
If I am the only one impacted don't bother though, just giving a heads up.

[danlehmann]

Thanks! 2.0.1 was itself a hotfix for 2.0.0.

.value was never meant to be used no - it's a private value, but Rust lets code in the same module access it freely. Using the Builder with value doesn't buy you anything though - the whole point is to NOT be able to build() without having everything initialized. If you just want to set some stuff, I'd recommend just skipping the builder, e.g. Foo::DEFAULT.with_bar(false).

I had never considered someone using value directly. I'm not sure if there is a good way to guard against that.

[Marcondiro]

I don't know how feasible/convoluted it would be but, for guarding against misuse, how about changing builder.value type to the raw u8/16/... type? In that way it can not be used and mistaken as build() ?