interp: fix inverted can_end guard in eliminate_shared_loans - #27
Open
karthikbhargavan wants to merge 6 commits into
Open
interp: fix inverted can_end guard in eliminate_shared_loans#27karthikbhargavan wants to merge 6 commits into
karthikbhargavan wants to merge 6 commits into
Conversation
Co-authored-by: Alexander Bentkamp <alexander@cryspen.com>
Co-authored-by: Alexander Bentkamp <alexander@cryspen.com>
Co-authored-by: Alexander Bentkamp <alexander@cryspen.com>
The guard selecting which abstractions eliminate_shared_loans updates was
inverted with respect to its own comment ("Only update the non-frozen
abstractions"): it updated an abstraction exactly when `not abs.can_end`,
i.e. exactly the frozen ones (input abstractions are created with
`can_end = false`), and skipped the endable join abstractions the pass was
written to clean up.
Because eliminate_shared_loans runs at the end of every reduce_ctx (and in
collapse_ctx), i.e. on every loop fixed-point round and every join, a
matchless shared loan sitting inside a frozen input abstraction at loop
entry caused the frozen abstraction to be mutated during the first loop
join. The mutated abstraction then failed the strict-equality recognition
used by compute_fixed_abs_ids, dropped out of fixed_aids, was marker-split
and re-kinded endable, and match_ctx_with_target later tried to *merge*
the input abstraction; input-abstraction continuations have no output, so
merge_abs_conts hit `[%craise] "Unreachable"` (InterpAbs.ml) — a spurious
failure (in both translation and -borrow-check modes) on rustc-valid code.
Invert the guard to match the comment, and strengthen the comment to
explain why frozen abstractions must not be touched. Cross-check: the
sibling end_endable_shared_loans_at_abs (InterpAbs.ml) implements the same
can_end polarity correctly.
Fixes #24
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Right, this is clearly an accidental error in the code. However, I don't think the lengthy comment is necessary. Instead, adding one or two of the examples you found as tests might be useful. |
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.
Bug
eliminate_shared_loans(src/interp/InterpReduceCollapse.ml) removes shared loans that have no matching shared borrow. Its guard was inverted w.r.t. its own comment ("Only update the non-frozen abstractions"):can_end = falseis the frozen case (input abstractions are created withregion_can_end _ = false), so the pass mutated exactly the frozen abstractions and skipped the endable join abstractions it was written to clean up. The siblingend_endable_shared_loans_at_abs(InterpAbs.ml) implements the samecan_endpolarity correctly.Mechanism
eliminate_shared_loansruns at the end of everyreduce_ctxand incollapse_ctx, i.e. on every loop fixed-point round and every join — contexts where frozen-abstraction immutability is load-bearing (join_prefixes' hardsanity_check,compute_fixed_abs_ids). When a matchless shared loan sits inside the frozen input abstraction at loop entry (e.g. afterlet s = *x; x = y;), the first loop join mutates the frozen abstraction. It then fails the strict-equality recognition incompute_fixed_abs_ids, drops out offixed_aids, is marker-split and re-kinded endable, andmatch_ctx_with_targetlater tries to merge the input abstraction — whose continuation has no output — somerge_abs_contshits[%craise] "Unreachable"(InterpAbs.ml:1688ondev). Result: a spurious failure, in both translation and-borrow-checkmodes, on rustc-valid code.Fixes #24
Fix
Invert the guard to match its comment (
if abs.can_end then ... else abs) and strengthen the comment to state why frozen abstractions must not be touched.Validation
All with charon at the pinned commit (v0.1.223); repro files from the issue.
devbuild):f6_matchless_loan.rsandf6_variant_loop_ignores_x.rsfail with[Error] Unreachable(interp/InterpAbs.ml, line 1688) in-backend leanand are rejected by-borrow-check; both controls (control_no_loop.rs,control_loan_kept_alive.rs) pass.y's value (def f (x y : Std.U32) ... := f_loop y x 0#u32); both controls still pass;-borrow-checkaccepts all four.extract-testssuite (with-checks) passes; every regenerated backend output is byte-identical to the committed files (git statusclean apart from this fix). Additionally, the patched binary was run with-backend leanon all 101 regeneratedtests/llbc/*.llbcfixtures: 95 pass, and the 6 failures (borrow_check_negative,loops_borrow_check_negative,higher_ranked_implied_bounds_{borrow,regions,types},raw_pointers) fail identically under an unpatcheddevbuild — pre-existing expected failures, not caused by this patch.dune buildanddune build @fmtpass.🤖 Generated with Claude Code