Skip to content

Fix overlapping memcpy in CoNgram history shift - #289

Merged
bab2min merged 2 commits into
bab2min:mainfrom
ollipa:fix-overlapping-history-memcpy
Oct 19, 2026
Merged

bab2min merged 2 commits into
bab2min:mainfrom
ollipa:fix-overlapping-history-memcpy

Conversation

@ollipa

@ollipa ollipa commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Depends on #288 (wasm stack size). This branch is on top of it, so the first commit is from that PR.

CoNgramModel::progress() shifts the distant-token history in place with memcpy(&history[0], &history[1], ...). The ranges overlap, which is undefined behaviour. glibc happens to handle it, but Emscripten's memcpy corrupts the history, so cong-global results in the WASM build differ from native. This PR uses memmove instead.

Example: 그는 죄를 지었기 때문에 감옥에 갔다.

  • native: 짓/VV-I 었/EP (−41.00)
  • WASM before: 지/VV 었/EP (−43.88)
  • WASM after: same as native

Testing:

  • New WASM test in fails without the fix, passes with it.
  • ASan on native x86-64 reports memcpy-param-overlap at this line.

Summary by CodeRabbit

  • Bug Fixes

    • Corrected tokenization of certain Korean word forms, including preserving 짓/VV-I when processing a sentence containing 지었기.
    • Improved handling of overlapping token history during model processing, helping ensure consistent results for Korean text.
  • Tests

    • Added checks for tokenization of sentences containing 생각, 기도, and 지었기.

Default stack is 64 KB and cong-global overflows it
on some sentences.
@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: cfbc4b7e-bc61-4da1-9fbe-4b24f9562bdc
📥 Commits

Reviewing files that changed from the base of the PR and between 28e8415 and 5d00ae0.

📒 Files selected for processing: 3
  • bindings/wasm/CMakeLists.txt
  • bindings/wasm/package/test/kiwi.test.ts
  • src/CoNgramModel.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

📝 Walkthrough

The change uses overlap-safe memory movement for CoNgram history, adds tokenization tests, and sets the WebAssembly stack size to 256 KB.

Changes: CoNgram history update

Layer / File(s) Summary
History shifting and tokenization tests
src/CoNgramModel.cpp, bindings/wasm/package/test/kiwi.test.ts
progress shifts history with memmove. Tests check expected tokens in two sentences, including 짓/VV-I instead of 지/VV.

WebAssembly stack size

Layer / File(s) Summary
WebAssembly stack size flag
bindings/wasm/CMakeLists.txt
The kiwi-wasm target adds -s STACK_SIZE=256KB to its link flags.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: sebjacobs

Merge Risk: ⚪ Minimal · up to 5d00a

The change is ready to merge after normal checks; the supported WASM test workflow builds its required binary first.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: replacing the overlapping memcpy operation in the CoNgram history shift.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@bab2min

bab2min commented Oct 19, 2026

Copy link
Copy Markdown
Owner

@ollipa Oh I didn't know that memcpy has a different behavior between compilers. Thank you for fixing it.

@bab2min
bab2min merged commit 060c61c into bab2min:main Oct 19, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants