Reconcile ARPA writer output with actual trie counts - #501
Open
lenzo-ka wants to merge 2 commits into
Open
Conversation
ngram_model_trie_write_arpa wrote the n-gram section counts from the stored header (base->n_counts) and asserted that the number of n-grams recovered by walking the trie matched. When a model's header disagrees with its trie content, that assertion aborted; with assertions disabled the writer indexed the raw_ngram array beyond the entries actually filled, dereferencing NULL word pointers or writing past the allocation. Discover the true per-order counts by walking the trie before writing anything, and emit those counts so the output is always self-consistent. lm_trie_fill_raw_ngram now takes a capacity: it stops writing once the array is full but keeps counting, so the true count is known without overflowing. When the header and the trie content disagree the writer logs a warning and writes the actual counts. Output for a consistent model is unchanged.
test-lm-convert-mismatch.sh converts the shipped en-us model, whose header claims more bigrams than its trie holds: the writer warns, does not crash, and produces an ARPA whose declared counts match the lines in each section. test_lm_write_consistency checks that a consistent model still writes an ARPA whose counts match its sections.
lenzo-ka
force-pushed
the
arpa_writer_count_check
branch
from
August 15, 2026 19:40
1d3e5e6 to
713adca
Compare
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
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.
ngram_model_trie_write_arpawrote each n-gram section's count from the stored header (base->n_counts) and asserted that the number of n-grams recovered by walking the trie matched. When a model's header disagrees with its trie content, that assertion aborts the program; with assertions disabled the writer indexes theraw_ngramsarray beyond the entries actually filled, dereferencing NULL word pointers or writing past the allocation.The shipped
model/en-us/en-us.lm.bintriggers this: its header declares 2051547 bigrams while the trie holds 2051541 (the six extra header slots are zero-padding). Converting it to ARPA aborts.This discovers the true per-order counts by walking the trie before writing anything, and emits those counts so the output is always self-consistent.
lm_trie_fill_raw_ngramnow takes a capacity: it stops writing once the destination array is full but keeps counting, so the actual count is known without overflowing. When the header and the trie content disagree, the writer logs a warning and writes the actual counts; conversion of the en-us model now succeeds with a warning instead of aborting. Output for a consistent model is unchanged (byte-identical).test-lm-convert-mismatch.shconverts the en-us model and checks that the writer warns, does not crash, and produces an ARPA whose declared counts equal the lines in each section.test_lm_write_consistencychecks that a consistent model still writes an ARPA whose counts match its sections.Tested on macOS (Apple clang) and Linux x86-64 (GCC 13.3); full unit and regression suite passes on both.
Fixes #502