Skip to content

Backport upstream LDM size-estimation fix - #359

Open
yousefissa wants to merge 1 commit into
indygreg:mainfrom
yousefissa:fix-ldm-size-estimation
Open

yousefissa wants to merge 1 commit into
indygreg:mainfrom
yousefissa:fix-ldm-size-estimation

Conversation

@yousefissa

@yousefissa yousefissa commented Oct 14, 2026 •

Copy link
Copy Markdown

Fixes #345.

Backport facebook/zstd#4591, authored by @fresh3nough (upstream commit 81cf153). Both context and stream size estimators resolve default LDM parameters on a local copy before estimation, avoiding division by zero without changing the caller's parameters. The vendor code changes match upstream exactly.

Regression coverage compares default and explicit LDM settings, includes a partial override and a different strategy, and checks parameter immutability and repeatability.

Verified on CPython 3.14.6, macOS ARM64:

  • Unpatched C and CFFI builds fail the regression; UBSan reports division by zero and aborts the issue reproducer. Patched reproducer and focused tests pass on both backends under UBSan; direct CCtx/CStream estimates also pass.
  • Full C suite: 249 passed, 44 skipped; CFFI: 225 passed, 68 skipped. Three regression subtests pass per backend. Optional slow fuzz tests were not enabled.
  • C extension builds with warnings as errors; Ruff checks and git diff --check pass.

Prepared with Codex assistance.

Resolve default LDM parameters before context and stream size estimation, using a local copy to preserve caller parameters. Add regression coverage for defaults, partial overrides, and strategy-dependent defaults.

Backport facebook/zstd commit 81cf153bce7c0ab41aeef574a70e32eaea7a8ba7 (PR #4591).

Fixes indygreg#345.

Co-authored-by: Cody <166262726+kowirth@users.noreply.github.com>
@yousefissa
yousefissa marked this pull request as ready for review October 14, 2026 22:01
@yousefissa

Copy link
Copy Markdown
Author

@indygreg Could you review this backport of facebook/zstd#4591 and approve CI? It fixes the LDM size-estimation crash; the C and CFFI suites both pass locally.

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.

estimated_compression_context_size() raises SIGFPE with `enable_ldm=True

1 participant