[codex] Fix dense symbol map offsets - #3465
Conversation
Coverage Report for CI Build 4214Coverage decreased (-0.001%) to 94.613%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
The |
|
@hackwaly You're right that
Eight profiles, all with On perf: the lookup at Where I'd push back: the configuration that produced this bug is exactly " Three options I see, in increasing order of structural safety:
I lean toward (3): it gets your perf back and removes the footgun. If you'd prefer (1) for now I can document the invariant explicitly with an assert in Happy to rework either way — what's your call? |
|
I agree 3) is best. |
6af08cc to
e1704e2
Compare
There was a problem hiding this comment.
Pull request overview
Adjusts regex-engine symbol-map finalization to avoid dense-table out-of-bounds indexing by changing finalize to work with absolute Rechar indices (dropping the explicit lower-bound parameter) and updating call sites accordingly.
Changes:
- Changed
SymbolMap::finalize(bytes + string) signature from(lb, ub)to(ub)and updated compile call sites. - Updated bytes symbol-map dense/sparse construction logic (dense allocation and interval iteration) to use absolute indexing.
- Added a bytes symbol-map unit test around dense-table absolute indexing behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| string/internal/regex_engine/symbol_map/symbol_map.mbt | Drops lb from finalize and changes representative (repr) construction. |
| string/internal/regex_engine/symbol_map/pkg.generated.mbti | Regenerates API signature for SymbolMap::finalize. |
| string/internal/regex_engine/compile.mbt | Updates symbol-map finalization call to pass only ub. |
| bytes/internal/regex_engine/symbol_map/symbol_map.mbt | Changes dense/sparse selection and dense table construction to use absolute indexing. |
| bytes/internal/regex_engine/symbol_map/symbol_map_test.mbt | Adds test asserting dense-table absolute indexing behavior. |
| bytes/internal/regex_engine/symbol_map/pkg.generated.mbti | Regenerates API signature for SymbolMap::finalize. |
| bytes/internal/regex_engine/compile.mbt | Updates symbol-map finalization call to pass only ub. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let table = FixedArray::make(ub + 1, 0) | ||
| self.each_intervals(0, ub, (lo, hi) => { | ||
| let symbol = repr.length() | ||
| repr.push(lo) | ||
| for c in lo..<=hi { |
| if ub < 1024 { | ||
| let (table, repr) = self.finalize_dense(ub) | ||
| (table as &Table, repr) | ||
| } else { | ||
| let (table, repr) = self.finalize_sparse(lb, ub) | ||
| let (table, repr) = self.finalize_sparse(ub) |
| let symbol_map = @symbol_map.new() | ||
| symbolize(profile~, symbol_map~, ast) | ||
| let (symbol_table, symbol_repr) = symbol_map.finalize(profile.lb, profile.ub) | ||
| let (symbol_table, symbol_repr) = symbol_map.finalize(profile.ub) |
| self.each_intervals(0, ub, (lo, hi) => { | ||
| let symbol = repr.length() | ||
| repr.push(Int::max(lo, lb)) | ||
| repr.push(lo) | ||
| for c in lo..<=hi { | ||
| table[c] = symbol |
| test "dense table uses absolute indexes" { | ||
| let symbol_map = @symbol_map.new() | ||
| symbol_map.split(@shared_types.RecharSet::char(11)) | ||
| let (table, repr) = symbol_map.finalize(12) | ||
| assert_eq(repr.length(), 3) | ||
| assert_eq(repr[0], 0) | ||
| assert_eq(repr[1], 11) | ||
| assert_eq(repr[2], 12) |
|
I'll file a new PR for 3). |
Summary
Fix dense symbol-map finalization for non-zero lower bounds in
bytes/internal/regex_engine/symbol_map.Dense tables are allocated as
ub - lb + 1, but the old implementation indexed them with absoluteRecharvalues. Callingfinalize(10, 12)could therefore index position10in a length-3 table and trap. The dense table now storeslb, writes entries atc - lb, and maps with the same offset.Validation
moon fmt bytes/internal/regex_engine/symbol_mapmoon info bytes/internal/regex_engine/symbol_mapmoon test bytes/internal/regex_engine/symbol_mapmoon test bytes/internal/regex_enginemoon checkmoon test