Port codegen-soundness fixes from harfbuzz/packtab#9#2
Merged
Conversation
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
--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) <noreply@anthropic.com>
nicoburns
pushed a commit
to nicoburns/parley
that referenced
this pull request
Jul 6, 2026
## Intent Upgrade to the new PackTab 1.9.0. --- I previously tested the PR (behdad/packtab.rs#2) in linebender@22bbd1d prior to updating this PR to use the new 1.9.0 version. That investigation is below: Tested against cases our existing `--compression=5`, but also `--compression=0` and `--compression=10`. Our regressions tests pass for all cases - so, it's correct as far as Parley is concerned. At compression=10, I noticed a 7 byte reduction relative to current PackTab - I believe that's expected because the proposed PR drops dead padding bytes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ports codegen/pruning soundness fixes from upstream harfbuzz/packtab into the Rust crates: the #9 set (found by round-trip fuzzing) plus the two applicable follow-up commits.
From PR #9 (one commit each)
len*8bits, corrupting wide values (whenunit_bits >= 8,dataholdsunit_bits-wide elements, not bytes). Now packs atelem_bitsto match the extraction stride, and caps the const-type shift at 32 to avoid a1 << 63overflow.default. The lookup return type was sized from the stored data range only, so a negative or oversizeddefault(emitted out-of-range / for culled default-prefix indices) would wrap in C or fail to compile in Rust. Widened to includedefaultin both the direct and palette paths.add_array, like the data-array path. Latent through the public API (eachgenerate()uses a freshCodeBuilder); fixed for parity.(n_lookups, full_cost)frontier, socompression>=10(min bytes) andcompression<=0(flat) couldn't reach the true optimum. Now prunes to the union of thefull_costandcostfrontiers at every pruning site. The 1..9 heuristic is unaffected (the extra solutions are alwaysfull_cost-dominated).split()padding-aliasing bug can't occur here: the Rust port only handlesi64and always builds the inner chain from a freshly-ownedVec, with the bounds check keyed on the unpadded outer length. Added a guard test.From follow-up commit 310edfc4f
build_layerspaddedlayer.datain place, so odd-length flat (unsplit) tables emitted one unreachable trailing byte and the reported cost was one element short of the emission. Pad a local copy for pairing instead; split solutions and all picks are unchanged, odd-length flat arrays lose the dead byte (e.g.data_u8[10]→data_u8[9]).--analyzeScore uses exact log2. The analyze table computed Score with flooredilog2whilepick_solutionranks with exactlog2, so the highlighted "Best solution" could disagree with the minimum-score row. Now uses exact log2 (two decimals).Two other post-#9 upstream commits are N/A:
Repair dead Code.print_h(no C-header generation path in the Rust port) andUse NotImplemented as the infer-default sentinel(the Rust API takesOption<i64>; there's no dict/None-as-value input forNoneto collide with).Tests
Existing tests that asserted the old single-frontier invariant are updated to the union check. New round-trip/frontier/regression tests cover every fix; each was confirmed to fail without its fix. Full workspace suite (187 tests, including the
packtab-icu4xend-to-end round-trip over real Unicode data) passes.🤖 Generated with Claude Code