From a239dfc09bf16c12383c2661d54a62d923e6c984 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Wed, 1 Jul 2026 12:03:05 -0600 Subject: [PATCH 1/7] =?UTF-8?q?Fix=20inline-constant=20packing=20for=20val?= =?UTF-8?q?ues=20=E2=89=A5=2016=20bits?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The inline codegen path packed each element at an 8-bit stride and sized the constant as len*8 bits, corrupting wide values: when unit_bits >= 8, `data` holds unit_bits-wide elements, not bytes. Pack at `elem_bits` (= unit_bits when >= 8, else 8) to match the extraction stride, and cap the const-type shift at 32 to avoid a `1 << 63` overflow. Ports F2 from harfbuzz/packtab#9. Co-Authored-By: Claude Opus 4.8 (1M context) --- packtab/src/codegen.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/packtab/src/codegen.rs b/packtab/src/codegen.rs index 930c91b..4945114 100644 --- a/packtab/src/codegen.rs +++ b/packtab/src/codegen.rs @@ -324,7 +324,13 @@ fn gen_inner_code( data = combine(data, layer.unit_bits); // Check if we can inline as a constant. - let can_inline = data.len() * 8 <= 64 && data.iter().all(|&v| v >= 0); + // + // `data` holds packed bytes when unit_bits < 8 (see `combine`), but holds + // unit_bits-wide elements when unit_bits >= 8. Inlining packs each element at + // `elem_bits` stride and must match the extraction stride below, so use the + // element width — not a hardcoded 8 — for both the fit check and the packing. + let elem_bits = if unit_bits >= 8 { unit_bits as usize } else { 8 }; + let can_inline = data.len() * elem_bits <= 64 && data.iter().all(|&v| v >= 0); let arr_name: String; let start: usize; @@ -369,10 +375,12 @@ fn gen_inner_code( if can_inline { let mut packed: u64 = 0; for (i, &b) in data.iter().enumerate() { - packed |= (b as u64) << (i * 8); + packed |= (b as u64) << (i * elem_bits); } - let total_bits = data.len() * 8; - let const_typ = if total_bits >= 64 { + let total_bits = data.len() * elem_bits; + // Any total_bits > 32 needs U64; capping the shift at 32 also avoids the + // `1i64 << total_bits` overflow that a 63-bit constant would otherwise hit. + let const_typ = if total_bits > 32 { IntType::U64 } else { IntType::for_range(0, (1i64 << total_bits) - 1) From e10d0ce0c5b9033141625bceffea8c6926963af4 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Wed, 1 Jul 2026 12:03:15 -0600 Subject: [PATCH 2/7] Widen return type to hold the default value The lookup return type was sized from the stored data range only, but `default` is emitted in the out-of-range branch and returned for culled default-prefix indices. A negative or oversized default would wrap (C) or fail to compile (Rust). Widen the range to include `default` in both the direct and palette codegen paths. Ports F3 from harfbuzz/packtab#9. Co-Authored-By: Claude Opus 4.8 (1M context) --- packtab/src/codegen.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/packtab/src/codegen.rs b/packtab/src/codegen.rs index 4945114..d021b4f 100644 --- a/packtab/src/codegen.rs +++ b/packtab/src/codegen.rs @@ -468,7 +468,13 @@ fn gen_outer_code( let input_var = var; let var = if name.is_some() { "u" } else { var }; - let typ = IntType::for_range(outer_info.min_v, outer_info.max_v); + // The return type must also hold `default` — it is emitted in the out-of-range + // branch and returned for culled default-prefix indices, so a negative or + // oversized default would otherwise wrap (C) or fail to compile (Rust) even + // though the stored data fits a narrower type. + let lo = outer_info.min_v.min(outer_info.default); + let hi = outer_info.max_v.max(outer_info.default); + let typ = IntType::for_range(lo, hi); let ret_type = typ; let lookup_var = if outer_info.base > 0 { wrapping_sub(var, &usize_literal(outer_info.base, lang), lang) @@ -541,7 +547,12 @@ fn gen_palette_outer_code( let input_var = var; let var = if name.is_some() { "u" } else { var }; - let typ = IntType::for_range(outer_info.min_v, outer_info.max_v); + // The return type must also hold `default` (returned out-of-range / for culled + // default-prefix indices), so widen the range to include it — same as the + // direct path in `gen_outer_code`. + let lo = outer_info.min_v.min(outer_info.default); + let hi = outer_info.max_v.max(outer_info.default); + let typ = IntType::for_range(lo, hi); let ret_type = typ; let lookup_var = if outer_info.base > 0 { wrapping_sub(var, &usize_literal(outer_info.base, lang), lang) From f595b132033b0bc529b72427b2cd3ce2f695dc2b Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Wed, 1 Jul 2026 12:03:29 -0600 Subject: [PATCH 3/7] Honor palette array start offset in palette lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The palette lookup indexed the palette array from zero, discarding the start offset returned by add_array. When a palette array is shared across solutions in one CodeBuilder, later solutions must index from their offset — the same as the data-array path already does. Latent through the public API (each generate() uses a fresh CodeBuilder), fixed for parity and defensiveness. Ports F4 from harfbuzz/packtab#9. Co-Authored-By: Claude Opus 4.8 (1M context) --- packtab/src/codegen.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packtab/src/codegen.rs b/packtab/src/codegen.rs index d021b4f..6ac4011 100644 --- a/packtab/src/codegen.rs +++ b/packtab/src/codegen.rs @@ -565,7 +565,7 @@ fn gen_palette_outer_code( let pal_min = *palette.iter().min().unwrap(); let pal_max = *palette.iter().max().unwrap(); let palette_typ = IntType::for_range(pal_min, pal_max); - let (palette_name, _) = code_builder.add_array(palette_typ, "palette", palette); + let (palette_name, palette_start) = code_builder.add_array(palette_typ, "palette", palette); // Generate the index lookup expression from the palette inner chain. let palette_inner = outer_info.palette_inner.as_ref().unwrap(); @@ -579,7 +579,15 @@ fn gen_palette_outer_code( ); // Cast index to usize (required in Rust; no-op in C) and look up palette. + // The palette array may be shared across solutions in one CodeBuilder, so honor + // the start offset returned by add_array (as the data-array path does) rather + // than indexing from zero. let index_usize = as_usize(&index_expr, lang); + let index_usize = if palette_start > 0 { + format!("{}+{}", usize_literal(palette_start, lang), index_usize) + } else { + index_usize + }; let mut expr = array_index(&palette_name, &index_usize, lang); expr = cast(&expr, ret_type, lang); From c6da0404fdc3a20cfaabeed1ee5eb8ca5778f43b Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Wed, 1 Jul 2026 12:03:51 -0600 Subject: [PATCH 4/7] Keep the cost frontier when pruning, not just full-cost Pareto pruning kept only the (n_lookups, full_cost) frontier, discarding byte-minimal solutions before they reached the root. This left the compression>=10 (min bytes) and compression<=0 (flat) selectors unable to reach the true optimum. Prune to the union of the (n_lookups, full_cost) and (n_lookups, cost) frontiers via a shared dual_frontier_indices helper, applied at every pruning site (inner intermediate/root layers and the outer direct+palette set). The size/speed heuristic (1..9) is unaffected: the extra cost-frontier solutions are always full_cost-dominated, and ties order the full_cost incumbent first. Existing tests that asserted the old single-frontier invariant are updated to the union check. Ports F1 from harfbuzz/packtab#9. Co-Authored-By: Claude Opus 4.8 (1M context) --- packtab/src/layer.rs | 200 +++++++++++++++++++++++-------------------- packtab/src/tests.rs | 34 +++++--- 2 files changed, 132 insertions(+), 102 deletions(-) diff --git a/packtab/src/layer.rs b/packtab/src/layer.rs index 9e33040..d4584a9 100644 --- a/packtab/src/layer.rs +++ b/packtab/src/layer.rs @@ -253,24 +253,20 @@ impl InnerLayerChain { } } - /// Pareto-prune a list of solution indices. + /// Pareto-prune a list of solution indices, keeping the union of the + /// `(n_lookups, full_cost)` and `(n_lookups, cost)` frontiers. /// - /// Returns the subset of `indices` that are non-dominated: sorted by - /// (n_lookups, full_cost) ascending, keeping only solutions whose - /// full_cost strictly improves on all previously kept solutions. + /// Retaining the cost frontier here (not just full_cost) is what lets a + /// byte-minimal solution survive up through the parent layers to the root, so + /// the compression >= 10 selector can reach the true minimum bytes. Keeping + /// more solutions is always safe: the heuristic never prefers the extra ones. fn pareto_prune_indices(solutions: &[InnerSolution], indices: &[usize]) -> Vec { - let mut sorted = indices.to_vec(); - sorted.sort_by_key(|&i| (solutions[i].n_lookups, solutions[i].full_cost())); - let mut kept = Vec::new(); - let mut best_cost = usize::MAX; - for i in sorted { - let fc = solutions[i].full_cost(); - if fc < best_cost { - kept.push(i); - best_cost = fc; - } - } - kept + dual_frontier_indices( + indices, + |i| solutions[i].n_lookups, + |i| solutions[i].cost, + |i| solutions[i].full_cost(), + ) } fn prune_solutions(&mut self) { @@ -290,24 +286,15 @@ impl InnerLayerChain { .map(|(i, _)| i) .collect(); - // Pareto pruning: sort by (nLookups, fullCost), keep non-dominated. - let mut indexed: Vec<(usize, usize, usize)> = root_solutions - .iter() - .map(|&i| { - let s = &self.solutions[i]; - (i, s.n_lookups, s.full_cost()) - }) - .collect(); - indexed.sort_by_key(|&(_, nl, fc)| (nl, fc)); - - let mut kept_indices = Vec::new(); - let mut best_cost = usize::MAX; - for (idx, _, fc) in indexed { - if fc < best_cost { - kept_indices.push(idx); - best_cost = fc; - } - } + // Pareto pruning: keep the union of the (nLookups, fullCost) and + // (nLookups, cost) frontiers so byte-minimal solutions survive for the + // compression >= 10 selector. + let kept_indices = dual_frontier_indices( + &root_solutions, + |i| self.solutions[i].n_lookups, + |i| self.solutions[i].cost, + |i| self.solutions[i].full_cost(), + ); // Mark which solutions are kept (need to keep all referenced children too). let mut keep = vec![false; self.solutions.len()]; @@ -418,20 +405,60 @@ impl AnyOuterSolution { } } -pub(crate) fn prune_pareto_solutions(mut solutions: Vec) -> Vec { - solutions.sort_by_key(|s| (s.n_lookups(), s.full_cost())); - let mut kept = Vec::new(); - let mut best_cost = usize::MAX; - for solution in solutions { - let full_cost = solution.full_cost(); - if full_cost < best_cost { - kept.push(solution); - best_cost = full_cost; +/// Keep the union of the `(n_lookups, full_cost)` frontier and the +/// `(n_lookups, cost)` frontier over `candidates`, returning kept indices. +/// +/// The size/speed heuristic (compression 1..9) navigates the full_cost frontier +/// and is unaffected: the extra cost-frontier solutions are always full_cost- +/// dominated, so the monotone heuristic score can never prefer them, and on an +/// exact tie the full_cost-frontier incumbent is ordered first. Retaining the +/// cost frontier lets the byte-minimizing (compression >= 10) and flat +/// (compression <= 0) selectors reach the true optimum, which pruning on +/// full_cost alone would discard. +fn dual_frontier_indices( + candidates: &[usize], + n_lookups: impl Fn(usize) -> usize, + cost: impl Fn(usize) -> usize, + full_cost: impl Fn(usize) -> usize, +) -> Vec { + let mut seen = std::collections::HashSet::new(); + let mut kept: Vec = Vec::new(); + + // Pass 0: full_cost frontier (incumbents). Pass 1: cost frontier. + for pass in 0..2 { + let key = |i: usize| if pass == 0 { full_cost(i) } else { cost(i) }; + let mut order = candidates.to_vec(); + order.sort_by_key(|&i| (n_lookups(i), key(i))); + let mut best = usize::MAX; + for i in order { + let k = key(i); + if k < best { + best = k; + if seen.insert(i) { + kept.push(i); + } + } } } + + // Stable sort keeps full_cost incumbents ahead of any cost-only tie. + kept.sort_by_key(|&i| (n_lookups(i), full_cost(i))); kept } +pub(crate) fn prune_pareto_solutions(solutions: Vec) -> Vec { + let candidates: Vec = (0..solutions.len()).collect(); + let kept = dual_frontier_indices( + &candidates, + |i| solutions[i].n_lookups(), + |i| solutions[i].cost(), + |i| solutions[i].full_cost(), + ); + // Materialize kept solutions in frontier order (indices are unique). + let mut owned: Vec> = solutions.into_iter().map(Some).collect(); + kept.into_iter().map(|i| owned[i].take().unwrap()).collect() +} + /// Arithmetic preprocessing result. #[derive(Debug)] pub struct OuterLayerInfo { @@ -609,6 +636,11 @@ impl OuterLayerInfo { try_palette_encoding(&reduced_data, extra_ops); solutions.extend(palette_sols); + // Prune the combined (direct + palette) set to the union frontier, dropping + // solutions that are redundant — dominated on both full_cost and cost — such + // as an inner split beaten outright by a palette solution. + let solutions = prune_pareto_solutions(solutions); + OuterLayerInfo { data, default, @@ -750,23 +782,35 @@ mod tests { assert_eq!(lookups, sorted); } + /// Every root solution must be minimal on the full_cost OR the cost axis among + /// peers with `n_lookups <= its own` — the dual frontier the pruner keeps. What + /// must never survive is a fully redundant solution: dominated on both axes. + fn assert_on_union_frontier(chain: &InnerLayerChain) { + let root_idxs = chain.root_solutions(); + for &s in &root_idxs { + let ns = chain.solutions[s].n_lookups; + let peers: Vec<&InnerSolution> = root_idxs + .iter() + .map(|&i| &chain.solutions[i]) + .filter(|t| t.n_lookups <= ns) + .collect(); + let on_fullcost = + chain.solutions[s].full_cost() <= peers.iter().map(|t| t.full_cost()).min().unwrap(); + let on_cost = chain.solutions[s].cost <= peers.iter().map(|t| t.cost).min().unwrap(); + assert!( + on_fullcost || on_cost, + "solution (nl={}, fc={}, cost={}) is redundant (dominated on both axes)", + ns, + chain.solutions[s].full_cost(), + chain.solutions[s].cost, + ); + } + } + #[test] fn test_inner_solutions_not_dominated() { let chain = InnerLayerChain::new((0..256).map(|x| x as i64).collect()); - let root_idxs = chain.root_solutions(); - for &a in &root_idxs { - for &b in &root_idxs { - if a == b { - continue; - } - let sa = &chain.solutions[a]; - let sb = &chain.solutions[b]; - assert!( - !(sa.n_lookups <= sb.n_lookups && sa.full_cost() <= sb.full_cost()), - "Found dominated solution" - ); - } - } + assert_on_union_frontier(&chain); } #[test] @@ -859,28 +903,11 @@ mod tests { ); } - /// All root solutions for a deep chain must be Pareto-optimal. + /// All root solutions for a deep chain must lie on the union frontier. #[test] fn test_inner_deep_chain_pareto_optimal() { let chain = InnerLayerChain::new(deep_chain_data()); - let root_idxs = chain.root_solutions(); - for &a in &root_idxs { - for &b in &root_idxs { - if a == b { - continue; - } - let sa = &chain.solutions[a]; - let sb = &chain.solutions[b]; - assert!( - !(sa.n_lookups <= sb.n_lookups && sa.full_cost() <= sb.full_cost()), - "Solution (nl={}, fc={}) dominates (nl={}, fc={})", - sa.n_lookups, - sa.full_cost(), - sb.n_lookups, - sb.full_cost() - ); - } - } + assert_on_union_frontier(&chain); } /// Root solutions must be sorted by n_lookups ascending. @@ -897,23 +924,14 @@ mod tests { assert_eq!(lookups, sorted, "Root solutions not sorted by n_lookups"); } - /// When sorted by n_lookups, full_cost must strictly decrease - /// (otherwise dominated solutions would have survived pruning). + /// No kept root solution may be redundant (dominated on both full_cost and + /// cost). With the dual frontier, two solutions can share a lookup count — one + /// full_cost-optimal, one cost-optimal — so full_cost no longer strictly + /// decreases across the sequence, but neither is ever fully dominated. #[test] - fn test_inner_deep_chain_cost_strictly_decreases() { + fn test_inner_deep_chain_no_redundant_solutions() { let chain = InnerLayerChain::new(deep_chain_data()); - let root_idxs = chain.root_solutions(); - let mut prev_cost = usize::MAX; - for &i in &root_idxs { - let fc = chain.solutions[i].full_cost(); - assert!( - fc < prev_cost, - "full_cost did not strictly decrease: {} >= {}", - fc, - prev_cost - ); - prev_cost = fc; - } + assert_on_union_frontier(&chain); } /// pick_solution must return a valid index and choose a compact solution. diff --git a/packtab/src/tests.rs b/packtab/src/tests.rs index d79b47e..59d887a 100644 --- a/packtab/src/tests.rs +++ b/packtab/src/tests.rs @@ -583,20 +583,32 @@ fn test_palette_cost_calculation() { #[test] fn test_palette_in_pareto_frontier() { - // All returned solutions must be mutually non-dominated. + // Every returned solution is minimal on the full_cost OR the cost axis. + // + // The frontier is the union of the (n_lookups, full_cost) frontier (navigated + // by compression 1..9) and the (n_lookups, cost) frontier (needed for + // compression >= 10 to reach the true minimum bytes). A solution may be + // full_cost-dominated yet still belong, provided it is cost-minimal for its + // lookup count; what must never happen is a fully redundant solution — + // dominated on both axes. let data = vec![1i64, 2, 3, 2, 3, 2, 1, 0, 2, 1, 2, 2, 3, 3, 1, 11110124]; let info = pack_table_all(&data, Some(0)); - for a in &info.solutions { - for b in &info.solutions { - if std::ptr::eq(a, b) { - continue; - } - assert!( - !(a.n_lookups() <= b.n_lookups() && a.full_cost() <= b.full_cost()), - "Found dominated solution in Pareto frontier" - ); - } + for s in &info.solutions { + let peers: Vec<_> = info + .solutions + .iter() + .filter(|t| t.n_lookups() <= s.n_lookups()) + .collect(); + let on_fullcost = s.full_cost() <= peers.iter().map(|t| t.full_cost()).min().unwrap(); + let on_cost = s.cost() <= peers.iter().map(|t| t.cost()).min().unwrap(); + assert!( + on_fullcost || on_cost, + "redundant solution (nl={}, fc={}, cost={})", + s.n_lookups(), + s.full_cost(), + s.cost() + ); } } From 992d7421bdd0670d1742afd1d465f55694e2f65f Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Wed, 1 Jul 2026 12:04:05 -0600 Subject: [PATCH 5/7] Add codegen-soundness regression tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-trip and frontier regressions covering the ported fixes: inline wide-value packing (F2), default-value type widening incl. negative/ oversized/culled-prefix defaults (F3), odd-length out-of-range → default (F5-analog; the inner chain never aliases the outer data in Rust), and compression>=10 reaching global min bytes while 1..9 picks stay unchanged (F1). Ports the test additions from harfbuzz/packtab#9. Co-Authored-By: Claude Opus 4.8 (1M context) --- packtab/src/tests.rs | 154 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 154 insertions(+) diff --git a/packtab/src/tests.rs b/packtab/src/tests.rs index 59d887a..8ace7dd 100644 --- a/packtab/src/tests.rs +++ b/packtab/src/tests.rs @@ -855,3 +855,157 @@ fn test_inferred_default_considers_both_boundary_values() { assert!(inferred_costs.is_subset(&candidate_costs)); assert!(!inferred_costs.is_empty()); } + +// ── Codegen soundness regressions (port of harfbuzz/packtab PR #9) ── +// +// Round-trip regressions for the codegen/pruning soundness bugs found by fuzzing +// the Python implementation. Each generates code at the default compression and +// compiles + runs it in C and both Rust flavours; compile_and_run also asserts +// out-of-range indices return `default`. + +fn roundtrip_all_langs(data: &[i64], default: i64) { + for lang in [ + Language::C, + Language::Rust { unsafe_access: false }, + Language::Rust { unsafe_access: true }, + ] { + let code = gen(data, default, lang); + compile_and_run(&code, data, default, lang); + } +} + +// ── F2: inline-constant path with >= 16-bit values ── +// A wide value packed into the inline path used to be corrupted because the +// packing assumed 8-bit elements. These pick the inline solution and must +// round-trip. +#[test] +fn test_inline_16bit_value_roundtrips() { + let data = [ + vec![0i64; 36], + vec![255], + vec![0i64; 55], + vec![65000, 0, 0, 255], + vec![0i64; 8], + ] + .concat(); + roundtrip_all_langs(&data, 0); +} + +#[test] +fn test_inline_various_wide_values_roundtrip() { + for data in [ + vec![0i64, 65000, 0, 300], + vec![0i64, 0, 70000, 0], // 32-bit + vec![5i64, 5, 5, 4000, 5, 5, 5, 5], + ] { + roundtrip_all_langs(&data, 0); + } +} + +// ── F3: return type must hold `default` ── +#[test] +fn test_negative_default_roundtrips() { + roundtrip_all_langs(&[0, 1, 2, 3], -1); +} + +#[test] +fn test_large_default_roundtrips() { + roundtrip_all_langs(&[0, 1, 2, 3], 1000); +} + +#[test] +fn test_default_outside_range_constant_data() { + roundtrip_all_langs(&[0; 8], -5); +} + +#[test] +fn test_negative_default_culls_prefix() { + // The leading default value is culled by base-rebasing; the culled index must + // still return the (correctly typed) default. + roundtrip_all_langs(&[-2, 1], -2); +} + +// ── F5-analog: odd-length data must not leak the split() padding ── +// Rust always builds the inner chain from a freshly-owned Vec (never aliased to +// the outer data), and the bounds check uses the unpadded outer length, so the +// padding can never leak. These guard that invariant. +#[test] +fn test_odd_length_out_of_range_returns_default() { + roundtrip_all_langs(&[1, 2, 3], 0); + roundtrip_all_langs(&[3, 1, 4, 1, 5], 0); +} + +#[test] +fn test_inner_chain_does_not_alias_outer_data() { + // OuterLayerInfo keeps its own `data`; the inner layer 0 owns a separate, + // possibly padded copy. Padding must not extend the outer bounds. + let info = OuterLayerInfo::new(&[1, 2, 3], 0); + assert_eq!(info.data, vec![1, 2, 3]); + assert!(info.inner.layers[0].data.len() >= info.data.len()); +} + +// ── F1: compression >= 10 must reach the true minimum-byte solution ── +#[test] +fn test_compression_10_is_global_min_bytes() { + let cases: Vec> = vec![ + [ + vec![0i64; 8], + vec![9999], + vec![0i64; 30], + vec![5], + vec![0i64; 60], + vec![1], + vec![0i64; 40], + ] + .concat(), + [vec![0i64; 200], vec![7], vec![0i64; 55]].concat(), + (0..400i64).map(|i| i % 4).collect(), + ]; + for data in cases { + let all = pack_table_all(&data, Some(0)); + let true_min = all.solutions.iter().map(|s| s.cost()).min().unwrap(); + let (info, best) = pack_table(&data, Some(0), 10.0); + assert_eq!( + info.solutions[best].cost(), + true_min, + "compression>=10 did not reach global min bytes for {:?}", + &data[..8.min(data.len())] + ); + } +} + +#[test] +fn test_compression_1to9_unchanged_by_frontier_enrichment() { + // The cost-frontier solutions re-admitted for compression >= 10 are always + // full_cost-dominated, so they must never change a 1..9 pick. + let data = [ + vec![0i64; 64], + vec![1, 2, 3, 0, 0, 5], + vec![0i64; 40], + vec![255], + vec![0i64; 20], + ] + .concat(); + let info = pack_table_all(&data, Some(0)); + for c in 1..10 { + let cf = c as f64; + let picked = pick_solution(&info.solutions, cf); + let best = info + .solutions + .iter() + .enumerate() + .min_by(|(_, a), (_, b)| { + let sa = a.n_lookups() as f64 + cf * (a.full_cost() as f64).log2(); + let sb = b.n_lookups() as f64 + cf * (b.full_cost() as f64).log2(); + sa.partial_cmp(&sb).unwrap() + }) + .map(|(i, _)| i) + .unwrap(); + assert_eq!( + (info.solutions[picked].cost(), info.solutions[picked].n_lookups()), + (info.solutions[best].cost(), info.solutions[best].n_lookups()), + "compression {} pick changed", + c + ); + } +} From 04ee7539e90bc47e2fe52f4d4530b7ee16d55bc7 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Wed, 1 Jul 2026 12:35:16 -0600 Subject: [PATCH 6/7] Drop the dead padding byte from odd-length flat tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit build_layers padded layer.data in place so the flat (unsplit) solution emitted odd-length tables with one unreachable trailing byte, while the cost — computed from the un-padded length — was one element short of the emission. Pad a local copy for pairing instead; layer.data keeps the original array, so the flat solution emits exactly len(data) elements and its cost matches. Split solutions still pair over the optimally-padded copy, so all costs and pick_solution choices are unchanged. Ports part of harfbuzz/packtab@310edfc4f. Co-Authored-By: Claude Opus 4.8 (1M context) --- packtab/src/layer.rs | 25 +++++++++++++++---------- packtab/src/tests.rs | 37 ++++++++++++++++++++++++++++++++++--- 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/packtab/src/layer.rs b/packtab/src/layer.rs index d4584a9..6dd9aac 100644 --- a/packtab/src/layer.rs +++ b/packtab/src/layer.rs @@ -62,19 +62,24 @@ impl InnerLayerChain { break; } - // Split: pad to even length, pair adjacent elements - // Smart padding: choose value that creates most common pair. - // The padded position is never accessed, so this is safe. - let cur = self.layers.last_mut().unwrap(); - if cur.data.len() & 1 != 0 { - let last_val = cur.data[cur.data.len() - 1]; - let padding = Self::choose_optimal_padding(&cur.data, last_val); - cur.data.push(padding); + // Split: pair adjacent elements. Pad a LOCAL copy to even length, + // leaving layer.data as the original array, so the flat (unsplit) + // solution emits exactly data.len() elements with no unreachable + // trailing byte (and its cost, computed from the un-padded length, + // matches the emission). Split solutions pair over the + // optimally-padded copy, so their child data — and thus all + // costs/picks — are unchanged. The padded position is never + // accessed, so the pad value is free. + let mut padded: Vec = self.layers.last().unwrap().data.clone(); + if padded.len() & 1 != 0 { + let last_val = padded[padded.len() - 1]; + let padding = Self::choose_optimal_padding(&padded, last_val); + padded.push(padding); } // Collect pairs with frequencies and first occurrence positions use std::collections::HashMap; - let pairs: Vec<(usize, usize)> = cur.data + let pairs: Vec<(usize, usize)> = padded .chunks(2) .map(|pair| (pair[0] as usize, pair[1] as usize)) .collect(); @@ -110,7 +115,7 @@ impl InnerLayerChain { let id = mapping.get_or_insert(pair); data2.push(id as i64); } - cur.mapping = Some(mapping); + self.layers.last_mut().unwrap().mapping = Some(mapping); data = data2; } } diff --git a/packtab/src/tests.rs b/packtab/src/tests.rs index 8ace7dd..5e561ab 100644 --- a/packtab/src/tests.rs +++ b/packtab/src/tests.rs @@ -937,11 +937,42 @@ fn test_odd_length_out_of_range_returns_default() { #[test] fn test_inner_chain_does_not_alias_outer_data() { - // OuterLayerInfo keeps its own `data`; the inner layer 0 owns a separate, - // possibly padded copy. Padding must not extend the outer bounds. + // OuterLayerInfo keeps its own `data`; the inner layer 0 owns a separate copy + // built from the reduced values. split() never pads layer.data in place, so the + // inner layer length always matches the outer length (no dead trailing byte). let info = OuterLayerInfo::new(&[1, 2, 3], 0); assert_eq!(info.data, vec![1, 2, 3]); - assert!(info.inner.layers[0].data.len() >= info.data.len()); + assert_eq!(info.inner.layers[0].data.len(), info.data.len()); +} + +// ── Odd-length flat tables must not carry the split() padding byte ── +// build_layers pads a local copy for pairing only; layer.data stays the original +// array, so the flat (unsplit) solution emits exactly len(data) elements. +#[test] +fn test_inner_layer_data_stays_unpadded() { + let chain = InnerLayerChain::new(vec![1, 2, 3]); + assert_eq!(chain.layers[0].data, vec![1, 2, 3]); +} + +#[test] +fn test_odd_length_flat_has_no_dead_byte() { + // 9 distinct bytes stay flat (splitting doesn't help); the emitted array must + // have exactly 9 elements, not a padded 10. + let data = vec![10i64, 20, 30, 40, 50, 60, 70, 80, 90]; + let (info, best) = pack_table(&data, Some(0), 10.0); + let code = generate(&info, best, "data", Language::C); + assert!( + code.contains(&format!("data_u8[{}]", data.len())), + "flat array should have exactly {} elements:\n{}", + data.len(), + code + ); + assert!( + !code.contains(&format!("data_u8[{}]", data.len() + 1)), + "flat array must not carry a dead padding byte:\n{}", + code + ); + compile_and_run_c(&code, &data, 0); } // ── F1: compression >= 10 must reach the true minimum-byte solution ── From 558be7e723bbf6e7b22873c79ddbb7c9da498fc0 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Wed, 1 Jul 2026 12:35:21 -0600 Subject: [PATCH 7/7] Make --analyze Score use exact log2, matching pick_solution --analyze computed the displayed Score with floor(log2) via ilog2, while pick_solution ranks 1..9 with exact log2, so the highlighted "Best solution" could disagree with the minimum-score row. Use exact log2 (and two decimals) so the ranking is visibly consistent. Ports part of harfbuzz/packtab@310edfc4f. Co-Authored-By: Claude Opus 4.8 (1M context) --- packtab-cli/src/main.rs | 12 +++++----- packtab-cli/tests/cli.rs | 50 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 6 deletions(-) diff --git a/packtab-cli/src/main.rs b/packtab-cli/src/main.rs index 20291c3..68b6f2d 100644 --- a/packtab-cli/src/main.rs +++ b/packtab-cli/src/main.rs @@ -247,13 +247,13 @@ fn print_analysis(data: &[i64], default: Option, compression: f64) { f64::INFINITY }; let full_cost = sol.full_cost(); - let score = if full_cost > 0 { - sol.n_lookups() as f64 + compression * ((full_cost as u64).ilog2() as f64) - } else { - sol.n_lookups() as f64 - compression - }; + // Use the same score pick_solution uses for 1..9 (exact log2, not floor + // via ilog2), so the highlighted "Best solution" matches the minimum-score + // row. Two decimals make the ranking visibly consistent. + let score = sol.n_lookups() as f64 + + compression * if full_cost > 0 { (full_cost as f64).log2() } else { 0.0 }; println!( - "{:<3} {:<8} {:<9} {:<6} {:<8} {:>6.2}x {:>7.1}", + "{:<3} {:<8} {:<9} {:<6} {:<8} {:>6.2}x {:>8.2}", i + 1, sol.n_lookups(), sol.n_extra_ops(), diff --git a/packtab-cli/tests/cli.rs b/packtab-cli/tests/cli.rs index 48180f1..62f138c 100644 --- a/packtab-cli/tests/cli.rs +++ b/packtab-cli/tests/cli.rs @@ -41,6 +41,56 @@ fn dual_compression_rejected_for_rust() { assert!(stderr.contains("dual compression")); } +#[test] +fn analyze_score_uses_exact_log2() { + // The displayed Score must equal pick_solution's exact-log2 formula + // (Lookups + compression*log2(FullCost)), not a floored ilog2, so the + // highlighted "Best solution" matches the minimum-score row. + let data: Vec = (0..600).map(|i| (i % 50).to_string()).collect(); + let mut args: Vec<&str> = vec!["--analyze", "--default", "0", "--compression", "3"]; + let refs: Vec<&str> = data.iter().map(|s| s.as_str()).collect(); + args.extend_from_slice(&refs); + let output = run(&args); + assert!(output.status.success()); + let stdout = String::from_utf8_lossy(&output.stdout); + + let compression = 3.0_f64; + let mut checked = 0; + for line in stdout.lines() { + let cols: Vec<&str> = line.split_whitespace().collect(); + // Data rows are: idx Lookups ExtraOps Bytes FullCost Ratio(x) Score + if cols.len() != 7 || cols[0].parse::().is_err() { + continue; + } + let (Ok(lookups), Ok(full_cost), Ok(score)) = ( + cols[1].parse::(), + cols[4].parse::(), + cols[6].parse::(), + ) else { + continue; + }; + let expected = lookups + compression * full_cost.log2(); + assert!( + (score - expected).abs() < 0.02, + "displayed score {} != exact {:.2} (lookups={}, fullcost={})\n{}", + score, + expected, + lookups, + full_cost, + stdout + ); + // Only a non-power-of-two FullCost distinguishes exact log2 from floor. + if (full_cost.log2().fract()) > 0.01 { + checked += 1; + } + } + assert!( + checked >= 1, + "no non-power-of-two rows to distinguish exact vs floor log2:\n{}", + stdout + ); +} + #[test] fn sparse_input_works() { let output = run(&["--sparse", "--default", "0", "7:42", "50:99"]);