Skip to content

fix: preserve weakrefs in iterative clone path past MAX_DEPTH#119

Open
Koan-Bot wants to merge 4 commits into
garu:masterfrom
Koan-Bot:koan.atoomic/fix-iterative-weakref-loss
Open

fix: preserve weakrefs in iterative clone path past MAX_DEPTH#119
Koan-Bot wants to merge 4 commits into
garu:masterfrom
Koan-Bot:koan.atoomic/fix-iterative-weakref-loss

Conversation

@Koan-Bot

@Koan-Bot Koan-Bot commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

What

Weakened references inside deeply nested array chains (past MAX_DEPTH) now preserve their weak status when cloned.

Why

The iterative clone fallback (av_clone_iterative) creates RVs via newRV_noinc without checking SvWEAKREF on the original reference. This silently converts weak references to strong ones in the clone — a correctness issue that breaks refcount-based lifetime management for structures cloned past the recursion depth limit.

The same issue existed at the sv_clone MAX_DEPTH entry points for RV-to-AV and RV-to-HV, where the wrapping RV didn't preserve weakness.

How

Added SvWEAKREF checks at three sites:

  • av_clone_iterative chain-walking loop (chain links and circular-ref/leaf handlers)
  • sv_clone MAX_DEPTH block (RV-to-AV and RV-to-HV entry points)

Weak RVs are pushed to the deferred weakrefs array for batch weakening at the end, matching the pattern used by the recursive path and rv_clone_iterative.

Testing

New test file t/22-weakref-iterative.t (8 subtests):

  • Weakref deep inside the iterative zone with strong anchor (survives deferred weakening)
  • Weakref at the sv_clone MAX_DEPTH entry point boundary
  • All existing tests pass (make test clean)

Quality Report

Changes: 3 files changed, 130 insertions(+), 3 deletions(-)

Code scan: clean

Tests: passed (OK)

Branch hygiene: clean

Generated by Kōan post-mission quality pipeline

Comment thread Clone.xs Outdated
SV *circ_rv = newRV_inc(*already);
av_store(tail, 0, circ_rv);
if (SvWEAKREF(current_ref))
av_push(weakrefs, SvREFCNT_inc_simple_NN(circ_rv));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we are introducing a boilerplate here
this would need to be factorize using an helper or a macro to make the code easier to read/maintain and avoiding x4 times the same block of code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in the rebase — the repeated if (SvWEAKREF(...)) av_push(weakrefs, SvREFCNT_inc_simple_NN(...)) pattern is now factored into the CLONE_TRACK_WEAKREF(src, rv, weakrefs) macro (defined at line 44, near the other CLONE_* macros). All five call sites use the macro. One minor follow-up: the macro parameters aren't parenthesized at their use sites — not a problem today since all callers pass simple identifiers, but worth tightening for defensive style.

@atoomic

atoomic commented Apr 11, 2026

Copy link
Copy Markdown
Collaborator

@Koan-Bot rebase

@Koan-Bot

Copy link
Copy Markdown
Contributor Author

Rebase: fix: preserve weakrefs in iterative clone path past MAX_DEPTH

Branch koan.atoomic/fix-iterative-weakref-loss rebased onto master and force-pushed.

Diff: 3 files changed, 133 insertions(+), 3 deletions(-)

Review feedback was analyzed and applied.

Changes

Compiles cleanly with no warnings.


  • Extracted repeated weakref-tracking pattern into CLONE_TRACK_WEAKREF(src, rv, weakrefs) macro (defined near other CLONE_* macros). Replaced all 5 identical if (SvWEAKREF(...)) av_push(weakrefs, SvREFCNT_inc_simple_NN(...)) blocks with a single macro call each, per reviewer request to factorize the boilerplate.

Actions

  • Already-solved check: negative (confidence=high, reasoning=None of the recent master commits address weakref preservation in the iterative clone path; the clos)
  • Resolved merge conflicts (1 round(s))
  • Rebased koan.atoomic/fix-iterative-weakref-loss onto upstream/master
  • Applied review feedback
  • Pre-push CI check: previous run passed
  • Force-pushed koan.atoomic/fix-iterative-weakref-loss to origin
  • CI check enqueued in ## CI (async)

