diff --git a/CHANGELOG.md b/CHANGELOG.md index c8586088..1eca1e20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## [Unreleased] +### Fixed + +- `Writer`: an `Lsb0` write that ends on a byte boundary no longer reorders the + whole bytes of the `Msb0` write that follows it ([#677](https://github.com/sharksforarms/deku/pull/677)) + ### Other - Bump MSRV to 1.88 diff --git a/deku-derive/src/lib.rs b/deku-derive/src/lib.rs index 559deb68..ff2cdafd 100644 --- a/deku-derive/src/lib.rs +++ b/deku-derive/src/lib.rs @@ -99,7 +99,7 @@ impl FromMeta for Id { } } -#[derive(Debug)] +#[derive(Debug, Clone)] enum Num { LitInt(syn::LitInt), TokenStream(TokenStream), @@ -645,7 +645,7 @@ impl<'a> TryFrom<&'a DekuData> for DekuDataStruct<'a> { } /// A post-processed version of `FieldReceiver` -#[derive(Debug)] +#[derive(Debug, Clone)] struct FieldData { ident: Option, ty: Type, @@ -747,58 +747,99 @@ struct FieldData { impl FieldData { pub fn any_field_set(&self) -> bool { - // NOTE: Ignore ident - let mut any_option_set = self.endian.is_some(); - - #[cfg(feature = "bits")] - { - any_option_set = any_option_set || self.bits.is_some(); - } - - any_option_set = any_option_set || self.bytes.is_some() || self.count.is_some(); - - #[cfg(feature = "bits")] - { - any_option_set = any_option_set || self.bits_read.is_some(); - } - - any_option_set = any_option_set - || self.bytes_read.is_some() - || self.until.is_some() - || self.map.is_some() - || self.ctx.is_some() - || self.update.is_some() - || self.reader.is_some() - || self.writer.is_some(); + // Exhaustive on purpose: a new `FieldData` attribute fails to compile + // here until it is classified. + let Self { + // Not attributes. + ident: _, + ty: _, + // Always `Some`: `from_receiver` fills it in for every field. + default: _, + + endian, + #[cfg(feature = "bits")] + bits, + bytes, + count, + #[cfg(feature = "bits")] + bits_read, + bytes_read, + until, + read_all, + map, + ctx, + update, + reader, + writer, + skip, + #[cfg(feature = "bits")] + pad_bits_before, + pad_bytes_before, + #[cfg(feature = "bits")] + pad_bits_after, + pad_bytes_after, + temp, + temp_value, + cond, + assert, + assert_eq, + seek_rewind, + seek_from_current, + seek_from_end, + seek_from_start, + bit_order, + magic, + } = self; #[cfg(feature = "bits")] - { - any_option_set = any_option_set || self.pad_bits_before.is_some(); - } + let bits_attr_set = bits.is_some() + || bits_read.is_some() + || pad_bits_before.is_some() + || pad_bits_after.is_some(); + #[cfg(not(feature = "bits"))] + let bits_attr_set = false; + + let any_option_set = bits_attr_set + || endian.is_some() + || bytes.is_some() + || count.is_some() + || bytes_read.is_some() + || until.is_some() + || map.is_some() + || ctx.is_some() + || update.is_some() + || reader.is_some() + || writer.is_some() + || pad_bytes_before.is_some() + || pad_bytes_after.is_some() + || temp_value.is_some() + || cond.is_some() + || assert.is_some() + || assert_eq.is_some() + || seek_from_current.is_some() + || seek_from_end.is_some() + || seek_from_start.is_some() + || bit_order.is_some() + || magic.is_some(); + + let any_bool_set = *read_all || skip.is_some() || *temp || *seek_rewind; - any_option_set = any_option_set || self.pad_bytes_before.is_some(); + any_option_set || any_bool_set + } - #[cfg(feature = "bits")] - { - any_option_set = any_option_set || self.pad_bits_after.is_some(); + /// True if the field carries an attribute a batched read cannot reproduce: + /// anything that moves the cursor, makes the read conditional, or depends on a + /// value read earlier. + #[cfg(feature = "bits")] + pub fn any_field_set_incompatible_with_bit_run(&self) -> bool { + Self { + endian: None, + bits: None, + bit_order: None, + update: None, + ..self.clone() } - - // NOTE: Ignore default - any_option_set = any_option_set - || self.pad_bytes_after.is_some() - || self.temp_value.is_some() - || self.cond.is_some() - || self.assert.is_some() - || self.assert_eq.is_some() - || self.seek_from_current.is_some() - || self.seek_from_end.is_some() - || self.seek_from_start.is_some() - || self.bit_order.is_some() - || self.magic.is_some(); - - let any_bool_set = self.read_all || self.skip.is_some() || self.temp || self.seek_rewind; - - any_option_set || any_bool_set + .any_field_set() } fn from_receiver(receiver: DekuFieldReceiver) -> Result { diff --git a/deku-derive/src/macros/deku_read.rs b/deku-derive/src/macros/deku_read.rs index 89d38398..2c3ac17c 100644 --- a/deku-derive/src/macros/deku_read.rs +++ b/deku-derive/src/macros/deku_read.rs @@ -557,7 +557,27 @@ fn emit_field_reads( let mut use_id = use_id; - for (i, f) in fields.iter().enumerate() { + #[cfg(feature = "bits")] + let runs = plan_bit_runs(input, fields, use_id); + + let mut i = 0; + while i < fields.len() { + #[cfg(feature = "bits")] + if let Some(run) = runs.get(&i) { + let (idents, read) = emit_bit_run_read(fields, i, run, ident); + for field_ident in idents { + field_idents.push(FieldIdent { + field_ident, + is_temp: false, + }); + } + field_reads.push(read); + i += run.len(); + use_id = false; + continue; + } + + let f = fields.fields[i]; let (field_ident, field_read) = emit_field_read(input, i, f, ident, use_id)?; use_id = false; field_idents.push(FieldIdent { @@ -565,11 +585,215 @@ fn emit_field_reads( is_temp: f.temp, }); field_reads.push(field_read); + i += 1; } Ok((field_idents, field_reads)) } +/// One field of a contiguous big-endian `Msb0` bit-field run. +#[cfg(feature = "bits")] +pub(crate) struct BitRunField { + pub(crate) bits: usize, + pub(crate) ty: syn::Type, + /// Field takes the `Order`-carrying write impl, which words overflow + /// differently. + pub(crate) ordered: bool, + /// Whether a value can exceed `bits` at all. If not, no check is emitted. + pub(crate) can_overflow: bool, +} + +/// Widths of a run of adjacent fields that one read can serve. +#[cfg(feature = "bits")] +pub(crate) type BitRun = Vec; + +/// A field a run can serve: a literal `bits` on an unsigned primitive or `bool`, +/// explicitly big-endian, `Msb0`, carrying nothing else. Anything else keeps its +/// own read. +#[cfg(feature = "bits")] +pub(crate) fn run_field(input: &DekuData, f: &FieldData) -> Option { + if f.any_field_set_incompatible_with_bit_run() { + return None; + } + + // Big-endian must be explicit: with no attribute the context endian is the + // target's, which is little on x86. + let endian = f.endian.as_ref().or(input.endian.as_ref())?; + if endian.value() != "big" { + return None; + } + + // Only `Msb0` batches: absent is the default and fine, "lsb" is not, and + // anything else is a ctx parameter name forwarded as a runtime order, which + // could be either at run time. + let explicit_order = f.bit_order.as_ref().or(input.bit_order.as_ref()); + if let Some(order) = explicit_order { + if order.value() != "msb" { + return None; + } + } + // Which overflow wording this field's own write would have used. + let ordered = explicit_order.is_some(); + + let width = match &f.ty { + syn::Type::Path(p) if p.qself.is_none() => match p.path.get_ident()?.to_string().as_str() { + "u8" => u8::BITS as usize, + "u16" => u16::BITS as usize, + "u32" => u32::BITS as usize, + "u64" => u64::BITS as usize, + // `impls::bool` delegates to `u8`, so a bool is a byte unless + // `bits` narrows it. Flags in a packed header are usually `bits = 1`. + "bool" => u8::BITS as usize, + _ => return None, + }, + _ => return None, + }; + + let bits = match f.bits.as_ref() { + Some(crate::Num::LitInt(lit)) => lit.base10_parse::().ok()?, + Some(crate::Num::TokenStream(_)) => return None, + // A plain big-endian integer field is exactly `bits = width`: when the + // cursor is byte-aligned it is a big-endian byte read, and when it is not, + // deku already routes it through `read_bits_into` for the same `width` + // bits, most-significant first. + None => width, + }; + if bits == 0 || bits > width { + return None; + } + + // Neither a value filling its type nor a bool can exceed its width. + let can_overflow = bits < width && !is_bool(&f.ty); + + Some(BitRunField { + bits, + ty: f.ty.clone(), + ordered, + can_overflow, + }) +} + +/// A plain `bool`, which a run compares rather than casts. +#[cfg(feature = "bits")] +fn is_bool(ty: &syn::Type) -> bool { + matches!(ty, syn::Type::Path(p) if p.qself.is_none() && p.path.is_ident("bool")) +} + +/// Groups adjacent run-eligible fields, keyed by the index the run starts at. +/// +/// A run is capped at 64 bits, the width the reader returns, and must hold at +/// least two fields to be worth a batch. +#[cfg(feature = "bits")] +pub(crate) fn plan_bit_runs( + input: &DekuData, + fields: &Fields<&FieldData>, + use_id: bool, +) -> std::collections::HashMap { + let mut runs = std::collections::HashMap::new(); + let mut i = 0; + while i < fields.len() { + // The first field can be the enum id storage, which is not a read at all. + if i == 0 && use_id { + i = 1; + continue; + } + let mut run: BitRun = Vec::new(); + let mut total = 0usize; + let mut j = i; + while j < fields.len() { + let Some(field) = run_field(input, fields.fields[j]) else { + break; + }; + if total + field.bits > u64::BITS as usize { + break; + } + total += field.bits; + run.push(field); + j += 1; + } + if run.len() >= 2 { + let len = run.len(); + runs.insert(i, run); + i += len; + } else { + i += 1; + } + } + runs +} + +/// One read for the whole run, then shift and mask each field out of it. This is +/// what a hand-written parser does, and it replaces one `DekuReader` call plus one +/// bit-cursor update per field with one of each per run. +#[cfg(feature = "bits")] +fn emit_bit_run_read( + fields: &Fields<&FieldData>, + start: usize, + run: &BitRun, + ident: &TokenStream, +) -> (Vec, TokenStream) { + let crate_ = super::get_crate_name(); + let total: usize = run.iter().map(|f| f.bits).sum(); + let run_ident = quote::format_ident!("__deku_bit_run_{}", start); + let ident = ident.to_string(); + + let mut idents = Vec::with_capacity(run.len()); + let mut extracts = TokenStream::new(); + let mut consumed = 0usize; + for (offset, field) in run.iter().enumerate() { + let f = fields.fields[start + offset]; + let field_ident = f.get_ident(start + offset, true); + let internal = gen_internal_field_ident(&field_ident); + let shift = total - consumed - field.bits; + // A run is two or more fields in 64 bits, so none is 64 wide. + debug_assert!(field.bits < u64::BITS as usize); + let mask: u64 = (1u64 << field.bits) - 1; + let ty = &field.ty; + // `as` cannot produce a bool, so a bool field is compared rather than cast. + let extract = if is_bool(ty) { + if field.bits == 1 { + // One bit is 0 or 1: nothing to reject. + quote! { ((#run_ident >> #shift) & 1) != 0 } + } else { + // Wider bools reject anything but 0 and 1, as `impls::bool` does. + quote! { + match (#run_ident >> #shift) & #mask { + 0 => false, + 1 => true, + __deku_bool => return Err(::#crate_::deku_error!( + ::#crate_::DekuError::Parse, + "cannot parse bool value", + "{}", + __deku_bool as u8 + )), + } + } + } + } else { + quote! { ((#run_ident >> #shift) & #mask) as #ty } + }; + let trace_field_log = if cfg!(feature = "logging") { + let field_ident_str = field_ident.to_string(); + quote! { log::trace!("Reading: {}.{}", #ident, #field_ident_str); } + } else { + quote! {} + }; + extracts.extend(quote! { + #trace_field_log + let #internal = #extract; + let #field_ident = &#internal; + }); + idents.push(field_ident); + consumed += field.bits; + } + + let read = quote! { + let #run_ident: u64 = __deku_reader.read_bits_uint_msb0(#total)?; + #extracts + }; + (idents, read) +} + fn emit_bit_byte_offsets( fields: &[&Option], ) -> (Option, Option) { @@ -1069,3 +1293,339 @@ pub fn emit_try_from( } } } + +#[cfg(test)] +#[cfg(feature = "bits")] +mod tests { + use rstest::rstest; + + use super::*; + + /// Sorts a planner result into `(index of the first field, widths)` pairs. + fn sorted(runs: std::collections::HashMap) -> Vec<(usize, Vec)> { + let mut runs: Vec<_> = runs + .into_iter() + .map(|(start, run)| (start, run.iter().map(|f| f.bits).collect::>())) + .collect(); + runs.sort_by_key(|(start, _)| *start); + runs + } + + /// Every run the planner forms over a struct. + fn plan(src: &str) -> Vec<(usize, Vec)> { + plan_with_id(src, false) + } + + /// As `plan`, with `use_id`: the first field is an enum's id storage, not a read. + fn plan_with_id(src: &str, use_id: bool) -> Vec<(usize, Vec)> { + let data = DekuData::from_input(src.parse().unwrap()).expect("input should parse"); + let fields = data + .data + .as_ref() + .take_struct() + .expect("test input should be a struct"); + + sorted(plan_bit_runs(&data, &fields, use_id)) + } + + /// Every run the planner forms over one variant of an enum. + fn plan_variant(src: &str, variant: usize, use_id: bool) -> Vec<(usize, Vec)> { + let data = DekuData::from_input(src.parse().unwrap()).expect("input should parse"); + let variants = data + .data + .as_ref() + .take_enum() + .expect("test input should be an enum"); + let fields = variants[variant].fields.as_ref(); + + sorted(plan_bit_runs(&data, &fields, use_id)) + } + + /// A struct of big-endian `u8` fields, one per `bits` width given. + fn be_struct(widths: &[usize]) -> String { + let fields: String = widths + .iter() + .enumerate() + .map(|(i, w)| format!("#[deku(bits = {w})] f{i}: u8,")) + .collect(); + format!(r#"#[deku(endian = "big")] struct Test {{ {fields} }}"#) + } + + #[test] + fn adjacent_fields_share_one_read() { + assert_eq!(plan(&be_struct(&[2, 3, 3])), vec![(0, vec![2, 3, 3])]); + } + + #[test] + fn a_lone_field_is_not_a_run() { + // One field costs the same read either way. + assert_eq!(plan(&be_struct(&[5])), vec![]); + } + + #[test] + fn plain_fields_without_bits_are_their_full_width() { + let src = r#"#[deku(endian = "big")] struct Test { a: u8, b: u16, c: u32 }"#; + assert_eq!(plan(src), vec![(0, vec![8, 16, 32])]); + } + + #[test] + fn a_run_is_capped_at_64_bits_and_the_next_one_starts_there() { + // 32 + 32 fills a run exactly, so the third field opens a second. + let src = r#"#[deku(endian = "big")] struct Test { a: u32, b: u32, c: u32, d: u32 }"#; + assert_eq!(plan(src), vec![(0, vec![32, 32]), (2, vec![32, 32])]); + + // A field that does not fit closes the run rather than overflowing it. + let src = r#"#[deku(endian = "big")] struct Test { a: u32, b: u16, c: u32 }"#; + assert_eq!(plan(src), vec![(0, vec![32, 16])]); + } + + #[test] + fn an_ineligible_field_splits_a_run_in_two() { + let src = r#" + #[deku(endian = "big")] + struct Test { + #[deku(bits = 2)] a: u8, + #[deku(bits = 2)] b: u8, + #[deku(endian = "little")] c: u16, + #[deku(bits = 2)] d: u8, + #[deku(bits = 2)] e: u8, + }"#; + assert_eq!(plan(src), vec![(0, vec![2, 2]), (3, vec![2, 2])]); + } + + #[test] + fn endianness_must_be_explicitly_big() { + // Absent means the target's endianness, little on x86. + assert_eq!(plan(r#"struct Test { a: u8, b: u8 }"#), vec![]); + assert_eq!( + plan(r#"#[deku(endian = "little")] struct Test { a: u8, b: u8 }"#), + vec![] + ); + // A field-level attribute qualifies a field inside a little-endian struct. + let src = r#"#[deku(endian = "little")] struct Test { + #[deku(endian = "big")] a: u8, + #[deku(endian = "big")] b: u8, + }"#; + assert_eq!(plan(src), vec![(0, vec![8, 8])]); + } + + #[test] + fn bit_order_must_be_msb() { + // `Msb0` is the default, so absent qualifies, and so does spelling it out. + let src = r#"#[deku(endian = "big")] struct Test { a: u8, b: u8 }"#; + assert_eq!(plan(src), vec![(0, vec![8, 8])]); + + let src = r#"#[deku(endian = "big", bit_order = "msb")] struct Test { a: u8, b: u8 }"#; + assert_eq!(plan(src), vec![(0, vec![8, 8])]); + + let src = r#"#[deku(endian = "big")] struct Test { + #[deku(bit_order = "msb")] a: u8, + b: u8, + }"#; + assert_eq!(plan(src), vec![(0, vec![8, 8])]); + + // "lsb" does not. + let src = r#"#[deku(endian = "big", bit_order = "lsb")] struct Test { a: u8, b: u8 }"#; + assert_eq!(plan(src), vec![]); + + let src = r#"#[deku(endian = "big")] struct Test { + #[deku(bit_order = "lsb")] a: u8, + b: u8, + }"#; + assert_eq!(plan(src), vec![]); + } + + #[test] + fn a_runtime_bit_order_does_not_batch() { + // An `Order`-typed `ctx` parameter is forwarded as `bit_order`, so the + // order is only known at run time and could be `Lsb0`. + let src = r#"#[deku(endian = "big", ctx = "order: deku::ctx::Order")] struct Test { + #[deku(bits = 4)] a: u8, + #[deku(bits = 4)] b: u8, + }"#; + assert_eq!(plan(src), vec![]); + + // A wildcard binds nothing, so there is no runtime order to honour. + let src = r#"#[deku(endian = "big", ctx = "_: deku::ctx::Order")] struct Test { + #[deku(bits = 4)] a: u8, + #[deku(bits = 4)] b: u8, + }"#; + assert_eq!(plan(src), vec![(0, vec![4, 4])]); + } + + #[test] + fn an_explicit_bit_order_selects_the_other_overflow_wording() { + // Both batch, but reach different write impls, so each needs its own wording. + let src = r#"#[deku(endian = "big")] struct Test { + #[deku(bits = 4, bit_order = "msb")] ordered: u8, + #[deku(bits = 4)] plain: u8, + }"#; + let data = DekuData::from_input(src.parse().unwrap()).unwrap(); + let fields = data.data.as_ref().take_struct().unwrap(); + let runs = plan_bit_runs(&data, &fields, false); + let run = runs.get(&0).expect("both fields should batch"); + assert_eq!( + run.iter().map(|f| f.ordered).collect::>(), + vec![true, false] + ); + + let emitted = emit_deku_read(&data).unwrap().to_string(); + assert_eq!(emitted.matches("read_bits_uint_msb0").count(), 1); + } + + #[test] + fn a_bool_joins_a_run() { + // Excluding bools would split a run wherever a flag sits. + let src = r#"#[deku(endian = "big")] struct Test { + #[deku(bits = 2)] a: u8, + #[deku(bits = 1)] flag: bool, + #[deku(bits = 5)] b: u8, + }"#; + assert_eq!(plan(src), vec![(0, vec![2, 1, 5])]); + + // Without `bits` a bool is a byte, as `impls::bool` reads it. + let src = r#"#[deku(endian = "big")] struct Test { flag: bool, b: u8 }"#; + assert_eq!(plan(src), vec![(0, vec![8, 8])]); + } + + #[test] + fn the_rtp_header_batches_into_one_read() { + // RFC 3550 fixed header: 9 fields, three of them flags. + let src = r#"#[deku(endian = "big")] struct Rtp { + #[deku(bits = 2)] version: u8, + #[deku(bits = 1)] padding: bool, + #[deku(bits = 1)] extension: bool, + #[deku(bits = 4)] csrc_count: u8, + #[deku(bits = 1)] marker: bool, + #[deku(bits = 7)] payload_type: u8, + sequence_number: u16, + timestamp: u32, + ssrc: u32, + }"#; + // The first eight sum to 64 bits; `ssrc` cannot fit. Two reads, not seven. + assert_eq!(plan(src), vec![(0, vec![2, 1, 1, 4, 1, 7, 16, 32])]); + } + + #[test] + fn only_unsigned_primitives_and_bool_qualify() { + for ty in ["i8", "i16", "f32", "MyEnum", "Vec", "[u8; 2]"] { + let src = format!(r#"#[deku(endian = "big")] struct Test {{ a: {ty}, b: {ty} }}"#); + assert_eq!(plan(&src), vec![], "{ty} must not form a run"); + } + } + + #[test] + fn bits_must_be_a_literal_and_fit_the_type() { + // A non-literal width is not known at expansion time. + let src = r#"#[deku(endian = "big", ctx = "n: usize")] struct Test { + #[deku(bits = "n")] a: u8, + #[deku(bits = "n")] b: u8, + }"#; + assert_eq!(plan(src), vec![]); + + // Wider than its container. + let src = r#"#[deku(endian = "big")] struct Test { + #[deku(bits = 9)] a: u8, + #[deku(bits = 2)] b: u8, + }"#; + assert_eq!(plan(src), vec![]); + } + + /// Every attribute that must keep a field out of a run. Two fields that would + /// otherwise batch, with the attribute on the first. + #[rstest] + #[case::bytes("bytes = 1")] + #[case::pad_bits_before("pad_bits_before = \"1\"")] + #[case::pad_bytes_before("pad_bytes_before = \"1\"")] + #[case::pad_bits_after("pad_bits_after = \"1\"")] + #[case::pad_bytes_after("pad_bytes_after = \"1\"")] + #[case::cond("cond = \"true\"")] + #[case::assert("assert = \"true\"")] + #[case::assert_eq("assert_eq = \"0\"")] + #[case::map("map = \"|v: u8| -> Result<_, DekuError> { Ok(v) }\"")] + #[case::reader("reader = \"read_it()\"")] + #[case::writer("writer = \"write_it()\"")] + #[case::skip_with_default("skip, default = \"0\"")] + #[case::temp("temp")] + #[case::seek_rewind("seek_rewind")] + #[case::seek_from_current("seek_from_current = \"1\"")] + #[case::seek_from_end("seek_from_end = \"0\"")] + #[case::seek_from_start("seek_from_start = \"0\"")] + #[case::magic("magic = b\"\\x01\"")] + fn a_disqualifying_attribute_keeps_a_field_out_of_a_run(#[case] attr: &str) { + let src = + format!(r#"#[deku(endian = "big")] struct Test {{ #[deku({attr})] a: u8, b: u8 }}"#); + assert_eq!( + plan(&src), + vec![], + "`{attr}` must keep the field out of a run" + ); + } + + #[test] + fn the_id_storage_field_is_never_part_of_a_run() { + // The id has already been read, so it cannot join the run behind it. + let src = &be_struct(&[2, 3, 3]); + assert_eq!(plan_with_id(src, false), vec![(0, vec![2, 3, 3])]); + assert_eq!(plan_with_id(src, true), vec![(1, vec![3, 3])]); + + // One field left behind the id is no run. + let src = &be_struct(&[2, 6]); + assert_eq!(plan_with_id(src, true), vec![]); + } + + #[test] + fn a_run_forms_inside_an_enum_variant() { + let src = r#" + #[deku(id_type = "u8", endian = "big")] + enum Test { + #[deku(id = 1)] + Named { + #[deku(bits = 2)] a: u8, + #[deku(bits = 6)] b: u8, + }, + #[deku(id = 2)] + Unnamed(#[deku(bits = 4)] u8, #[deku(bits = 4)] u8), + }"#; + assert_eq!(plan_variant(src, 0, false), vec![(0, vec![2, 6])]); + // Unnamed fields take a different ident path but plan the same. + assert_eq!(plan_variant(src, 1, false), vec![(0, vec![4, 4])]); + } + + #[test] + fn a_run_forms_in_a_tuple_struct() { + let src = r#"#[deku(endian = "big")] struct Test( + #[deku(bits = 3)] u8, + #[deku(bits = 5)] u8, + );"#; + assert_eq!(plan(src), vec![(0, vec![3, 5])]); + } + + #[test] + fn update_does_not_keep_a_field_out_of_a_run() { + // `update` feeds only `DekuUpdate`, so it cannot change the read or write. + let src = r#"#[deku(endian = "big")] struct Test { + #[deku(bits = 4, update = "0")] a: u8, + #[deku(bits = 4)] b: u8, + }"#; + assert_eq!(plan(src), vec![(0, vec![4, 4])]); + } + + #[test] + fn the_emitted_read_makes_one_call_for_the_whole_run() { + // What round-trip tests cannot show: three fields, one call. + let data = DekuData::from_input(be_struct(&[2, 3, 3]).parse().unwrap()).unwrap(); + let emitted = emit_deku_read(&data).unwrap().to_string(); + assert_eq!(emitted.matches("read_bits_uint_msb0").count(), 1); + + // And without a run, one call per field. + let src = r#"#[deku(endian = "little")] struct Test { + #[deku(bits = 2)] a: u8, + #[deku(bits = 3)] b: u8, + #[deku(bits = 3)] c: u8, + }"#; + let data = DekuData::from_input(src.parse().unwrap()).unwrap(); + let emitted = emit_deku_read(&data).unwrap().to_string(); + assert_eq!(emitted.matches("read_bits_uint_msb0").count(), 0); + } +} diff --git a/deku-derive/src/macros/deku_write.rs b/deku-derive/src/macros/deku_write.rs index 52dab44b..a5c04304 100644 --- a/deku-derive/src/macros/deku_write.rs +++ b/deku-derive/src/macros/deku_write.rs @@ -471,11 +471,98 @@ fn emit_field_writes( ident: &TokenStream, ) -> Result, syn::Error> { let mut is_id_pat = is_id_pat; - fields - .iter() - .enumerate() - .map(|(i, f)| emit_field_write(input, i, f, &object_prefix, ident, &mut is_id_pat)) - .collect() + + #[cfg(feature = "bits")] + let runs = super::deku_read::plan_bit_runs(input, fields, is_id_pat); + + let mut writes = Vec::with_capacity(fields.len()); + let mut i = 0; + while i < fields.len() { + #[cfg(feature = "bits")] + if let Some(run) = runs.get(&i) { + writes.push(emit_bit_run_write(fields, i, run, &object_prefix, ident)); + i += run.len(); + is_id_pat = false; + continue; + } + + let f = fields.fields[i]; + writes.push(emit_field_write( + input, + i, + f, + &object_prefix, + ident, + &mut is_id_pat, + )?); + i += 1; + } + + Ok(writes) +} + +/// Composes a run of adjacent fields into one integer and writes it once, the +/// mirror of the read side. Each field keeps its own "does the value fit" check, +/// which is the only per-field work the individual writes did. +#[cfg(feature = "bits")] +fn emit_bit_run_write( + fields: &Fields<&FieldData>, + start: usize, + run: &super::deku_read::BitRun, + object_prefix: &Option, + ident: &TokenStream, +) -> TokenStream { + let crate_ = super::get_crate_name(); + let total: usize = run.iter().map(|f| f.bits).sum(); + let ident = ident.to_string(); + + let mut traces = TokenStream::new(); + let mut checks = TokenStream::new(); + let mut terms = Vec::with_capacity(run.len()); + let mut widths = Vec::with_capacity(run.len()); + let mut consumed = 0usize; + for (offset, field) in run.iter().enumerate() { + let f = fields.fields[start + offset]; + let field_ident = f.get_ident(start + offset, object_prefix.is_none()); + let bits = field.bits; + let shift = total - consumed - bits; + // A run holds at least two fields totalling at most 64 bits, so no single + // field in one is 64 bits wide and the shift below cannot overflow. + debug_assert!(bits < u64::BITS as usize); + let mask: u64 = (1u64 << bits) - 1; + + if cfg!(feature = "logging") { + let field_ident_str = field_ident.to_string(); + traces.extend(quote! { log::trace!("Writing: {}.{}", #ident, #field_ident_str); }); + } + + let value = quote! { (*(#object_prefix #field_ident) as u64) }; + // Every field keeps the rejection its own write performed. Where the field + // fills its type this folds away at compile time, because a value cast from + // that type cannot exceed the width. + if field.can_overflow { + let ordered = field.ordered; + checks.extend(quote! { + ::#crate_::writer::check_bit_size::<#ordered>(#value, #bits)?; + }); + } + terms.push(quote! { ((#value & #mask) << #shift) }); + widths.push(bits); + consumed += bits; + } + + quote! { + #traces + #checks + let __deku_bit_run: u64 = #(#terms)|*; + // A partial `Lsb0` leftover cannot be spliced onto in one go: the general + // path reorders across a multi-byte write, so fall back per field. + if __deku_writer.can_write_bits_uint_msb0() { + __deku_writer.write_bits_uint_msb0(__deku_bit_run, #total)?; + } else { + __deku_writer.write_bits_uint_fields(__deku_bit_run, &[#(#widths),*])?; + } + } } fn emit_field_updates( @@ -871,3 +958,78 @@ fn check_update_use(vec: &[T]) -> TokenStream { quote! {} } } + +#[cfg(test)] +#[cfg(feature = "bits")] +mod tests { + use super::*; + + /// The `DekuWrite` impl the derive emits for `src`, as a token string. + fn emitted(src: &str) -> String { + let data = crate::DekuData::from_input(src.parse().unwrap()).expect("input should parse"); + emit_deku_write(&data) + .expect("input should emit") + .to_string() + } + + /// Three adjacent big-endian bit fields, which form one run. + const RUN: &str = r#"#[deku(endian = "big")] struct Test { + #[deku(bits = 2)] a: u8, + #[deku(bits = 3)] b: u8, + #[deku(bits = 3)] c: u8, + }"#; + + /// The same widths, little-endian, so no run forms. + const NO_RUN: &str = r#"#[deku(endian = "little")] struct Test { + #[deku(bits = 2)] a: u8, + #[deku(bits = 3)] b: u8, + #[deku(bits = 3)] c: u8, + }"#; + + #[test] + fn the_emitted_write_makes_one_call_for_the_whole_run() { + assert_eq!(emitted(RUN).matches("can_write_bits_uint_msb0").count(), 1); + } + + #[test] + fn without_a_run_no_batched_write_is_emitted() { + assert_eq!( + emitted(NO_RUN).matches("can_write_bits_uint_msb0").count(), + 0 + ); + } + + #[test] + fn a_field_that_cannot_overflow_gets_no_check() { + // Whole bytes fill their type and bools are 0 or 1: neither can overflow. + let src = r#"#[deku(endian = "big")] struct Test { a: u8, b: u16 }"#; + assert_eq!(emitted(src).matches("check_bit_size").count(), 0); + + let src = r#"#[deku(endian = "big")] struct Test { + #[deku(bits = 1)] flag: bool, + #[deku(bits = 7)] rest: u8, + }"#; + // Only `rest` is narrower than its type. + assert_eq!(emitted(src).matches("check_bit_size").count(), 1); + } + + #[test] + fn every_field_in_a_run_keeps_its_overflow_check() { + // Folding three fields into one integer must not lose the per-field check. + assert_eq!(emitted(RUN).matches("check_bit_size").count(), 3); + } + + #[test] + fn a_run_inside_an_enum_variant_is_written_in_one_call() { + let src = r#" + #[deku(id_type = "u8", endian = "big")] + enum Test { + #[deku(id = 1)] + A { + #[deku(bits = 4)] a: u8, + #[deku(bits = 4)] b: u8, + }, + }"#; + assert_eq!(emitted(src).matches("can_write_bits_uint_msb0").count(), 1); + } +} diff --git a/src/reader.rs b/src/reader.rs index d7a844ba..0d8b66d6 100644 --- a/src/reader.rs +++ b/src/reader.rs @@ -383,6 +383,9 @@ impl Reader { /// Reads `amt` bits (`1..=64`) most-significant-bit first, returning them /// right-aligned in a `u64`. /// + /// Public because the derive calls it to read a run of contiguous + /// big-endian `Msb0` bit-fields in one go. + /// /// This is the integer fast path for the common big-endian / `Msb0` case. It /// is equivalent to `read_bits_into` followed by `load_be`, but the value /// never touches a `BitSlice`: the leftover is a byte and a length, so @@ -394,7 +397,7 @@ impl Reader { /// `amt`, exactly as `read_bits_into` does, so it consumes no more input. #[inline] #[cfg(feature = "bits")] - pub(crate) fn read_bits_uint_msb0(&mut self, amt: usize) -> Result { + pub fn read_bits_uint_msb0(&mut self, amt: usize) -> Result { debug_assert!((1..=64).contains(&amt)); // Up to 7 leftover bits plus 64 requested does not fit a u64. @@ -416,10 +419,11 @@ impl Reader { } // One byte at a time, which reads exactly as much as the field needs and - // no more. Batching the whole field into a single variable-length - // `read_exact` wins on a microbenchmark over wide fields, but loses on a - // realistic multi-protocol pipeline, where fields are mostly narrow and - // the extra branch costs more than it saves. + // no more. Batching the whole field into one variable-length `read_exact` + // wins on a microbenchmark but loses on a realistic multi-protocol + // pipeline. Tried three times: with narrow fields, with a `need == 1` + // fast path, and again after the derive started batching runs so the + // reads are wide. Slower every time. while have < amt { let mut buf = [0u8; 1]; if let Err(e) = self.inner.read_exact(&mut buf) { diff --git a/src/writer.rs b/src/writer.rs index d743dbc4..d1903f09 100644 --- a/src/writer.rs +++ b/src/writer.rs @@ -17,6 +17,49 @@ const fn bits_of() -> usize { core::mem::size_of::().saturating_mul(::BITS as usize) } +/// Errors unless `value` fits in `bits` bits. +/// +/// The derive calls this per field before composing a run into one write. Two +/// comparisons, so it inlines and folds away where a field fills its type. +/// +/// `ORDERED` picks the wording: `DekuWriter<(Endian, BitSize, Order)>` omits the +/// second "bit" that `DekuWriter<(Endian, BitSize)>` includes. Unify those two and +/// this parameter goes away. +#[cfg(feature = "bits")] +#[inline] +pub fn check_bit_size(value: u64, bits: usize) -> Result<(), DekuError> { + if bits >= u64::BITS as usize || (value >> bits) == 0 { + return Ok(()); + } + Err(bit_size_error::(value, bits)) +} + +/// Cold path of [`check_bit_size`], out of line so the check inlines. +#[cfg(feature = "bits")] +#[cold] +#[inline(never)] +fn bit_size_error(value: u64, bits: usize) -> DekuError { + // Bits `value` occupies, which is what the per-field writes report. + let significant = (u64::BITS - value.leading_zeros()) as usize; + if ORDERED { + crate::deku_error!( + DekuError::InvalidParam, + "bit size of input is larger than requested size", + "{} exceeds {}", + significant, + bits + ) + } else { + crate::deku_error!( + DekuError::InvalidParam, + "bit size of input is larger than bit requested size", + "{} exceeds {}", + significant, + bits + ) + } +} + /// Container to use with `from_reader` pub struct Writer { pub(crate) inner: W, @@ -65,16 +108,26 @@ impl Writer { /// Writes the low `amt` bits (`1..=64`) of `value`, most-significant-bit /// first. The integer mirror of `Reader::read_bits_uint_msb0`. /// - /// Only valid when the pending leftover is `Msb0`; the caller checks that. + /// Public because the derive calls it to write a run of contiguous + /// big-endian `Msb0` bit-fields in one go. + /// + /// A pending `Lsb0` leftover cannot be spliced onto by the integer path, so + /// that case falls back to [`Writer::write_bits_uint_fields`]. It happens when + /// a struct written `Lsb0` is followed by one written `Msb0`. + /// /// Equivalent to `write_bits_order(.., Order::Msb0)` over the same bits, but /// the value never becomes a `BitSlice`: whole bytes leave in one `write_all` /// instead of one call per byte, and the leftover is a byte and a length /// rather than a `BoundedBitVec` rebuilt bit by bit. #[inline] #[cfg(feature = "bits")] - pub(crate) fn write_bits_uint_msb0(&mut self, value: u64, amt: usize) -> Result<(), DekuError> { + pub fn write_bits_uint_msb0(&mut self, value: u64, amt: usize) -> Result<(), DekuError> { debug_assert!((1..=64).contains(&amt)); - debug_assert_eq!(self.leftover.1, Order::Msb0); + + // A partial `Lsb0` byte cannot be spliced onto here: callers must check + // `can_write_bits_uint_msb0` and use `write_bits_uint_fields` instead. An + // empty leftover is already `Msb0`, so there is no flag to reset. + debug_assert!(self.can_write_bits_uint_msb0()); let (lead, lead_len) = self.leftover.0.as_msb0_byte(); // Leftover bits first, then the value's low `amt` bits: at most 7 + 64. @@ -108,6 +161,47 @@ impl Writer { Ok(()) } + /// Whether [`Writer::write_bits_uint_msb0`] can serve the next write. + /// + /// False only with a partial `Lsb0` byte pending, where one batched write is + /// not equivalent to the per-field writes it would replace. + #[inline] + #[cfg(feature = "bits")] + pub fn can_write_bits_uint_msb0(&self) -> bool { + self.leftover.1 == Order::Msb0 || self.leftover.0.is_empty() + } + + /// Writes the fields packed into `value` one at a time through the general bit + /// path, `widths` giving each field's width most-significant first. + /// + /// The per-field equivalent of [`Writer::write_bits_uint_msb0`], for when + /// [`Writer::can_write_bits_uint_msb0`] is false. One call rather than one per + /// field, so the branch the derive emits for it stays small. + #[cfg(feature = "bits")] + pub fn write_bits_uint_fields( + &mut self, + value: u64, + widths: &[usize], + ) -> Result<(), DekuError> { + let total: usize = widths.iter().sum(); + debug_assert!((1..=64).contains(&total)); + + let mut consumed = 0usize; + for &amt in widths { + let shift = total - consumed - amt; + let mask = if amt >= u64::BITS as usize { + u64::MAX + } else { + (1u64 << amt) - 1 + }; + // Left-align so an `Msb0` view reads the bits most-significant first. + let bytes = (((value >> shift) & mask) << (u64::BITS as usize - amt)).to_be_bytes(); + self.write_bits_order(&bytes.view_bits::()[..amt], Order::Msb0)?; + consumed += amt; + } + Ok(()) + } + #[cfg(feature = "bits")] fn write_bits_order_msb_msb( &mut self, @@ -346,7 +440,7 @@ impl Writer { bits: &BitSlice, order: Order, ) -> Result<(), DekuError> { - match self.leftover.1 { + let result = match self.leftover.1 { Order::Msb0 => match order { Order::Msb0 => self.write_bits_order_msb_msb(bits, order), Order::Lsb0 => self.write_bits_order_msb_lsb(bits, order), @@ -355,7 +449,17 @@ impl Writer { Order::Msb0 => self.write_bits_order_lsb_msb(bits, order), Order::Lsb0 => self.write_bits_order_lsb_lsb(bits, order), }, + }; + + // The paths above record `order` even with no bits left pending, but an + // empty leftover has no order: left set, the flag steers the next write + // into a `Lsb0` path, which emits whole bytes back to front. Reset it, so + // every reader of the flag can take an empty leftover as `Msb0`. + if self.leftover.0.is_empty() { + self.leftover.1 = Order::Msb0; } + + result } /// Write all bits to `Writer` buffer if bits can fit into a byte buffer @@ -461,6 +565,37 @@ mod tests { ); } + /// A `Lsb0` write that ends on a byte boundary leaves no bits pending, so it + /// must not steer the `Msb0` write that follows into the `Lsb0` path, which + /// emits whole bytes back to front. + #[test] + fn test_msb0_after_byte_aligned_lsb0_is_not_reordered() { + let mut stale = Cursor::new(vec![]); + let mut writer = Writer::new(&mut stale); + writer + .write_bits_order(&BitVec::::from_slice(&[0x41]), Order::Lsb0) + .unwrap(); + let pending = (writer.leftover.0.is_empty(), writer.leftover.1); + writer + .write_bits_order(&BitVec::::from_slice(&[0xab, 0xcd]), Order::Msb0) + .unwrap(); + writer.finalize().unwrap(); + + // The same two writes on a writer that never saw `Lsb0`. + let mut fresh = Cursor::new(vec![]); + let mut writer = Writer::new(&mut fresh); + writer + .write_bits_order(&BitVec::::from_slice(&[0x41]), Order::Msb0) + .unwrap(); + writer + .write_bits_order(&BitVec::::from_slice(&[0xab, 0xcd]), Order::Msb0) + .unwrap(); + writer.finalize().unwrap(); + + assert_eq_hex!(stale.into_inner(), fresh.into_inner()); + assert_eq!(pending, (true, Order::Msb0)); + } + #[test] fn test_writer_bytes() { let mut out_buf = Cursor::new(vec![]); diff --git a/tests/test_attributes/test_update.rs b/tests/test_attributes/test_update.rs index 5326251d..f25a460b 100644 --- a/tests/test_attributes/test_update.rs +++ b/tests/test_attributes/test_update.rs @@ -76,3 +76,35 @@ fn test_update_error() { val.update().unwrap(); } + +/// `update` on a field inside a batched bit-field run still rewrites it, because +/// it feeds only the `DekuUpdate` impl. +#[test] +#[cfg(feature = "bits")] +fn test_update_in_bit_run() { + #[derive(PartialEq, Debug, DekuRead, DekuWrite)] + #[deku(endian = "big")] + struct TestStruct { + #[deku(bits = 4)] + field_a: u8, + #[deku(bits = 4, update = "9")] + field_b: u8, + field_c: u8, + } + + let mut ret_read = TestStruct::try_from([0x12u8, 0x34].as_slice()).unwrap(); + assert_eq!( + TestStruct { + field_a: 0x1, + field_b: 0x2, + field_c: 0x34 + }, + ret_read + ); + + ret_read.update().unwrap(); + assert_eq!(0x9, ret_read.field_b); + + let ret_write: Vec = ret_read.try_into().unwrap(); + assert_eq!(vec![0x19, 0x34], ret_write); +} diff --git a/tests/test_bit_run_logging.rs b/tests/test_bit_run_logging.rs new file mode 100644 index 00000000..6651841a --- /dev/null +++ b/tests/test_bit_run_logging.rs @@ -0,0 +1,56 @@ +//! A batched run must trace every field it serves, as the reads and writes it +//! replaces each did. +//! +//! Its own binary: the logger is global, so a capture here would otherwise pick +//! up every other test in the same process. +#![cfg(all(feature = "logging", feature = "alloc", feature = "bits"))] + +use deku::prelude::*; +use std::sync::Mutex; + +static LINES: Mutex> = Mutex::new(Vec::new()); + +struct Capture; + +impl log::Log for Capture { + fn enabled(&self, _: &log::Metadata) -> bool { + true + } + + fn log(&self, record: &log::Record) { + let line = format!("{}", record.args()); + if line.starts_with("Reading:") || line.starts_with("Writing:") { + LINES.lock().unwrap().push(line); + } + } + + fn flush(&self) {} +} + +#[derive(Debug, PartialEq, DekuRead, DekuWrite)] +#[deku(endian = "big")] +struct Batched { + #[deku(bits = 12)] + a: u16, + #[deku(bits = 4)] + b: u8, +} + +#[test] +fn a_run_traces_every_field_it_serves() { + log::set_logger(&Capture).unwrap(); + log::set_max_level(log::LevelFilter::Trace); + + let (_, v) = Batched::from_bytes((&[0xAB, 0xCD], 0)).unwrap(); + v.to_bytes().unwrap(); + + assert_eq!( + *LINES.lock().unwrap(), + [ + "Reading: Batched.a", + "Reading: Batched.b", + "Writing: Batched.a", + "Writing: Batched.b", + ] + ); +} diff --git a/tests/test_bit_runs.rs b/tests/test_bit_runs.rs new file mode 100644 index 00000000..a29a030c --- /dev/null +++ b/tests/test_bit_runs.rs @@ -0,0 +1,777 @@ +//! Adjacent big-endian `Msb0` bit fields served by one read and one write. +//! +//! Batching must be invisible: same values, same rejections, same bytes as an +//! unbatched struct. `assert = "true"` is the control, since it disqualifies a +//! field from a run while being a no-op. +#![cfg(all(feature = "alloc", feature = "bits"))] + +use deku::prelude::*; + +/// A bit-packed header: 5 fields, 48 bits. Declared twice from one definition so +/// the two cannot drift apart. +macro_rules! header { + ($name:ident, $($extra:tt)*) => { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big")] + struct $name { + #[deku(bits = 3 $($extra)*)] + version: u8, + #[deku(bits = 13 $($extra)*)] + id: u16, + #[deku(bits = 2 $($extra)*)] + seq_flags: u8, + #[deku(bits = 14 $($extra)*)] + seq_count: u16, + #[deku(bits = 16 $($extra)*)] + length: u16, + } + }; +} + +// One run of 48 bits. +header!(Batched,); +// Every field disqualified, so five separate reads and writes. +header!(Unbatched, , assert = "true"); + +/// Deterministic pseudo-random bytes, so a failure is reproducible. +fn wire(len: usize, seed: u32) -> Vec { + let mut x = seed; + (0..len) + .map(|_| { + x = x.wrapping_mul(1_664_525).wrapping_add(1_013_904_223); + (x >> 24) as u8 + }) + .collect() +} + +/// The core claim: every field and the bytes written back agree with unbatched. +#[test] +fn batched_matches_unbatched() { + for seed in 1..20_000u32 { + let data = wire(6, seed); + + let (_, batched) = Batched::from_bytes((&data, 0)).unwrap(); + let (_, plain) = Unbatched::from_bytes((&data, 0)).unwrap(); + + assert_eq!(batched.version, plain.version, "version, seed {seed}"); + assert_eq!(batched.id, plain.id, "id, seed {seed}"); + assert_eq!(batched.seq_flags, plain.seq_flags, "seq_flags, seed {seed}"); + assert_eq!(batched.seq_count, plain.seq_count, "seq_count, seed {seed}"); + assert_eq!(batched.length, plain.length, "length, seed {seed}"); + + // And both must write back the bytes they came from. + assert_eq!(batched.to_bytes().unwrap(), data, "seed {seed}"); + assert_eq!(plain.to_bytes().unwrap(), data, "seed {seed}"); + } +} + +/// One wire against independently computed values, in case both paths are wrong. +#[test] +fn known_wire_yields_hand_computed_fields() { + let data = [0xABu8, 0xCD, 0xEF, 0x12, 0x34, 0x56]; + let (_, h) = Batched::from_bytes((&data, 0)).unwrap(); + assert_eq!( + h, + Batched { + version: 5, + id: 3021, + seq_flags: 3, + seq_count: 12050, + length: 13398, + } + ); + assert_eq!(h.to_bytes().unwrap(), data); +} + +/// Composing fields into one integer must not let a too-wide value collide with +/// its neighbour. +#[test] +fn an_oversized_field_is_still_rejected() { + let bad = Batched { + version: 0b111, // fits + id: 0xFFFF, // 16 bits into a 13-bit field + seq_flags: 0, + seq_count: 0, + length: 0, + }; + let err = bad.to_bytes().expect_err("13-bit field cannot hold 0xFFFF"); + let msg = format!("{err:?}"); + assert!( + msg.contains("bit size of input is larger than bit requested size"), + "unexpected message: {msg}" + ); + + // The unbatched path rejects it identically. + let plain = Unbatched { + version: 0b111, + id: 0xFFFF, + seq_flags: 0, + seq_count: 0, + length: 0, + }; + let plain_err = plain.to_bytes().expect_err("same field, same rejection"); + assert_eq!(format!("{plain_err:?}"), msg); +} + +/// The boundary, not just the overflow. +#[test] +fn each_field_accepts_its_widest_value() { + let full = Batched { + version: 0b111, + id: 0x1FFF, + seq_flags: 0b11, + seq_count: 0x3FFF, + length: 0xFFFF, + }; + let bytes = full.to_bytes().unwrap(); + assert_eq!(bytes, vec![0xFF; 6]); + let (_, back) = Batched::from_bytes((&bytes, 0)).unwrap(); + assert_eq!(back, full); +} + +/// Enum variants, named and unnamed: a different field-ident path. +#[test] +fn runs_inside_enum_variants() { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(id_type = "u8", endian = "big")] + enum Message { + #[deku(id = 1)] + Named { + #[deku(bits = 2)] + a: u8, + #[deku(bits = 6)] + b: u8, + }, + #[deku(id = 2)] + Unnamed(#[deku(bits = 4)] u8, #[deku(bits = 12)] u16), + } + + let data = [1u8, 0b11_010101]; + let (_, m) = Message::from_bytes((&data, 0)).unwrap(); + assert_eq!( + m, + Message::Named { + a: 0b11, + b: 0b010101 + } + ); + assert_eq!(m.to_bytes().unwrap(), data); + + let data = [2u8, 0xAB, 0xCD]; + let (_, m) = Message::from_bytes((&data, 0)).unwrap(); + assert_eq!(m, Message::Unnamed(0xA, 0xBCD)); + assert_eq!(m.to_bytes().unwrap(), data); +} + +/// A tuple struct: same planning, different ident path. +#[test] +fn run_in_a_tuple_struct() { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big")] + struct Packed(#[deku(bits = 3)] u8, #[deku(bits = 13)] u16); + + let data = [0xABu8, 0xCD]; + let (_, p) = Packed::from_bytes((&data, 0)).unwrap(); + assert_eq!(p, Packed(0b101, 0x0BCD)); + assert_eq!(p.to_bytes().unwrap(), data); +} + +/// Runs either side of an ineligible field. +#[test] +fn runs_around_ineligible_fields() { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big")] + struct Mixed { + #[deku(bits = 4)] + a: u8, + #[deku(bits = 4)] + b: u8, + #[deku(endian = "little")] + middle: u16, + #[deku(bits = 4)] + c: u8, + #[deku(bits = 4)] + d: u8, + } + + let data = [0x12u8, 0x34, 0x56, 0x78]; + let (_, m) = Mixed::from_bytes((&data, 0)).unwrap(); + assert_eq!( + m, + Mixed { + a: 0x1, + b: 0x2, + middle: 0x5634, // little-endian, so the bytes swap + c: 0x7, + d: 0x8, + } + ); + assert_eq!(m.to_bytes().unwrap(), data); +} + +/// One read for 48 bits must not succeed on fewer. +#[test] +fn a_short_wire_still_errors() { + for len in 0..6 { + let data = wire(len, 5); + assert!( + Batched::from_bytes((&data, 0)).is_err(), + "{len} bytes should not satisfy a 48-bit header" + ); + } + assert!(Batched::from_bytes((&wire(6, 5), 0)).is_ok()); +} + +/// `check_bit_size` preserves the per-field rejection, so pin its boundaries. +#[test] +fn check_bit_size_boundaries() { + use deku::writer::check_bit_size; + + // Widest value that fits. + assert!(check_bit_size::(0b11, 2).is_ok()); + assert!(check_bit_size::(0xFF, 8).is_ok()); + assert!(check_bit_size::(0, 1).is_ok()); + // A full-width request cannot overflow, so it never errors. + assert!(check_bit_size::(u64::MAX, 64).is_ok()); + + // One bit too wide, and the message names both widths. + let err = check_bit_size::(0b100, 2).expect_err("3 bits do not fit in 2"); + let msg = format!("{err:?}"); + assert!( + msg.contains("bit size of input is larger than bit requested size"), + "unexpected message: {msg}" + ); + // Only `descriptive-errors` appends the two widths. + #[cfg(feature = "descriptive-errors")] + { + assert!(msg.contains('3') && msg.contains('2'), "message: {msg}"); + let err = check_bit_size::(0x100, 8).expect_err("9 bits do not fit in 8"); + assert!(format!("{err:?}").contains('9')); + } + + // Same verdict either way; only the wording differs. + for (value, bits) in [(0b11u64, 2), (0xFF, 8), (0, 1), (u64::MAX, 64), (0b100, 2)] { + assert_eq!( + check_bit_size::(value, bits).is_ok(), + check_bit_size::(value, bits).is_ok(), + "{value:#x} in {bits} bits" + ); + } + let ordered = format!("{:?}", check_bit_size::(0b100, 2).unwrap_err()); + assert!( + ordered.contains("bit size of input is larger than requested size") + && !ordered.contains("bit requested size"), + "unexpected message: {ordered}" + ); +} + +/// Flags as bools, shaped like the RFC 3550 fixed header: 64 bits, three bools. +macro_rules! flags_header { + ($name:ident, $($extra:tt)*) => { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big")] + struct $name { + #[deku(bits = 2 $($extra)*)] + version: u8, + #[deku(bits = 1 $($extra)*)] + padding: bool, + #[deku(bits = 1 $($extra)*)] + extension: bool, + #[deku(bits = 4 $($extra)*)] + csrc_count: u8, + #[deku(bits = 1 $($extra)*)] + marker: bool, + #[deku(bits = 7 $($extra)*)] + payload_type: u8, + #[deku(bits = 16 $($extra)*)] + sequence_number: u16, + #[deku(bits = 32 $($extra)*)] + timestamp: u32, + } + }; +} + +flags_header!(FlagsBatched,); +flags_header!(FlagsUnbatched, , assert = "true"); + +/// A one-bit bool has no invalid value, so every wire must agree with unbatched. +#[test] +fn a_header_of_flags_matches_unbatched() { + for seed in 1..20_000u32 { + let data = wire(8, seed); + + let (_, b) = FlagsBatched::from_bytes((&data, 0)).unwrap(); + let (_, p) = FlagsUnbatched::from_bytes((&data, 0)).unwrap(); + + assert_eq!(b.version, p.version, "version, seed {seed}"); + assert_eq!(b.padding, p.padding, "padding, seed {seed}"); + assert_eq!(b.extension, p.extension, "extension, seed {seed}"); + assert_eq!(b.csrc_count, p.csrc_count, "csrc_count, seed {seed}"); + assert_eq!(b.marker, p.marker, "marker, seed {seed}"); + assert_eq!(b.payload_type, p.payload_type, "payload_type, seed {seed}"); + assert_eq!( + b.sequence_number, p.sequence_number, + "sequence_number, seed {seed}" + ); + assert_eq!(b.timestamp, p.timestamp, "timestamp, seed {seed}"); + + assert_eq!(b.to_bytes().unwrap(), data, "seed {seed}"); + assert_eq!(p.to_bytes().unwrap(), data, "seed {seed}"); + } +} + +/// Both flag states, written back exactly. +#[test] +fn flags_round_trip_in_both_states() { + let all_set = [0xFFu8; 8]; + let (_, b) = FlagsBatched::from_bytes((&all_set, 0)).unwrap(); + assert!(b.padding && b.extension && b.marker); + assert_eq!(b.to_bytes().unwrap(), all_set); + + let none_set = [0x00u8; 8]; + let (_, b) = FlagsBatched::from_bytes((&none_set, 0)).unwrap(); + assert!(!b.padding && !b.extension && !b.marker); + assert_eq!(b.to_bytes().unwrap(), none_set); +} + +/// A bool without `bits` is a byte, so it has invalid values to reject. +#[test] +fn a_byte_wide_bool_in_a_run_rejects_a_non_boolean_value() { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big")] + struct Batched { + flag: bool, + other: u8, + } + + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big")] + struct Unbatched { + #[deku(assert = "true")] + flag: bool, + other: u8, + } + + // 0 and 1 are the only accepted encodings, and round-trip. + for (byte, expected) in [(0x00u8, false), (0x01, true)] { + let data = [byte, 0x42]; + let (_, b) = Batched::from_bytes((&data, 0)).unwrap(); + assert_eq!( + b, + Batched { + flag: expected, + other: 0x42 + } + ); + assert_eq!(b.to_bytes().unwrap(), data); + } + + // Anything else fails, with the same error the unbatched path gives. + for byte in [0x02u8, 0x7F, 0xFF] { + let data = [byte, 0x42]; + let b = Batched::from_bytes((&data, 0)).expect_err("not a bool"); + let p = Unbatched::from_bytes((&data, 0)).expect_err("not a bool"); + assert_eq!(format!("{b:?}"), format!("{p:?}"), "byte {byte:#04x}"); + assert!( + format!("{b:?}").contains("cannot parse bool value"), + "unexpected message: {b}" + ); + } +} + +/// `bit_order = "msb"` is the default spelled out, so it must batch. It takes the +/// `Order`-carrying impl, whose overflow wording the batched write must follow. +#[test] +fn an_explicit_msb_bit_order_batches() { + macro_rules! ordered_header { + ($name:ident, $($extra:tt)*) => { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big")] + struct $name { + #[deku(bits = 3, bit_order = "msb" $($extra)*)] + a: u8, + #[deku(bits = 13, bit_order = "msb" $($extra)*)] + b: u16, + } + }; + } + + ordered_header!(OrderedBatched,); + ordered_header!(OrderedUnbatched, , assert = "true"); + + for seed in 1..2_000u32 { + let data = wire(2, seed); + let (_, x) = OrderedBatched::from_bytes((&data, 0)).unwrap(); + let (_, y) = OrderedUnbatched::from_bytes((&data, 0)).unwrap(); + assert_eq!((x.a, x.b), (y.a, y.b), "seed {seed}"); + assert_eq!(x.to_bytes().unwrap(), data, "seed {seed}"); + } + + let batched = format!( + "{:?}", + OrderedBatched { a: 0, b: 0xFFFF }.to_bytes().unwrap_err() + ); + let unbatched = format!( + "{:?}", + OrderedUnbatched { a: 0, b: 0xFFFF }.to_bytes().unwrap_err() + ); + assert_eq!(batched, unbatched); + assert!( + batched.contains("bit size of input is larger than requested size"), + "unexpected message: {batched}" + ); + assert!( + !batched.contains("bit requested size"), + "used the default impl's wording: {batched}" + ); +} + +/// A run may mix the two, and each field keeps its own wording. +#[test] +fn a_mixed_run_keeps_each_fields_wording() { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big")] + struct Mixed { + #[deku(bits = 4, bit_order = "msb")] + ordered: u8, + #[deku(bits = 4)] + plain: u8, + } + + // Both fields still share one read and one write. + let data = [0x12u8]; + let (_, m) = Mixed::from_bytes((&data, 0)).unwrap(); + assert_eq!( + m, + Mixed { + ordered: 0x1, + plain: 0x2 + } + ); + assert_eq!(m.to_bytes().unwrap(), data); + + let ordered_err = format!( + "{:?}", + Mixed { + ordered: 0xFF, + plain: 0 + } + .to_bytes() + .unwrap_err() + ); + assert!( + ordered_err.contains("bit size of input is larger than requested size") + && !ordered_err.contains("bit requested size"), + "unexpected message: {ordered_err}" + ); + + let plain_err = format!( + "{:?}", + Mixed { + ordered: 0, + plain: 0xFF + } + .to_bytes() + .unwrap_err() + ); + assert!( + plain_err.contains("bit size of input is larger than bit requested size"), + "unexpected message: {plain_err}" + ); +} + +/// A struct written `Msb0` nested after one written `Lsb0`. The parent leaves the +/// writer's order flag stale, and a batched write must not reorder the child's +/// bytes because of it. Mirrors the `bit_order` example in `attributes.rs`. +#[test] +fn a_run_after_an_lsb0_parent() { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big", bit_order = "lsb")] + struct Parent { + #[deku(bits = 13)] + offset: u16, + #[deku(bits = 3)] + t: u8, + child: Child, + } + + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big", ctx = "_: deku::ctx::Endian, _: deku::ctx::Order")] + struct Child { + field_a: u8, + #[deku(bits = 1)] + flag: bool, + #[deku(bits = 7)] + field_b: u8, + } + + let data = [0x10u8, 0x81, 0xAB, 0b1001_0110]; + let (_, p) = Parent::from_bytes((&data, 0)).unwrap(); + assert_eq!(p.child.field_a, 0xAB); + assert!(p.child.flag); + assert_eq!(p.child.field_b, 0b001_0110); + assert_eq!(p.to_bytes().unwrap(), data); +} + +// --------------------------------------------------------------------------- +// Equivalence audit: places where a batched path could diverge from the +// per-field one it replaces. Each pairs a batched struct with an `assert`- +// disqualified twin and compares everything observable. +// --------------------------------------------------------------------------- + +/// `from_bytes` reports the trailing slice and bit offset from `bits_read`, so a +/// run must account for exactly the bits its fields would have. +#[test] +fn from_bytes_reports_the_same_rest_and_offset() { + macro_rules! h { + ($name:ident, $($extra:tt)*) => { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big")] + struct $name { + #[deku(bits = 3 $($extra)*)] + a: u8, + #[deku(bits = 7 $($extra)*)] + b: u8, + } + }; + } + h!(RestBatched,); + h!(RestUnbatched, , assert = "true"); + + let data = [0xABu8, 0xCD, 0xEF]; + let ((rb, ob), _) = RestBatched::from_bytes((&data, 0)).unwrap(); + let ((ru, ou), _) = RestUnbatched::from_bytes((&data, 0)).unwrap(); + assert_eq!((rb, ob), (ru, ou)); + // 10 bits consumed: two bits into the second byte. + assert_eq!(ob, 2); + assert_eq!(rb, &data[1..]); +} + +/// A run that starts part way through a byte, so the batched read splices onto a +/// partial leftover rather than starting aligned. +#[test] +fn a_run_starting_mid_byte() { + macro_rules! h { + ($name:ident, $($extra:tt)*) => { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big")] + struct $name { + // Disqualified, so the run starts at bit 3. + #[deku(bits = 3, assert = "true")] + head: u8, + #[deku(bits = 5 $($extra)*)] + a: u8, + #[deku(bits = 8 $($extra)*)] + b: u8, + } + }; + } + h!(MidBatched,); + h!(MidUnbatched, , assert = "true"); + + for seed in 1..2_000u32 { + let data = wire(2, seed); + let (_, x) = MidBatched::from_bytes((&data, 0)).unwrap(); + let (_, y) = MidUnbatched::from_bytes((&data, 0)).unwrap(); + assert_eq!((x.head, x.a, x.b), (y.head, y.a, y.b), "seed {seed}"); + assert_eq!(x.to_bytes().unwrap(), data, "seed {seed}"); + } +} + +/// Reading from a non-zero starting bit offset, which `from_bytes` reaches via +/// `skip_bits` before the first field. +#[test] +fn a_run_at_every_starting_bit_offset() { + macro_rules! h { + ($name:ident, $($extra:tt)*) => { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big")] + struct $name { + #[deku(bits = 5 $($extra)*)] + a: u8, + #[deku(bits = 3 $($extra)*)] + b: u8, + } + }; + } + h!(OffBatched,); + h!(OffUnbatched, , assert = "true"); + + for off in 0..8usize { + for seed in 1..200u32 { + let data = wire(3, seed); + let (_, x) = OffBatched::from_bytes((&data, off)).unwrap(); + let (_, y) = OffUnbatched::from_bytes((&data, off)).unwrap(); + assert_eq!((x.a, x.b), (y.a, y.b), "offset {off}, seed {seed}"); + } + } +} + +/// A bit-packed enum id leaves a partial byte before the variant's run. +#[test] +fn a_run_after_a_bit_packed_enum_id() { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(id_type = "u8", bits = 4, endian = "big")] + enum Batched { + #[deku(id = 1)] + A { + #[deku(bits = 4)] + a: u8, + #[deku(bits = 8)] + b: u8, + }, + } + + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(id_type = "u8", bits = 4, endian = "big")] + enum Unbatched { + #[deku(id = 1)] + A { + #[deku(bits = 4, assert = "true")] + a: u8, + #[deku(bits = 8, assert = "true")] + b: u8, + }, + } + + for seed in 1..500u32 { + let mut data = wire(2, seed); + data[0] = 0x10 | (data[0] & 0x0F); // id = 1 + let (_, x) = Batched::from_bytes((&data, 0)).unwrap(); + let (_, y) = Unbatched::from_bytes((&data, 0)).unwrap(); + let (Batched::A { a: xa, b: xb }, Unbatched::A { a: ya, b: yb }) = (&x, &y); + assert_eq!((xa, xb), (ya, yb), "seed {seed}"); + assert_eq!(x.to_bytes().unwrap(), data, "seed {seed}"); + } +} + +/// `deku::byte_offset` on a field after a run must see the bits the run consumed. +#[test] +fn byte_offset_after_a_run_is_correct() { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big")] + struct S { + a: u8, + b: u16, + // Three bytes consumed by the run above. + #[deku(assert = "deku::byte_offset == 3")] + tail: u8, + } + + let data = [1u8, 2, 3, 4]; + let (_, s) = S::from_bytes((&data, 0)).unwrap(); + assert_eq!( + s, + S { + a: 1, + b: 0x0203, + tail: 4 + } + ); + assert_eq!(s.to_bytes().unwrap(), data); +} + +/// A partial `Lsb0` leftover before a run: one batched write would reorder across +/// bytes, so the derive falls back to a write per field. Must be byte-identical to +/// the per-field path it replaces. +#[test] +fn a_run_after_a_partial_lsb0_leftover() { + macro_rules! kid { + ($name:ident, $($extra:tt)*) => { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big", ctx = "_: deku::ctx::Endian, _: deku::ctx::Order")] + struct $name { + #[deku(bits = 6 $($extra)*)] + a: u8, + #[deku(bits = 7 $($extra)*)] + b: u8, + #[deku(bits = 3 $($extra)*)] + c: u8, + } + }; + } + kid!(KidBatched,); + kid!(KidPlain, , assert = "true"); + + macro_rules! parent { + ($name:ident, $kid:ident) => { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big", bit_order = "lsb")] + struct $name { + // Three bits, so the child starts mid-byte with an `Lsb0` leftover. + #[deku(bits = 3)] + x: u8, + child: $kid, + } + }; + } + parent!(PBatched, KidBatched); + parent!(PPlain, KidPlain); + + for seed in 1..3_000u32 { + let data = wire(3, seed); + let (_, b) = PBatched::from_bytes((&data, 0)).unwrap(); + let (_, p) = PPlain::from_bytes((&data, 0)).unwrap(); + assert_eq!( + (b.x, b.child.a, b.child.b, b.child.c), + (p.x, p.child.a, p.child.b, p.child.c), + "read, seed {seed}" + ); + assert_eq!( + b.to_bytes().unwrap(), + p.to_bytes().unwrap(), + "write, seed {seed}" + ); + } +} + +/// A byte-aligned `Lsb0` parent leaves no bits pending, so both paths write the +/// child from a clean boundary and must agree. The child spans more than a byte, +/// which is where the `Lsb0` write path would reorder whole bytes. +#[test] +fn a_byte_aligned_lsb0_parent_before_a_run() { + macro_rules! kid { + ($name:ident, $($extra:tt)*) => { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big", ctx = "_: deku::ctx::Endian, _: deku::ctx::Order")] + struct $name { + #[deku(bits = 12 $($extra)*)] + a: u16, + #[deku(bits = 4 $($extra)*)] + b: u8, + } + }; + } + kid!(KidBatched,); + kid!(KidPlain, , assert = "true"); + + macro_rules! parent { + ($name:ident, $kid:ident) => { + #[derive(Debug, PartialEq, DekuRead, DekuWrite)] + #[deku(endian = "big", bit_order = "lsb")] + struct $name { + // Fills the byte, so the child starts aligned. + #[deku(bits = 8)] + x: u8, + child: $kid, + } + }; + } + parent!(PBatched, KidBatched); + parent!(PPlain, KidPlain); + + for seed in 1..3_000u32 { + let data = wire(3, seed); + let (_, b) = PBatched::from_bytes((&data, 0)).unwrap(); + let (_, p) = PPlain::from_bytes((&data, 0)).unwrap(); + assert_eq!( + (b.x, b.child.a, b.child.b), + (p.x, p.child.a, p.child.b), + "read, seed {seed}" + ); + let (bb, pb) = (b.to_bytes().unwrap(), p.to_bytes().unwrap()); + assert_eq!(bb, pb, "write, seed {seed}"); + // Neither path may reorder the child's bytes, so both round-trip. + assert_eq!(bb, data, "round-trip, seed {seed}"); + } +}