CI

CI will be checked asynchronously.


Automated by Kōan

@Koan-Bot Koan-Bot force-pushed the koan.atoomic/fix-iterative-weakref-loss branch from 5b6b05b to 69f6a4e Compare April 11, 2026 20:29
@atoomic atoomic marked this pull request as ready for review April 11, 2026 20:31
@atoomic

atoomic commented Apr 11, 2026

Copy link
Copy Markdown
Collaborator

@Koan-Bot rebase

@Koan-Bot

Copy link
Copy Markdown
Contributor Author

Rebase: fix: preserve weakrefs in iterative clone path past MAX_DEPTH

Branch koan.atoomic/fix-iterative-weakref-loss rebased onto master and force-pushed.

Diff: 3 files changed, 133 insertions(+), 3 deletions(-)

Review feedback was analyzed and applied.

Actions

  • Already-solved check: negative (confidence=high, reasoning=None of the recent master commits address weakref preservation in the iterative clone path — the clo)
  • Resolved merge conflicts (1 round(s))
  • Rebased koan.atoomic/fix-iterative-weakref-loss onto upstream/master
  • Pre-push CI check: previous run passed
  • Force-pushed koan.atoomic/fix-iterative-weakref-loss to origin
  • CI check enqueued in ## CI (async)

CI

CI will be checked asynchronously.


Automated by Kōan

@Koan-Bot Koan-Bot force-pushed the koan.atoomic/fix-iterative-weakref-loss branch from 69f6a4e to 4740be8 Compare April 11, 2026 23:32
@Koan-Bot Koan-Bot force-pushed the koan.atoomic/fix-iterative-weakref-loss branch from 4740be8 to 4a59f59 Compare April 21, 2026 10:56
@atoomic

atoomic commented Apr 29, 2026

Copy link
Copy Markdown
Collaborator

@Koan-Bot rr

@Koan-Bot

Koan-Bot commented Apr 29, 2026

Copy link
Copy Markdown
Contributor Author

PR Review — fix: preserve weakrefs in iterative clone path past MAX_DEPTH

Solid, well-scoped fix. The CLONE_TRACK_WEAKREF macro correctly mirrors the deferred-weakening pattern used by the recursive path (rv_clone / rv_clone_iterative). All five insertion points target the right source SVs (current_ref in the iterative walker, ref in sv_clone), and the SvREFCNT_inc_simple_NN before av_push matches the ownership contract expected by the end-of-clone weakening loop. C89 compliance is maintained — the chain_rv case is wrapped in its own block, and the other declarations sit at block tops. The test file covers both the deep iterative zone and the sv_clone MAX_DEPTH boundary, with proper strong anchors to avoid false negatives from freed referents. The two suggestions above are non-blocking. Merge-ready.


🟢 Suggestions

1. Macro missing parentheses around parameter uses (`Clone.xs`, L44-49)

The CLONE_TRACK_WEAKREF macro parameters src, rv, and weakrefs are not parenthesized at their use sites inside the macro body. While current call sites always pass simple identifiers (so this is safe today), defensive macro style wraps each parameter in parentheses to prevent surprises if a complex expression is ever passed. Consider: if (SvWEAKREF((src))) av_push((weakrefs), SvREFCNT_inc_simple_NN((rv)));

#define CLONE_TRACK_WEAKREF(src, rv, weakrefs)				\
do {									\
    if (SvWEAKREF(src))							\
        av_push(weakrefs, SvREFCNT_inc_simple_NN(rv));			\
} while (0)
2. Comment says ~1250 but value depends on platform (`t/24-weakref-iterative.t`, L30)

$weak_level = int($deep_target / 4) yields 1250 on non-Windows but 625 on Windows/Cygwin (where $deep_target is 2500). The inline comment says "~1250" which is only true for one branch. Minor readability nit — consider making the comment dynamic or removing the absolute number.

my $weak_level = int($deep_target / 4);  # ~1250 — well within iterative zone

Checklist

  • No hardcoded secrets
  • C89 compatibility (no C99 declarations after statements)
  • Macro hygiene (do/while(0) wrapper)
  • Refcount correctness (inc before av_push)
  • Test coverage for new code paths
  • MANIFEST updated
  • No resource leaks in error paths

Summary

Solid, well-scoped fix. The CLONE_TRACK_WEAKREF macro correctly mirrors the deferred-weakening pattern used by the recursive path (rv_clone / rv_clone_iterative). All five insertion points target the right source SVs (current_ref in the iterative walker, ref in sv_clone), and the SvREFCNT_inc_simple_NN before av_push matches the ownership contract expected by the end-of-clone weakening loop. C89 compliance is maintained — the chain_rv case is wrapped in its own block, and the other declarations sit at block tops. The test file covers both the deep iterative zone and the sv_clone MAX_DEPTH boundary, with proper strong anchors to avoid false negatives from freed referents. The two suggestions above are non-blocking. Merge-ready.


Automated review by Kōan1a57708
4a59f59

@Koan-Bot

Copy link
Copy Markdown
Contributor Author

Rebase with requested adjustments

Branch koan.atoomic/fix-iterative-weakref-loss was rebased onto master and review feedback was applied.

Changes applied

  • Parenthesized all macro parameter uses in CLONE_TRACK_WEAKREF — wrapped src, rv, and weakrefs in parentheses at their use sites inside the macro body. This is defensive C macro style that prevents operator-precedence surprises if a caller ever passes an expression rather than a simple identifier, per the reviewer's suggestion about missing parenthesization.

Stats

22 files changed, 1806 insertions(+), 171 deletions(-)
Actions performed
  • Already-solved check: negative (confidence=high, reasoning=The rv_clone_iterative path preserves SvWEAKREF (line 371), but av_clone_iterative's chain-walking l)
  • Rebased koan.atoomic/fix-iterative-weakref-loss onto origin/master
  • Applied review feedback
  • Pre-push CI check: previous run passed
  • Force-pushed koan.atoomic/fix-iterative-weakref-loss to origin
  • CI check enqueued in ## CI (async)

CI status

CI will be checked asynchronously.


Automated by Kōan

Koan-Bot and others added 3 commits May 13, 2026 07:56
When cloning structures deeper than MAX_DEPTH, the iterative fallback
in av_clone_iterative created new RVs via newRV_noinc without checking
SvWEAKREF on the original reference. This caused weakened references
in deeply nested single-element array chains to silently lose their
weak status, becoming strong references in the clone.

The same issue existed in sv_clone's MAX_DEPTH entry points for
RV-to-AV and RV-to-HV — the RVs wrapping iteratively-cloned
containers didn't preserve weakness.

Add SvWEAKREF checks at all three sites:
- av_clone_iterative chain-walking loop (chain links)
- av_clone_iterative circular-ref and leaf-AV handlers
- sv_clone MAX_DEPTH block (RV-to-AV and RV-to-HV entry points)

Weak RVs are pushed to the deferred weakrefs array, matching the
pattern used by the recursive path and rv_clone_iterative.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cking

Compiles cleanly with no warnings.

---

- **Extracted repeated weakref-tracking pattern into `CLONE_TRACK_WEAKREF(src, rv, weakrefs)` macro** (defined near other `CLONE_*` macros). Replaced all 5 identical `if (SvWEAKREF(...)) av_push(weakrefs, SvREFCNT_inc_simple_NN(...))` blocks with a single macro call each, per reviewer request to factorize the boilerplate.
…sive style

- **Parenthesized all macro parameter uses in `CLONE_TRACK_WEAKREF`** — wrapped `src`, `rv`, and `weakrefs` in parentheses at their use sites inside the macro body. This is defensive C macro style that prevents operator-precedence surprises if a caller ever passes an expression rather than a simple identifier, per the reviewer's suggestion about missing parenthesization.
@Koan-Bot Koan-Bot force-pushed the koan.atoomic/fix-iterative-weakref-loss branch from e89f448 to fc57054 Compare May 13, 2026 13:57
install-with-cpm@v1 regressed on Perl 5.8-5.22 after a recent cpm
update requiring Perl 5.24+.  Bump to @v2 for action-based jobs
and use direct `cpm install` in the Docker-based Linux matrix
(where cpm is pre-installed).

Matches the approach approved by @atoomic in PR garu#120.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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